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