[PATCH v6] malloc: Mark pages with MADV_DONTNEED to shrink and grow rather than mremap()

[Wilco Dijkstra]

Hi William,

+static __always_inline bool
+_int_realloc_madvise (mchunkptr oldp, size_t difference)
+{
+  /* Only MADV_DONTNEED if allowing overcommit memory, otherwise there
+     is a risk of exhausting the process' pre-determined VMA fraction.  */
+  if (__glibc_unlikely (!check_can_keep_vma ()))
+    return false;

I don't believe we need this check - it's never wrong to use madvise.

+  char *madv_start = (char *) mmap_base (oldp) + mmap_size (oldp) - difference;
+  return __madvise (madv_start, difference, MADV_DONTNEED) != -1;
+}

Only this part needs to check !check_may_shrink_heap():

+      difference = ALIGN_DOWN (difference, GLRO (dl_pagesize));
+      /* Don't shrink more than the relative threshold for the chunk.  */
+      if (difference <= max_madvise (mmap_size (oldp)) &&
+          _int_realloc_madvise (oldp, difference))
+          return oldmem;
+    }

Then if mremap fails/doesn't exist, we still use madvise since that does reduce
actual memory usage.

+/* When disabling overcommit behavior, we want to make sure that we are
+   judicious with our heap usage as well, and explicitly give away the freed
+   top of the heap to reduce our commit charge.  */
+static inline bool
+check_may_shrink_heap (void)
+{
+  static int may_shrink_heap = -1;
+
+  return may_shrink_heap >= 0 || check_can_keep_vma ();
 }

This removes the caching of the expensive systemcall... It may be worth renaming
to something more useful if we can think of a better name.

Note +  return !overcommitted; incorrectly flips the existing meaning of true/false.


Florian: I am not sure what the __libc_enable_secure has to do with overcommit - so
I am wondering whether we could remove that check altogether?

Note there is an existing bug in that it is not thread-safe:

static inline bool
check_may_shrink_heap (void)
{
  static int may_shrink_heap = -1;

  if (__builtin_expect (may_shrink_heap >= 0, 1))        // should use __glibc_likely
    return may_shrink_heap;

  may_shrink_heap = __libc_enable_secure;        // first write

  if (__builtin_expect (may_shrink_heap == 0, 1))  // spurious read (may not return same value as __libc_enable_secure)
    {
      int fd = __open_nocancel ("/proc/sys/vm/overcommit_memory",
                                O_RDONLY | O_CLOEXEC);
      if (fd >= 0)
        {
          char val;
          ssize_t n = __read_nocancel (fd, &val, 1);
          may_shrink_heap = n > 0 && val == '2';    // 2nd write
          __close_nocancel_nostatus (fd);
        }
    }

  return may_shrink_heap;      // spurious read (may return different value from last write)
}

Basically one can do one read of the global and one write if the read says it is not
initialized - otherwise you end up with glitches and incorrect values returned.

Cheers,
Wilco