[PATCH v2] malloc: Fix tcache leak on thread destruction [BZ #22111]

[Carlos O'Donell]

On 09/27/2017 02:57 AM, Florian Weimer wrote:
> On 09/27/2017 07:44 AM, Carlos O'Donell wrote:
>> commit message---
> 
> Usually, we use something like this as the subject line:
> 
> malloc: Fix tcache leak on thread destruction [BZ #22111]
> 
> That is, subsystem first, description of the fix, and bug reference last.

Agreed.

Subsystem: <description> [BZ #NNNN]

I confirmed that it appears that commit<->bugzilla integration knows
how to parse [BZ #NNNN] on the first line of the commit.

I've edited the contribution checklist slightly.
https://sourceware.org/glibc/wiki/Contribution%20checklist

>> The malloc tcache added in 2.26 will leak all of the elements
>> remaining in the cache and the cache structure itself when a
>> thread exits. The defect is that we do not set tcache_shutting_down
>> early enough, and the thread simply recreates the tcache and
>> places the elements back onto a new tcache which is subsequently
>> lost as the thread exits (unfreed memory). The fix is relatively
>> simple, move the setting of tcache_shutting_down earlier in the
>> tcache_thread_freeres. We add a test case which uses mallinfo
>> and some heuristics to look for 1000x memory usage between the
>> start and end of a thread start/join loop. It is very reliable
>> at detecting that there is a leak given the number of iterations.
>> Without the fix the test will consume 122MiB of leaked memory.
> 
> See below, commit message may need updating.
> 
> The change itself looks good to me.
> 
>> diff --git a/malloc/tst-malloc-tcache-leak.c b/malloc/tst-malloc-tcache-leak.c
>> new file mode 100644
>> index 0000000..ad7ea46
>> --- /dev/null
>> +++ b/malloc/tst-malloc-tcache-leak.c
>> @@ -0,0 +1,111 @@
> 
> 
>> +static int
>> +do_test (void)
>> +{
>> +Â  /* Allocate an arbitrary number of threads that can run concurrently
>> +     and carry out one allocation from the tcahce and then exit.  We
> 
> typo: “tcahce”

Fixed.

>> +Â Â Â Â  could do it one thread at a time to minimize memory usage, but
>> +Â Â Â Â  it's faster to do 10 threads at a time, and we won't run out of
>> +     memory.  */
> 
> The comment is wrong for NUMA systems at least.  Running
> single-threaded is faster, particularly if you set the CPU affinity
> first.

I just switched to doing one thread at a time to simplify the test.

No need to optimize it, it only takes 4 seconds to create/join 100,000
threads on an x86_64 and a I expect similarly < 20s performance on other
systems.

>> +#define TNUM 10
> 
> TNUM should be parallel_threads or something like that, but I think it is unneeded.

Removed.

>> +Â  pthread_t *threads;
>> +Â  unsigned int i, loop;
>> +Â  struct mallinfo info_before, info_after;
>> +Â  void *retval;
>> +
>> +Â  /* Avoid there being 0 malloc'd data at this point by allocating the
>> +     pthread_t's required to run the test.  */
>> +Â  threads = (pthread_t *) xmalloc (sizeof (pthread_t) * TNUM);
> 
> Should use xcalloc.

Fixed.

>> +Â  info_before = mallinfo ();
>> +
>> +Â  assert (info_before.uordblks != 0);
>> +
>> +Â  printf ("INFO: %d (bytes) are in use before starting threads.\n",
>> +Â Â Â Â Â Â Â Â Â  info_before.uordblks);
>> +
>> +  /* Again this is an arbitrary choice.  We run the 10 threads 10,000
>> +     times for a total of 100,000 threads created and joined.  This
>> +     gives us enough iterations to show a leak.  */
>> +Â  for (loop = 0; loop < 10000; loop++)
> 
> If you keep the parallel execution (maybe it is faster on small systems, I have not tried), the outer loop count should involve TNUM.

Removed. It's a bare single loop now.

>> +Â Â Â  {
>> +
>> +Â Â Â Â Â  for (i = 0; i < TNUM; i++)
>> +Â Â Â  {
>> +Â Â Â Â Â  threads[i] = xpthread_create (NULL, worker, NULL);
>> +Â Â Â  }
> 
> Extraneous braces.

Removed.

>> +Â Â Â Â Â  for (i = 0; i < TNUM; i++)
>> +Â Â Â  {
>> +Â Â Â Â Â  retval = xpthread_join (threads[i]);
>> +Â Â Â Â Â  free (retval);
>> +Â Â Â  }
>> +Â Â Â  }
>> +
>> +Â  info_after = mallinfo ();
>> +Â  printf ("INFO: %d (bytes) are in use after all threads joined.\n",
>> +Â Â Â Â Â Â Â Â Â  info_after.uordblks);
>> +
>> +Â  /* We need to compare the memory in use before and the memory in use
>> +     after starting and joining 100,000 threads.  We almost always grow
>> +     memory slightly, but not much.  If the growth factor is over 1000
>> +     then we know we have a leak with high confidence.  Consider that if
>> +Â Â Â Â  even 1-byte leaked per thread we'd have 100,000 bytes of additional
>> +Â Â Â Â  memory, and in general the in-use at the start of main is quite
>> +     low.  */
>> +Â  if (info_after.uordblks > (info_before.uordblks * 1000))
>> +Â Â Â  FAIL_EXIT1 ("Memory usage after threads is too high.\n");
> 
> I think you should use a fixed offset here, not something that
> essentially scales with the initially allocated amount of memory.

Sure, that's a good idea.

I just used the threads count as the fixed offset, which is an assumption
of a small 1-byte leak per thread, and anything higher than that is a leak.

e.g.

Before:
INFO: 624 (bytes) are in use before starting threads.
INFO: 118403536 (bytes) are in use after all threads joined.
error: tst-malloc-tcache-leak.c:91: Memory usage after threads is too high.

Which is a ~1.1kb leak per thread.

After:
INFO: 624 (bytes) are in use before starting threads.
INFO: 3472 (bytes) are in use after all threads joined.

v2 attached.

Would you give a Reviewed-by when you think this looks good?

-- 
Cheers,
Carlos.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: swbz22111-v2.patch
Type: text/x-patch
Size: 7728 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20170928/962721b8/attachment.bin>