On 29 October 2012 20:14, Christian Kruse <cjk+postgres@defunct.ch> wrote:
> created a patch which implements MAP_HUGETLB for sysv shared memory segments
> (PGSharedMemoryCreate). It is based on tests of Tom Lane and Andres Freund, I
> added error handling, huge page size detection and a GUC variable.
I have a few initial observations on this.
* 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.
* 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)
* 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. 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.
* 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
* You aren't following our style guide with respect to error messages [2].
[1] https://www.kernel.org/doc/Documentation/vm/map_hugetlb.c
[2] http://www.postgresql.org/docs/devel/static/error-style-guide.html
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
