Re: [PING] [v9] malloc: Consistently apply trim_threshold to all heaps [BZ #17195]

["Carlos O'Donell" <carlos at redhat dot com>]

On 04/02/2015 03:17 AM, Mel Gorman wrote:
> On Thu, Apr 02, 2015 at 02:47:13AM -0400, Siddhesh Poyarekar wrote:
>> ----- Original Message -----
>>>>>>> Ping as it's been a while since anyone said anything on it. I note
>>>>>>> it's
>>>>>>> in patchwork (http://patchwork.sourceware.org/patch/5496/) but do not
>>>>>>> know if that means anyone plans to look at it. Thanks.
>>
>> I've tested and pushed this patch now.  In future please also run the glibc
>> testsuite and mention in your submission what the results were.
>>
> 
> Thanks very much. FWIW, the test suite was run and there was no change in the
> number of passes or failures. I'll put that in the changelog in the future.

Siddhesh, Mel,

Sorry, I disappeared for vacation and just got back, but I did test the
patch against some real workloads, and wanted to propose a slight tweak
to what we have checked in. I would appreciate your opinions on the
change.

In _int_free, and systrim we have similar patterns to finding a chunk
and freeing it, but in _int_free the logic is slightly different and
my suggestion harmonizes with it:

malloc/malloc.c (_int_free):

4042       if (av == &main_arena) {
4043 #ifndef MORECORE_CANNOT_TRIM
4044         if ((unsigned long)(chunksize(av->top)) >=
4045             (unsigned long)(mp_.trim_threshold))
4046           systrim(mp_.top_pad, av);
4047 #endif

The trimming in _int_free is *only* done if the top chunk is at least
the size of the trim threshold.

This is slightly different to what we are doing right now in heap_trim
which is to check if the extra space is at least the trim size. It is more
precise to say "trim if the top chunk is at least the trim threshold"
and it still fixes the issue at hand.

What do you think?

diff --git a/malloc/arena.c b/malloc/arena.c
index d85f371..a93fc43 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -696,14 +696,20 @@ heap_trim (heap_info *heap, size_t pad)
     }
 
   /* Uses similar logic for per-thread arenas as the main arena with systrim
-     by preserving the top pad and at least a page.  */
+     and _int_free by preserving the top pad and rounding down to the nearest
+     page.  */
   top_size = chunksize (top_chunk);
+  if ((unsigned long)(top_size) <
+      (unsigned long)(mp_.trim_threshold))
+    return 0;
+
   top_area = top_size - MINSIZE - 1;
   if (top_area <= pad)
     return 0;
 
+  /* Release in pagesize units and round down to the nearest page.  */
   extra = ALIGN_DOWN(top_area - pad, pagesz);
-  if ((unsigned long) extra < mp_.trim_threshold)
+  if (extra == 0)
     return 0;
 
   /* Try to shrink. */
diff --git a/malloc/malloc.c b/malloc/malloc.c
index f361bad..e534d0e 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -241,7 +241,7 @@
 /* For MIN, MAX, powerof2.  */
 #include <sys/param.h>
 
-/* For ALIGN_UP.  */
+/* For ALIGN_UP et. al.  */
 #include <libc-internal.h>
 
 
@@ -2751,8 +2751,8 @@ systrim (size_t pad, mstate av)
   if (top_area <= pad)
     return 0;
 
-  /* Release in pagesize units, keeping at least one page */
-  extra = (top_area - pad) & ~(pagesize - 1);
+  /* Release in pagesize units and round down to the nearest page.  */
+  extra = ALIGN_DOWN(top_area - pad, pagesize);
 
   if (extra == 0)
     return 0;
---

Cheers,
Carlos.