This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] malloc: Add missing arena lock in mallinfo [BZ #22408]
- From: Siddhesh Poyarekar <siddhesh at gotplt dot org>
- To: Florian Weimer <fweimer at redhat dot com>, libc-alpha at sourceware dot org
- Date: Tue, 14 Nov 2017 20:09:25 +0530
- Subject: Re: [PATCH] malloc: Add missing arena lock in mallinfo [BZ #22408]
- Authentication-results: sourceware.org; auth=none
- References: <20171109105142.94C25439942E9@oldenburg.str.redhat.com>
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>
>