malloc: Trim unused arenas on thread exit

  On 11/15/2017 12:19 PM, Siddhesh Poyarekar wrote:
> On Wednesday 15 November 2017 04:14 PM, Florian Weimer wrote:
>> It could be related to the ordering between tcache_thread_freeres and
>> arena_thread_freeres.  We should merge the two to enforce the correct
>> ordering.
> 
> I agree, it sounds like a better idea to have a single freeres for a
> module anyway.

Here's a patch which does just that.  Tested with and without 
--disable-experimental-malloc.

It doesn't have any consolidation/trimming changes.

Thanks,
Florian
  

On Wednesday 15 November 2017 07:06 PM, Florian Weimer wrote:
> Here's a patch which does just that.  Tested with and without
> --disable-experimental-malloc.
> 
> It doesn't have any consolidation/trimming changes.

LGTM, I trust you'll write a nice git commit log to explain the change :)

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

> 
> malloc-shutdown.patch
> 
> 
> malloc: Use a single thread shutdown hook for tcache and arenas
> 
> This modifies the code to make the ordering explicit (tcache first).
> 
> 2017-11-15  Florian Weimer  <fweimer@redhat.com>
> 
> 	* malloc/malloc.c (tcache_thread_shutdown): Rename from
> 	tcache_thread_freeres.  Define for USE_TCACHE and !USE_TCACHE
> 	alike.  Remove freeres marker.
> 	* malloc/arena.c (arena_thread_freeres): Call
> 	tcache_thread_shutdown.
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 85b985e193..4d27e17c46 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -944,6 +944,11 @@ arena_get_retry (mstate ar_ptr, size_t bytes)
>  static void __attribute__ ((section ("__libc_thread_freeres_fn")))
>  arena_thread_freeres (void)
>  {
> +  /* Shut down the thread cache first.  This could deallocate data for
> +     the thread arena, so do this before we put the arena on the free
> +     list.  */
> +  tcache_thread_shutdown ();
> +
>    mstate a = thread_arena;
>    thread_arena = NULL;
>  
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 2999ac4d2f..79f0e9eac7 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1869,6 +1869,9 @@ void *weak_variable (*__memalign_hook)
>    = memalign_hook_ini;
>  void weak_variable (*__after_morecore_hook) (void) = NULL;
>  
> +/* This function is called from the arena shutdown hook, to free the
> +   thread cache (if it exists).  */
> +static void tcache_thread_shutdown (void);
>  
>  /* ------------------ Testing support ----------------------------------*/
>  
> @@ -2938,8 +2941,8 @@ tcache_get (size_t tc_idx)
>    return (void *) e;
>  }
>  
> -static void __attribute__ ((section ("__libc_thread_freeres_fn")))
> -tcache_thread_freeres (void)
> +static void
> +tcache_thread_shutdown (void)
>  {
>    int i;
>    tcache_perthread_struct *tcache_tmp = tcache;
> @@ -2965,7 +2968,6 @@ tcache_thread_freeres (void)
>  
>    __libc_free (tcache_tmp);
>  }
> -text_set_element (__libc_thread_subfreeres, tcache_thread_freeres);
>  
>  static void
>  tcache_init(void)
> @@ -3002,13 +3004,20 @@ tcache_init(void)
>  
>  }
>  
> -#define MAYBE_INIT_TCACHE() \
> +# define MAYBE_INIT_TCACHE() \
>    if (__glibc_unlikely (tcache == NULL)) \
>      tcache_init();
>  
> -#else
> -#define MAYBE_INIT_TCACHE()
> -#endif
> +#else  /* !USE_TCACHE */
> +# define MAYBE_INIT_TCACHE()
> +
> +static void
> +tcache_thread_shutdown (void)
> +{
> +  /* Nothing to do if there is no thread cache.  */
> +}
> +
> +#endif /* !USE_TCACHE  */
>  
>  void *
>  __libc_malloc (size_t bytes)
>
  

On 11/15/2017 03:00 PM, Siddhesh Poyarekar wrote:
> On Wednesday 15 November 2017 07:06 PM, Florian Weimer wrote:
>> Here's a patch which does just that.  Tested with and without
>> --disable-experimental-malloc.
>>
>> It doesn't have any consolidation/trimming changes.
> 
> LGTM, I trust you'll write a nice git commit log to explain the change :)

I assumed the comment was sufficient:

>>   static void __attribute__ ((section ("__libc_thread_freeres_fn")))
>>   arena_thread_freeres (void)
>>   {
>> +  /* Shut down the thread cache first.  This could deallocate data for
>> +     the thread arena, so do this before we put the arena on the free
>> +     list.  */
>> +  tcache_thread_shutdown ();

If it is not, we need to put more documentation in the code itself.

Thanks,
Florian
  

On Wednesday 15 November 2017 07:40 PM, Florian Weimer wrote:
> 
> I assumed the comment was sufficient:
> 
>>>   static void __attribute__ ((section ("__libc_thread_freeres_fn")))
>>>   arena_thread_freeres (void)
>>>   {
>>> +  /* Shut down the thread cache first.  This could deallocate data for
>>> +     the thread arena, so do this before we put the arena on the free
>>> +     list.  */
>>> +  tcache_thread_shutdown ();
> 
> If it is not, we need to put more documentation in the code itself.

The comment describes the current behaviour while the commit log should
describe the change, something like:

"Call tcache destructor in arena_thread_freeres

Having separate cleanup functions for arena and tcache could result in
the tcache freeres function being called later and thus not deallocate
data for the thread arena.  Avoid this by calling the tcache cleanup
function from within arena_thread_freeres."

Siddhesh
  

On 11/15/2017 03:17 PM, Siddhesh Poyarekar wrote:
> On Wednesday 15 November 2017 07:40 PM, Florian Weimer wrote:
>>
>> I assumed the comment was sufficient:
>>
>>>>    static void __attribute__ ((section ("__libc_thread_freeres_fn")))
>>>>    arena_thread_freeres (void)
>>>>    {
>>>> +  /* Shut down the thread cache first.  This could deallocate data for
>>>> +     the thread arena, so do this before we put the arena on the free
>>>> +     list.  */
>>>> +  tcache_thread_shutdown ();
>>
>> If it is not, we need to put more documentation in the code itself.
> 
> The comment describes the current behaviour while the commit log should
> describe the change, something like:
> 
> "Call tcache destructor in arena_thread_freeres
> 
> Having separate cleanup functions for arena and tcache could result in
> the tcache freeres function being called later and thus not deallocate
> data for the thread arena.  Avoid this by calling the tcache cleanup
> function from within arena_thread_freeres."

But I have not verified that this actually happens, and we agreed that 
it was a good change nevertheless.

Thanks,
Florian
  

On Wednesday 15 November 2017 08:05 PM, Florian Weimer wrote:
>> "Call tcache destructor in arena_thread_freeres
>>
>> Having separate cleanup functions for arena and tcache could result in
>> the tcache freeres function being called later and thus not deallocate
>> data for the thread arena.  Avoid this by calling the tcache cleanup
>> function from within arena_thread_freeres."
> 
> But I have not verified that this actually happens, and we agreed that
> it was a good change nevertheless.

Then maybe this is a more accurate description:

"Call tcache destructor in arena_thread_freeres

It does not make sense to register separate cleanup functions for arena
and tcache since they're always going to be called together.  Call the
tcache cleanup function from within arena_thread_freeres since it at
least makes the order of those cleanups clear in the code."

My point is that we should be documenting the change clearly enough in
the commit log that one shouldn't have to read the code to get a high
level understanding of what the change is about.

Siddhesh