Re: [PATCH] Bug 22111: Fix malloc tcache leak.

[Florian Weimer <fweimer at redhat dot com>]

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.

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

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

> +#define TNUM 10

TNUM should be parallel_threads or something like that, but I think it 
is unneeded.

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

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

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

Extraneous braces.

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

Thanks,
Florian