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

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

[Florian Weimer]

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.

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>

Re: [PATCH] malloc: Add missing arena lock in malloc_info [BZ #22408]

[Florian Weimer]

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

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

[Siddhesh Poyarekar]

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>
> 

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

[Florian Weimer]

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

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

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
> 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.

I don't see the benefit of splitting up the patch, as there would be no
patch left - Florian's solution did not require any changes to the
locks.  Given no obvious change specific to the BZ, I think it's OK to
use the existing patch to close the existing BZ as "fixed by other
means".

So +1 for the patch as-is, from me.

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

[Siddhesh Poyarekar]

On Wednesday 15 November 2017 07:17 AM, DJ Delorie wrote:
> Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
>> 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.
> 
> I don't see the benefit of splitting up the patch, as there would be no
> patch left - Florian's solution did not require any changes to the
> locks.  Given no obvious change specific to the BZ, I think it's OK to
> use the existing patch to close the existing BZ as "fixed by other
> means".

I don't see how you came to that conclusion, there is a fix in there
that brings heap->size into the lock scope of ar_ptr->mutex.  That is
one fix and the other fix is to then expand the heap traversal so that
all heaps are accounted for.  Florian's follow-up patches have correctly
made that split.

They're two very different bugs and hence should have two different
patches to clearly indicate what changed.

Siddhesh

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

[DJ Delorie]

Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
> I don't see how you came to that conclusion, there is a fix in there
> that brings heap->size into the lock scope of ar_ptr->mutex.  That is
> one fix and the other fix is to then expand the heap traversal so that
> all heaps are accounted for.  Florian's follow-up patches have correctly
> made that split.

Florian's new patches move a line of code in one, and immediately remove
it in the second, just to satisfy a BZ.  I doubt the readers of the BZ
care about anything beyond "it was fixed" 99% of the time.

> They're two very different bugs and hence should have two different
> patches to clearly indicate what changed.

I don't feel too strongly about this because the code is the same in the
end, but it seems like extra work with a weak justification.  These are
the types of requirements that may put off a first-time contributor.
Obviously this doesn't apply to Florian ;-)

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

[Siddhesh Poyarekar]

On Wednesday 15 November 2017 09:23 AM, DJ Delorie wrote:
> I don't feel too strongly about this because the code is the same in the
> end, but it seems like extra work with a weak justification.  These are
> the types of requirements that may put off a first-time contributor.
> Obviously this doesn't apply to Florian ;-)

I probably feel stronger about it because of having to sift through
commits that mixed in multiple bug fixes in a past life.  For example,
RHEL-5 may want the arena lock fix in malloc_info but may not want the
change in malloc_info output format, in which case downstream will end
up having to figure out which parts should get excluded.  In this
particular case it is simple enough to do the separation (which is why I
was OK with the idea of calling the commit as "fixing multiple problems"
but not completely happy), but it can get very complicated very fast.

Siddhesh

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

[Florian Weimer]

On 11/15/2017 04:53 AM, DJ Delorie wrote:
> 
> Siddhesh Poyarekar <siddhesh@gotplt.org> writes:
>> I don't see how you came to that conclusion, there is a fix in there
>> that brings heap->size into the lock scope of ar_ptr->mutex.  That is
>> one fix and the other fix is to then expand the heap traversal so that
>> all heaps are accounted for.  Florian's follow-up patches have correctly
>> made that split.
> 
> Florian's new patches move a line of code in one, and immediately remove
> it in the second, just to satisfy a BZ.  I doubt the readers of the BZ
> care about anything beyond "it was fixed" 99% of the time.

Don't forget I also added a new statistics to make the split a bit more 
palatable.

>> They're two very different bugs and hence should have two different
>> patches to clearly indicate what changed.
> 
> I don't feel too strongly about this because the code is the same in the
> end, but it seems like extra work with a weak justification.  These are
> the types of requirements that may put off a first-time contributor.

But you weren't doing that to avoid a decision on patch acceptance.  I 
think this is the key difference that matters.

I'm more concerned with the documentation request for protection keys, 
for example.  The manual did not document mprotect before, so I have to 
fix that first.  But mprotect is complicated, and now we are looking at 
documenting the personality function.  Who knows where this will end? 
And all this goes into the manual, which very few people read because 
the manual pages are more immediately accessible (for both technical and 
non-technical reasons).

And all this when it's not even clear if we would accept the protection 
key patch in the end.

Thanks,
Florian