Re: Re: [HACKERS] Patch für MAP_HUGETLB for mmap() shared memory

On Tuesday, October 30, 2012 08:20:33 PM Christian Kruse wrote:
> Hey,
>
> Oh man, first I didn't sent the email to the list and now I forgot the
> attachment. I should really get some sleep, sorry for any
> inconveniences :(

+#ifdef MAP_HUGETLB
+# ifdef __ia64__
+# define PG_HUGETLB_BASE_ADDR (void *)(0x8000000000000000UL)
+# define PG_MAP_HUGETLB (MAP_HUGETLB|MAP_FIXED)
+# else

Not your fault, but that looks rather strange to me. The level of documentation
around the requirement of using MAP_FIXED is subpar...

+long
+InternalGetHugepageSize()
+{
+...
+ if (biggest_size < size && InternalGetFreeHugepagesCount(ent-
>d_name) > 0)
+ {
+ biggest_size = size;
+ }

I dislike automatically using the biggest size here. There are platforms with
16GB hugepages... I think we should rather always use the smallest size and
leave it to the kernel to choose whatever pagesize he wants to use.

+#ifdef MAP_HUGETLB
+ long pagesize = InternalGetHugepageSize();
+
+ if (pagesize <= 0)
+ pagesize = sysconf(_SC_PAGE_SIZE);
+#else
long pagesize = sysconf(_SC_PAGE_SIZE);
+#endif

Thats going to log two warnings if the current system doesn't have hugepage
support compiled in and thus no /sys/kernel/mm/hugepages directory.
I think you should 1. only test this if TRY or ON is configured, 2. make the
messages in InternalGetHugepageSize DEBUG1 at least for the TRY case.

+ {
+ {"huge_tlb_pages",
+#ifdef MAP_HUGETLB
+ PGC_SUSET,
+#else
+ PGC_INTERNAL,
+#endif
+ RESOURCES_MEM,
+ gettext_noop("Enable/disable the use of the hugepages feature"),
+ NULL
+ },
+ &huge_tlb_pages,
+ HUGE_TLB_TRY, huge_tlb_options,
+ NULL, NULL, NULL
+ },

you use HUGE_TLB_TRY with ifdef here.

Looking forward to having this...

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services