| Message ID | 5542E696.2060402@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 30, 2015 at 10:36:06PM -0400, Carlos O'Donell wrote: > 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, Hope it was a good one! > 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. > I'd be interested in hearing about which workload motivated this for future reference. I'm not 100% happy with the ones I used (mostly artificial microbenches or proprietary workloads). The research papers for malloc either used poor workloads or the source was unavailable but I could have missed something. Also FWIW, my trim patch is still making its way into opensuse. It got held up by a bug in another package that was using uninitialsed memory. In the future if there is a problem that points the finger at this patch, it's worth testing with MALLOC_CHECK_ to see if that "fixes" it by zeroing memory. > 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? > I cannot think of any problems this might cause and it's better to be consistent.
On Fri, May 01, 2015 at 11:05:46AM +0100, Mel Gorman wrote: > Also FWIW, my trim patch is still making its way into opensuse. It got held > up by a bug in another package that was using uninitialsed memory. In the > future if there is a problem that points the finger at this patch, it's > worth testing with MALLOC_CHECK_ to see if that "fixes" it by zeroing memory. Is this other package java? We had openjdk crashing due to this patch in rawhide. Siddhesh
On Mon, May 04, 2015 at 08:39:58PM +0530, Siddhesh Poyarekar wrote: > On Fri, May 01, 2015 at 11:05:46AM +0100, Mel Gorman wrote: > > Also FWIW, my trim patch is still making its way into opensuse. It got held > > up by a bug in another package that was using uninitialsed memory. In the > > future if there is a problem that points the finger at this patch, it's > > worth testing with MALLOC_CHECK_ to see if that "fixes" it by zeroing memory. > > Is this other package java? We had openjdk crashing due to this patch > in rawhide. > It was llvm 3.5 regression tests.
On 05/04/2015 11:09 AM, Siddhesh Poyarekar wrote: > On Fri, May 01, 2015 at 11:05:46AM +0100, Mel Gorman wrote: >> Also FWIW, my trim patch is still making its way into opensuse. It got held >> up by a bug in another package that was using uninitialsed memory. In the >> future if there is a problem that points the finger at this patch, it's >> worth testing with MALLOC_CHECK_ to see if that "fixes" it by zeroing memory. > > Is this other package java? We had openjdk crashing due to this patch > in rawhide. What openjdk crash? :-) c.
On Mon, May 04, 2015 at 02:20:41PM -0400, Carlos O'Donell wrote: > On 05/04/2015 11:09 AM, Siddhesh Poyarekar wrote: > > On Fri, May 01, 2015 at 11:05:46AM +0100, Mel Gorman wrote: > >> Also FWIW, my trim patch is still making its way into opensuse. It got held > >> up by a bug in another package that was using uninitialsed memory. In the > >> future if there is a problem that points the finger at this patch, it's > >> worth testing with MALLOC_CHECK_ to see if that "fixes" it by zeroing memory. > > > > Is this other package java? We had openjdk crashing due to this patch > > in rawhide. > > What openjdk crash? :-) https://bugzilla.redhat.com/show_bug.cgi?id=1209451 This became very frequent due to this patch. I'm going to leave out this patch till they figure out what the problem is. Siddhesh
On 05/07/2015 01:01 AM, Siddhesh Poyarekar wrote: > On Mon, May 04, 2015 at 02:20:41PM -0400, Carlos O'Donell wrote: >> On 05/04/2015 11:09 AM, Siddhesh Poyarekar wrote: >>> On Fri, May 01, 2015 at 11:05:46AM +0100, Mel Gorman wrote: >>>> Also FWIW, my trim patch is still making its way into opensuse. It got held >>>> up by a bug in another package that was using uninitialsed memory. In the >>>> future if there is a problem that points the finger at this patch, it's >>>> worth testing with MALLOC_CHECK_ to see if that "fixes" it by zeroing memory. >>> >>> Is this other package java? We had openjdk crashing due to this patch >>> in rawhide. >> >> What openjdk crash? :-) > > https://bugzilla.redhat.com/show_bug.cgi?id=1209451 > > This became very frequent due to this patch. I'm going to leave out > this patch till they figure out what the problem is. Or put it back in to force them to figure it out :} c.
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;