This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][RFC] Allow explicit shrinking of arena heaps using anenvironment variable


Hi,

Here you go Carlos:

malloc for multithreaded processes leads to the creation of arenas for
each thread (up to a particular limit) to reduce locking contention
during allocation on heap. These arenas (64M each on x86_64) are
initially allocated without permissions (PROT_NONE) and then 'grown' by
giving read+write permissions as required.

However, when it comes to shrinking arenas, we only use
madvise(MADV_DONTNEED) to tell the kernel that we may not use this
space in future. This is different for setuid programs, where we shrink
their arenas by doing an mmap(PROT_NONE, MAP_FIXED) on the section of
the arena to be dropped so as to drop permissions on the region and
give the pages back to the kernel.

The MADV_DONTNEED does not have any visible effect in /proc/PID/maps and
this was what I wanted to change with my original patch, since I
initially thought of /proc/PID/maps as a way to figure out resource
usage of a program. I realized that RSS usage monitoring may be a
better idea than that.

However, Rich Felker and others pointed out that MADV_DONTNEED does not
affect the commit charge and hence, doing that to release pages back to
the kernel is not good enough in some cases. One should set those
mappings to PROT_NONE as well so that the commit charge is actually
given back, thus suggesting that mprotect(PROT_NONE) be done on the
mapping to achieve this. I had seen cases of programs (for RHEL
customers) getting OOM killed in RHEL-6, that I can relate those
problems to this observation in retrospect, but this is only
circumstantial since I don't have a way to verify this now. I do have
experimental verification that I describe later.

There were also suggestions by Roland and Jakub that a benchmark would
help us decide if there is any real performance benefit in just doing
MADV_DONTNEED for regular programs instead of the mmap(PROT_NONE,
MAP_FIXED) that setuid programs do. If the performance benefit is not
that noticeable then we might as well do mmap(PROT_NONE, MAP_FIXED)
unconditionally.

KOSAKI Motohiro further clarified Rich Felker's point, saying that
commit charge is given back only when the region is unmapped, either
directly through an munmap call or by doing the mmap(PROT_NONE,
MAP_FIXED) on an already existing mapping, similar to what we do to
shrink arenas for setuid programs.

I then did a few tests based on this information and came up with
observations described here:

http://sourceware.org/ml/libc-alpha/2012-08/msg00174.html

The observations can be summarized as follows:

1) madvise(MADV_DONTNEED) is only about 25us faster on average than
   mmap(PROT_NONE, MAP_FIXED) and given that the average speed of
   either of those calls is in the range of 5500us, that is barely a
   0.5% difference.
2) With overcommit disabled/throttled by setting vm.overcommit_memory
   to 2, only mmap(PROT_NONE, MAP_FIXED) is effective in reducing the
   commit charge of a process and hence in such a scenario, is the only
   correct option for releasing resources back to the kernel

Based on this, I propose making the heap shrinking unconditionally use
mmap(PROT_NONE, MAP_FIXED). I have been using the attached patch to do
this since about 24 hours now on my Fedora 16 x86_64 without any
problems. I have also verified that the patch does not cause any
regressions in the testsuite.

Regards,
Siddhesh

ChangeLog:

	* malloc/arena.c (shrink_heap): Unconditionally release memory
	  using mmap.
diff --git a/malloc/arena.c b/malloc/arena.c
index 06bdd77..943ceb6 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -618,20 +618,14 @@ shrink_heap(heap_info *h, long diff)
   new_size = (long)h->size - diff;
   if(new_size < (long)sizeof(*h))
     return -1;
+
   /* Try to re-map the extra heap space freshly to save memory, and
      make it inaccessible. */
-  if (__builtin_expect (__libc_enable_secure, 0))
-    {
-      if((char *)MMAP((char *)h + new_size, diff, PROT_NONE,
-		      MAP_FIXED) == (char *) MAP_FAILED)
-	return -2;
-      h->mprotect_size = new_size;
-    }
-  else
-    madvise ((char *)h + new_size, diff, MADV_DONTNEED);
-  /*fprintf(stderr, "shrink %p %08lx\n", h, new_size);*/
+  if((char *)MMAP((char *)h + new_size, diff, PROT_NONE,
+		  MAP_FIXED) == (char *) MAP_FAILED)
+    return -2;
 
-  h->size = new_size;
+  h->size = h->mprotect_size = new_size;
   return 0;
 }
 

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]