Message ID | 20171109105142.94C25439942E9@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | malloc: Add missing arena lock in mallinfo [BZ #22408] | expand |
On 11/09/2017 11:51 AM, Florian Weimer wrote: > Also count all heaps in an arena, not just the top-most one. > > 2017-11-09 Florian Weimer <fweimer@redhat.com> > > [BZ #22408] > * malloc/malloc.c (__malloc_info): Calculate arena heap statistics > under the per-arena lock. Count all heaps, not just the top one. > * malloc/Makefile (tests): Add tst-malloc_info. > (tst-malloc_info): Link with libpthread. > * malloc/tst-malloc_info.c: New file. Ping? https://patchwork.sourceware.org/patch/24179/ (I fixed the subject line.) Thanks, Florian
On Thursday 09 November 2017 04:21 PM, Florian Weimer wrote: > Also count all heaps in an arena, not just the top-most one. > Please add a more verbose description of the problem and the patch in the git commit message. I would put the subject line as "Fix arena heap statistics reporting in malloc_info" since it does more than adding the missing arena lock. That or split out the loop to iterate over heaps into a separate patch since it is a logically distinct change from the fix to BZ#22408, with a bz to track the fix. I prefer the latter approach. The change itself looks OK. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > 2017-11-09 Florian Weimer <fweimer@redhat.com> > > [BZ #22408] > * malloc/malloc.c (__malloc_info): Calculate arena heap statistics > under the per-arena lock. Count all heaps, not just the top one. > * malloc/Makefile (tests): Add tst-malloc_info. > (tst-malloc_info): Link with libpthread. > * malloc/tst-malloc_info.c: New file. > > diff --git a/malloc/Makefile b/malloc/Makefile > index 7ae3d825b9..17936fc04d 100644 > --- a/malloc/Makefile > +++ b/malloc/Makefile > @@ -35,6 +35,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \ > tst-interpose-thread \ > tst-alloc_buffer \ > tst-malloc-tcache-leak \ > + tst-malloc_info \ > > tests-static := \ > tst-interpose-static-nothread \ > @@ -246,3 +247,4 @@ $(objpfx)tst-dynarray-fail-mem.out: $(objpfx)tst-dynarray-fail.out > $(evaluate-test) > > $(objpfx)tst-malloc-tcache-leak: $(shared-thread-library) > +$(objpfx)tst-malloc_info: $(shared-thread-library) > diff --git a/malloc/malloc.c b/malloc/malloc.c > index f94d51cca1..1f003d2ef0 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -5455,6 +5455,21 @@ __malloc_info (int options, FILE *fp) > avail += sizes[NFASTBINS - 1 + i].total; > } > > + size_t heap_size = 0; > + size_t heap_mprotect_size = 0; > + if (ar_ptr != &main_arena) > + { > + /* Iterate over the arena heaps from back to front. */ > + heap_info *heap = heap_for_ptr (top (ar_ptr)); > + do > + { > + heap_size += heap->size; > + heap_mprotect_size += heap->mprotect_size; > + heap = heap->prev; > + } > + while (heap != NULL); > + } > + > __libc_lock_unlock (ar_ptr->mutex); > > total_nfastblocks += nfastblocks; > @@ -5488,13 +5503,12 @@ __malloc_info (int options, FILE *fp) > > if (ar_ptr != &main_arena) > { > - heap_info *heap = heap_for_ptr (top (ar_ptr)); > fprintf (fp, > "<aspace type=\"total\" size=\"%zu\"/>\n" > "<aspace type=\"mprotect\" size=\"%zu\"/>\n", > - heap->size, heap->mprotect_size); > - total_aspace += heap->size; > - total_aspace_mprotect += heap->mprotect_size; > + heap_size, heap_mprotect_size); > + total_aspace += heap_size; > + total_aspace_mprotect += heap_mprotect_size; > } > else > { > diff --git a/malloc/tst-malloc_info.c b/malloc/tst-malloc_info.c > new file mode 100644 > index 0000000000..44d460b29c > --- /dev/null > +++ b/malloc/tst-malloc_info.c > @@ -0,0 +1,101 @@ > +/* Smoke test for malloc_info > + Copyright (C) 2017 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +/* The purpose of this test is to provide a quick way to run > + malloc_info in a multi-threaded process. */ > + > +#include <array_length.h> > +#include <malloc.h> > +#include <stdlib.h> > +#include <support/support.h> > +#include <support/xthread.h> > + > +/* This barrier is used to have the main thread wait until the helper > + threads have performed their allocations. */ > +static pthread_barrier_t barrier; > + > +enum > + { > + /* Number of threads performing allocations. */ > + thread_count = 4, > + > + /* Amount of memory allocation per thread. This should be large > + enough to cause the allocation of multiple heaps per arena. */ > + per_thread_allocations > + = sizeof (void *) == 4 ? 16 * 1024 * 1024 : 128 * 1024 * 1024, > + }; > + > +static void * > +allocation_thread_function (void *closure) > +{ > + struct list > + { > + struct list *next; > + long dummy[4]; > + }; > + > + struct list *head = NULL; > + size_t allocated = 0; > + while (allocated < per_thread_allocations) > + { > + struct list *new_head = xmalloc (sizeof (*new_head)); > + allocated += sizeof (*new_head); > + new_head->next = head; > + head = new_head; > + } > + > + xpthread_barrier_wait (&barrier); > + > + /* Main thread prints first statistics here. */ > + > + xpthread_barrier_wait (&barrier); > + > + while (head != NULL) > + { > + struct list *next_head = head->next; > + free (head); > + head = next_head; > + } > + > + return NULL; > +} > + > +static int > +do_test (void) > +{ > + xpthread_barrier_init (&barrier, NULL, thread_count + 1); > + > + pthread_t threads[thread_count]; > + for (size_t i = 0; i < array_length (threads); ++i) > + threads[i] = xpthread_create (NULL, allocation_thread_function, NULL); > + > + xpthread_barrier_wait (&barrier); > + puts ("info: After allocation:"); > + malloc_info (0, stdout); > + > + xpthread_barrier_wait (&barrier); > + for (size_t i = 0; i < array_length (threads); ++i) > + xpthread_join (threads[i]); > + > + puts ("\ninfo: After deallocation:"); > + malloc_info (0, stdout); > + > + return 0; > +} > + > +#include <support/test-driver.c> >
On 11/14/2017 03:39 PM, Siddhesh Poyarekar wrote: > Please add a more verbose description of the problem and the patch in > the git commit message. I would put the subject line as "Fix arena heap > statistics reporting in malloc_info" since it does more than adding the > missing arena lock. That or split out the loop to iterate over heaps > into a separate patch since it is a logically distinct change from the > fix to BZ#22408, with a bz to track the fix. I prefer the latter approach. Okay, I will send two separate patches. I'm worried it looks like as if I'm fudging the commit statistics. Thanks, Florian
diff --git a/malloc/Makefile b/malloc/Makefile index 7ae3d825b9..17936fc04d 100644 --- a/malloc/Makefile +++ b/malloc/Makefile @@ -35,6 +35,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \ tst-interpose-thread \ tst-alloc_buffer \ tst-malloc-tcache-leak \ + tst-malloc_info \ tests-static := \ tst-interpose-static-nothread \ @@ -246,3 +247,4 @@ $(objpfx)tst-dynarray-fail-mem.out: $(objpfx)tst-dynarray-fail.out $(evaluate-test) $(objpfx)tst-malloc-tcache-leak: $(shared-thread-library) +$(objpfx)tst-malloc_info: $(shared-thread-library) diff --git a/malloc/malloc.c b/malloc/malloc.c index f94d51cca1..1f003d2ef0 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -5455,6 +5455,21 @@ __malloc_info (int options, FILE *fp) avail += sizes[NFASTBINS - 1 + i].total; } + size_t heap_size = 0; + size_t heap_mprotect_size = 0; + if (ar_ptr != &main_arena) + { + /* Iterate over the arena heaps from back to front. */ + heap_info *heap = heap_for_ptr (top (ar_ptr)); + do + { + heap_size += heap->size; + heap_mprotect_size += heap->mprotect_size; + heap = heap->prev; + } + while (heap != NULL); + } + __libc_lock_unlock (ar_ptr->mutex); total_nfastblocks += nfastblocks; @@ -5488,13 +5503,12 @@ __malloc_info (int options, FILE *fp) if (ar_ptr != &main_arena) { - heap_info *heap = heap_for_ptr (top (ar_ptr)); fprintf (fp, "<aspace type=\"total\" size=\"%zu\"/>\n" "<aspace type=\"mprotect\" size=\"%zu\"/>\n", - heap->size, heap->mprotect_size); - total_aspace += heap->size; - total_aspace_mprotect += heap->mprotect_size; + heap_size, heap_mprotect_size); + total_aspace += heap_size; + total_aspace_mprotect += heap_mprotect_size; } else { diff --git a/malloc/tst-malloc_info.c b/malloc/tst-malloc_info.c new file mode 100644 index 0000000000..44d460b29c --- /dev/null +++ b/malloc/tst-malloc_info.c @@ -0,0 +1,101 @@ +/* Smoke test for malloc_info + Copyright (C) 2017 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +/* The purpose of this test is to provide a quick way to run + malloc_info in a multi-threaded process. */ + +#include <array_length.h> +#include <malloc.h> +#include <stdlib.h> +#include <support/support.h> +#include <support/xthread.h> + +/* This barrier is used to have the main thread wait until the helper + threads have performed their allocations. */ +static pthread_barrier_t barrier; + +enum + { + /* Number of threads performing allocations. */ + thread_count = 4, + + /* Amount of memory allocation per thread. This should be large + enough to cause the allocation of multiple heaps per arena. */ + per_thread_allocations + = sizeof (void *) == 4 ? 16 * 1024 * 1024 : 128 * 1024 * 1024, + }; + +static void * +allocation_thread_function (void *closure) +{ + struct list + { + struct list *next; + long dummy[4]; + }; + + struct list *head = NULL; + size_t allocated = 0; + while (allocated < per_thread_allocations) + { + struct list *new_head = xmalloc (sizeof (*new_head)); + allocated += sizeof (*new_head); + new_head->next = head; + head = new_head; + } + + xpthread_barrier_wait (&barrier); + + /* Main thread prints first statistics here. */ + + xpthread_barrier_wait (&barrier); + + while (head != NULL) + { + struct list *next_head = head->next; + free (head); + head = next_head; + } + + return NULL; +} + +static int +do_test (void) +{ + xpthread_barrier_init (&barrier, NULL, thread_count + 1); + + pthread_t threads[thread_count]; + for (size_t i = 0; i < array_length (threads); ++i) + threads[i] = xpthread_create (NULL, allocation_thread_function, NULL); + + xpthread_barrier_wait (&barrier); + puts ("info: After allocation:"); + malloc_info (0, stdout); + + xpthread_barrier_wait (&barrier); + for (size_t i = 0; i < array_length (threads); ++i) + xpthread_join (threads[i]); + + puts ("\ninfo: After deallocation:"); + malloc_info (0, stdout); + + return 0; +} + +#include <support/test-driver.c>