Hi,
On 29/10/12 21:14, Peter Geoghegan wrote:
> I have a few initial observations on this.
Thanks for your feedback.
>
> * I think you should be making the new GUC PGC_INTERNAL on platforms
> where MAP_HUGETLB is not defined or available. See also,
> effective_io_concurrency. This gives sane error handling.
Ok, sounds good for me.
> * Why aren't you failing when HUGE_TLB_ON and the mmap fails? You do
> the same thing as HUGE_TLB_TRY, contrary to your documentation:
>
> + if (huge_tlb_pages == HUGE_TLB_ON || huge_tlb_pages == HUGE_TLB_TRY)
No, what I did was mmap()ing the memory with MAP_HUGETLB and falling
back to mmap() w/o MAP_HUGETLB when mmap() failed. But there was
another bug, I didn't mmap() at all when huge_tlb_pages == off.
> * I think you should add MAP_HUGETLB to PG_MMAP_FLAGS directly, rather
> than XOR'ing after the fact. We already build the flags that comprise
> PG_MMAP_FLAGS using another set of intermediary flags, based on
> platform considerations, so what you're doing here is just
> inconsistent.
This would mean that I have to disable the bit when huge_tlb_pages ==
off. I think it is better to enable it if wanted and to just leave the
flags as they were when this feature has been turned off, do you
disagree?
> Don't wrap the mmap() code in ifdefs, and instead rely
> on the GUC being available on all platforms, with setting the GUC
> causing an error on unsupported platforms, in the style of
> effective_io_concurrency, as established in commit
> 3b07182e613a0e63ff662e3dc7f820f010923ec7 . Build a PG_MAP_HUGETLB
> intermediary flag on all platforms.
Ok, this sounds good. It will remove complexity from the code.
> * Apparently you're supposed to do this for the benefit of Itanic [1]:
>
> /* Only ia64 requires this */
> #ifdef __ia64__
> #define ADDR (void *)(0x8000000000000000UL)
> #define FLAGS (MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB | MAP_FIXED)
> #else
> #define ADDR (void *)(0x0UL)
> #define FLAGS (MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB)
> #endif
Hm… is this enough? Or do we have to do more work for ia64?
> * You aren't following our style guide with respect to error messages [2].
Thanks, I wasn't aware of this. I attached a new version of the patch.
Greetings,
CK
