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

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Sat, Aug 11, 2012 at 11:22 AM, Siddhesh Poyarekar
<siddhesh@redhat.com> wrote:
> Hi,
>
> Here you go Carlos:

That was an *excellent* re-cap and summary of the existing thread!

I really appreciate the effort you took to write this up.

> 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.

This looks like a very reasonable solution.

Machines don't have infinite memory and we all want to get more out of
our systems, and this patch is a step towards that goal.

> ChangeLog:
>
>         * malloc/arena.c (shrink_heap): Unconditionally release memory
>           using mmap.

Some nits:

1. Please add a verbose comment about *why* we do this unconditionally
anymore and *why* we don't use MADV_DONTNEED.

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;
+
[CAO] 2. Don't add spaces.
   /* 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,
[CAO] 3. Space after if, MMAP, and casts (even if the old code didn't).
+		  MAP_FIXED) == (char *) MAP_FAILED)
[CAO] 4. This whole construct would be more understandable as:
[CAO] if ((char *) MMAP ((char *)h + new_size, diff, PROT_NONE, MAP_FIXED)
[CAO]     == (char *) MAP_FAILED)
[CAO] i.e. break after operator.
+    return -2;

-  h->size = new_size;
+  h->size = h->mprotect_size = new_size;
   return 0;
 }

OK with those changes.

Check it in on Wednesday to give other people some more review time.

Cheers,
Carlos.