From: | Christian Kruse <cjk+postgres(at)defunct(dot)ch> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Re: [HACKERS] Patch für MAP_HUGETLB for mmap() shared memory |
Date: | 2012-10-30 19:16:33 |
Message-ID: | 20121030191633.GE2330@achilles.local.defunct.ch |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Christian Kruse | 2012-10-30 19:18:18 | Re: Patch für MAP_HUGETLB for mmap() shared memory |
Previous Message | Andres Freund | 2012-10-30 15:32:31 | Re: [PATCH 3/8] Add support for a generic wal reading facility dubbed XLogReader |