malloc: Add missing arena lock in mallinfo [BZ #22408]

Message ID 20171109105142.94C25439942E9@oldenburg.str.redhat.com
State New
Headers show
Series
  • malloc: Add missing arena lock in mallinfo [BZ #22408]
Related show

Commit Message

Florian Weimer Nov. 9, 2017, 10:51 a.m.
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.

Comments

Florian Weimer Nov. 14, 2017, 2:30 p.m. | #1
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
Siddhesh Poyarekar Nov. 14, 2017, 2:39 p.m. | #2
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>
>
Florian Weimer Nov. 14, 2017, 3:09 p.m. | #3
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

Patch

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>