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
pgp3AmMtkv8SY.pgp
Description: PGP signature