[PATCH v8] elf: Support THP segment load with madvise enabled THP

[Wilco Dijkstra]

Hi HJ,

A few comments:

+static inline void
+_dl_get_thp_config (void)
+{
+  /* Check if there is GLIBC_TUNABLES=glibc.elf.thp=[0|1].  */
+  if (TUNABLE_IS_INITIALIZED_FULL (glibc, elf, thp))
+    GL(dl_elf_thp_control) = TUNABLE_GET_FULL (glibc, elf, thp, int32_t,
+					       NULL);
+  else
+    GL(dl_elf_thp_control) = 0;

Wouldn't it be easier to do:
  
 GL(dl_elf_thp_control) = TUNABLE_GET_FULL (glibc, elf, thp, int32_t, NULL);

+  /* Return if THP is disabled by GLIBC_TUNABLES=glibc.elf.thp=0.  */
+  if (GL(dl_elf_thp_control) == 0)
+    return;
+
+  _Static_assert (DL_MAP_DEFAULT_THP_PAGESIZE <= MAX_THP_PAGESIZE,
+		  "DL_MAP_DEFAULT_THP_PAGESIZE <= MAX_THP_PAGESIZE");
+
+  /* NB: Accessing /sys/kernel/mm files is quite expensive and the file
+     may not be accessible in containers.  If DL_MAP_DEFAULT_THP_PAGESIZE
+     is non-zero, assume THP mode is madvise and always call madvise.
+     Since madvise is a fast systemcall, it adds only a small overhead
+     compared to the cost of accessing /sys/kernel/mm files.  */
+  if (DL_MAP_DEFAULT_THP_PAGESIZE != 0)
+    {
+      GL(dl_elf_thp_pagesize) = DL_MAP_DEFAULT_THP_PAGESIZE;
+      GL(dl_thp_mode) = thp_mode_madvise;
+    }
+  else
+    {
+      GL(dl_elf_thp_pagesize) = __get_thp_size ();
+      GL(dl_thp_mode) = __get_thp_mode ();
+      /* We cap the huge page size at MAX_THP_PAGESIZE to avoid
+	 over-aligning on systems with very large normal pages
+	 (like 64K pages with 512M huge pages).  */
+      if (GL(dl_elf_thp_pagesize) > MAX_THP_PAGESIZE)
+	GL(dl_elf_thp_control) = 0;

Should also check if GL(dl_elf_thp_pagesize) == 0 (for if __get_thp_size() fails).

+      /* NB: Enable THP if THP is always enabled in the kernel.  */
+      else if (GL(dl_thp_mode) == thp_mode_always)
+	GL(dl_elf_thp_control) = 1;

This does not make sense. It's already 1 if we get here. This should check
mode is madvise or always and set dl_elf_thp_control = 0 if not (since we
can get thp_mode_not_supported too).

And in both cases we should reset dl_elf_thp_pagesize and dl_thp_mode since
there are some uses that do not first check dl_elf_thp_control > 0 (eg. 
_dl_postprocess_loadcmd_extra_needed).

+    }
+}


 ElfW (Addr)
 _dl_map_segment_align (const struct loadcmd *c, ElfW (Addr) p_align_max)
 {
-  static enum thp_mode_t thp_mode = thp_mode_not_supported;
-  static unsigned long int thp_pagesize;
+  enum thp_mode_t thp_mode = GL(dl_thp_mode);
+  size_t thp_pagesize = GL(dl_elf_thp_pagesize);
 
-  if (TUNABLE_GET (glibc, elf, thp, int32_t, NULL) == 0)
+  if (GL(dl_elf_thp_control) <= 0
+      || p_align_max >= thp_pagesize
+      || !(thp_mode == thp_mode_always || thp_mode == thp_mode_madvise))
     return p_align_max;

Now this check for thp_mode is no longer needed.
 

+static inline bool
+_dl_postprocess_loadcmd_extra_needed (void)
+{
+  return GL(dl_thp_mode) == thp_mode_madvise;
+}

I don't see the point of this interface. It's evaluated, the result passed around,
and then eventually _dl_postprocess_loadcmd_extra is called. Why not call it
unconditionally and do the check before calling _dl_segment_thp_eligible?

+
+/* After L has been mapped in, call madvise with MADV_HUGEPAGE if L is
+   THP eligible.  */
+
+static inline void
+_dl_postprocess_loadcmd_extra (struct link_map *l, const struct loadcmd *c)
+{
+  if (_dl_segment_thp_eligible (c, GL(dl_elf_thp_pagesize)))

Why not do:

if (GL(dl_thp_mode) == thp_mode_madvise
   && _dl_segment_thp_eligible (c, GL(dl_elf_thp_pagesize)))

+    __madvise ((void *) (l->l_addr + c->mapstart),
+	       c->mapend - c->mapstart, MADV_HUGEPAGE);
+}

Cheers,
Wilco