Re: [PATCH 5/8] nptl: Move stack cache management, __libpthread_freeres into libc

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 10/05/2021 09:37, Florian Weimer via Libc-alpha wrote:
> This replaces the FREE_P macro with the __nptl_stack_in_use inline
> function.  stack_list_del is renamed to __nptl_stack_list_del,
> stack_list_add to __nptl_stack_list_add, __deallocate_stack to
> __nptl_deallocate_stack, free_stacks to __nptl_free_stacks.
> 
> It is convenient to move __libpthread_freeres into libc at the
> same time.  This removes the temporary __default_pthread_attr_freeres
> export and restores full freeres coverage for __default_pthread_attr.

LGTM with some small nits below.  I still dislike the 'Contributed by' 
on *newer* file, so I ccing Carlos.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  malloc/set-freeres.c  |  15 +++--
>  nptl/Makefile         |   3 +-
>  nptl/Versions         |   5 +-
>  nptl/allocatestack.c  | 147 ++----------------------------------------
>  nptl/descr.h          |   6 ++
>  nptl/nptl-stack.c     | 130 +++++++++++++++++++++++++++++++++++++
>  nptl/nptl-stack.h     |  50 ++++++++++++++
>  nptl/nptlfreeres.c    |   3 +-
>  nptl/pthreadP.h       |   7 +-
>  nptl/pthread_create.c |   4 +-
>  10 files changed, 212 insertions(+), 158 deletions(-)
>  create mode 100644 nptl/nptl-stack.c
>  create mode 100644 nptl/nptl-stack.h
> 
> diff --git a/malloc/set-freeres.c b/malloc/set-freeres.c
> index d404250151..5c19a2725c 100644
> --- a/malloc/set-freeres.c
> +++ b/malloc/set-freeres.c
> @@ -29,7 +29,15 @@ DEFINE_HOOK (__libc_subfreeres, (void));
>  
>  symbol_set_define (__libc_freeres_ptrs);
>  
> -extern __attribute__ ((weak)) void __libpthread_freeres (void);
> +extern void __libpthread_freeres (void)
> +#if PTHREAD_IN_LIBC && defined SHARED
> +/* It is possible to call __libpthread_freeres directly in shared
> +   builds with an integrated libpthread.  */
> +  attribute_hidden
> +#else
> +  __attribute__ ((weak))
> +#endif
> +  ;
>  
>  void __libc_freeres_fn_section
>  __libc_freeres (void)
> @@ -51,10 +59,7 @@ __libc_freeres (void)
>        /* We run the resource freeing after IO cleanup.  */
>        RUN_HOOK (__libc_subfreeres, ());
>  
> -      /* Call the libpthread list of cleanup functions
> -	 (weak-ref-and-check).  */
> -      if (&__libpthread_freeres != NULL)
> -	__libpthread_freeres ();
> +      call_function_static_weak (__libpthread_freeres);
>  
>  #ifdef SHARED
>        __libc_unwind_link_freeres ();

Ok.

> diff --git a/nptl/Makefile b/nptl/Makefile
> index 2a18eadf71..3b83817163 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -44,9 +44,11 @@ routines = \
>    libc_multiple_threads \
>    libc_pthread_init \
>    lowlevellock \
> +  nptl-stack \
>    nptl_deallocate_tsd \
>    nptl_nthreads \
>    nptl_setxid \
> +  nptlfreeres \
>    old_pthread_atfork \
>    old_pthread_cond_broadcast \
>    old_pthread_cond_destroy \
> @@ -185,7 +187,6 @@ libpthread-routines = \
>    funlockfile \
>    libpthread-compat \
>    nptl-init \
> -  nptlfreeres \
>    pt-interp \
>    pthread_attr_getaffinity \
>    pthread_attr_getguardsize \

Ok.

> diff --git a/nptl/Versions b/nptl/Versions
> index de025e179c..93219d2657 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -303,7 +303,6 @@ libc {
>    }
>    GLIBC_PRIVATE {
>      __default_pthread_attr;
> -    __default_pthread_attr_freeres;
>      __default_pthread_attr_lock;
>      __futex_abstimed_wait64;
>      __futex_abstimed_wait_cancelable64;
> @@ -320,9 +319,12 @@ libc {
>      __lll_trylock_elision;
>      __lll_unlock_elision;
>      __mutex_aconf;
> +    __nptl_deallocate_stack;
>      __nptl_deallocate_tsd;
>      __nptl_nthreads;
>      __nptl_setxid_sighandler;
> +    __nptl_stack_list_add;
> +    __nptl_stack_list_del;
>      __pthread_attr_copy;
>      __pthread_attr_destroy;
>      __pthread_attr_init;
> @@ -459,7 +461,6 @@ libpthread {
>    }
>  
>    GLIBC_PRIVATE {
> -    __libpthread_freeres;
>      __pthread_clock_gettime;
>      __pthread_clock_settime;
>      __pthread_get_minstack;

OK.

> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 8672e89e75..c0a5c4d96d 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -31,7 +31,7 @@
>  #include <lowlevellock.h>
>  #include <futex-internal.h>
>  #include <kernel-features.h>
> -
> +#include <nptl-stack.h>
>  
>  #ifndef NEED_SEPARATE_REGISTER_STACK
>  
> @@ -92,56 +92,6 @@
>  # define MAP_STACK 0
>  #endif
>  
> -/* This yields the pointer that TLS support code calls the thread pointer.  */
> -#if TLS_TCB_AT_TP
> -# define TLS_TPADJ(pd) (pd)
> -#elif TLS_DTV_AT_TP
> -# define TLS_TPADJ(pd) ((struct pthread *)((char *) (pd) + TLS_PRE_TCB_SIZE))
> -#endif
> -
> -/* Cache handling for not-yet free stacks.  */
> -
> -/* Maximum size in kB of cache.  */
> -static size_t stack_cache_maxsize = 40 * 1024 * 1024; /* 40MiBi by default.  */
> -
> -/* Check whether the stack is still used or not.  */
> -#define FREE_P(descr) ((descr)->tid <= 0)
> -
> -
> -static void
> -stack_list_del (list_t *elem)
> -{
> -  GL (dl_in_flight_stack) = (uintptr_t) elem;
> -
> -  atomic_write_barrier ();
> -
> -  list_del (elem);
> -
> -  atomic_write_barrier ();
> -
> -  GL (dl_in_flight_stack) = 0;
> -}
> -
> -
> -static void
> -stack_list_add (list_t *elem, list_t *list)
> -{
> -  GL (dl_in_flight_stack) = (uintptr_t) elem | 1;
> -
> -  atomic_write_barrier ();
> -
> -  list_add (elem, list);
> -
> -  atomic_write_barrier ();
> -
> -  GL (dl_in_flight_stack) = 0;
> -}
> -
> -
> -/* We create a double linked list of all cache entries.  Double linked
> -   because this allows removing entries from the end.  */
> -
> -
>  /* Get a stack frame from the cache.  We have to match by size since
>     some blocks might be too small or far too large.  */
>  static struct pthread *

Ok.

> @@ -164,7 +114,7 @@ get_cached_stack (size_t *sizep, void **memp)
>        struct pthread *curr;
>  
>        curr = list_entry (entry, struct pthread, list);
> -      if (FREE_P (curr) && curr->stackblock_size >= size)
> +      if (__nptl_stack_in_use (curr) && curr->stackblock_size >= size)
>  	{
>  	  if (curr->stackblock_size == size)
>  	    {

Ok.

> @@ -193,10 +143,10 @@ get_cached_stack (size_t *sizep, void **memp)
>    result->setxid_futex = -1;
>  
>    /* Dequeue the entry.  */
> -  stack_list_del (&result->list);
> +  __nptl_stack_list_del (&result->list);
>  
>    /* And add to the list of stacks in use.  */
> -  stack_list_add (&result->list, &GL (dl_stack_used));
> +  __nptl_stack_list_add (&result->list, &GL (dl_stack_used));
>  
>    /* And decrease the cache size.  */
>    GL (dl_stack_cache_actsize) -= result->stackblock_size;

Ok.

> @@ -229,68 +179,6 @@ get_cached_stack (size_t *sizep, void **memp)
>    return result;
>  }
>  
> -
> -/* Free stacks until cache size is lower than LIMIT.  */
> -static void
> -free_stacks (size_t limit)
> -{
> -  /* We reduce the size of the cache.  Remove the last entries until
> -     the size is below the limit.  */
> -  list_t *entry;
> -  list_t *prev;
> -
> -  /* Search from the end of the list.  */
> -  list_for_each_prev_safe (entry, prev, &GL (dl_stack_cache))
> -    {
> -      struct pthread *curr;
> -
> -      curr = list_entry (entry, struct pthread, list);
> -      if (FREE_P (curr))
> -	{
> -	  /* Unlink the block.  */
> -	  stack_list_del (entry);
> -
> -	  /* Account for the freed memory.  */
> -	  GL (dl_stack_cache_actsize) -= curr->stackblock_size;
> -
> -	  /* Free the memory associated with the ELF TLS.  */
> -	  _dl_deallocate_tls (TLS_TPADJ (curr), false);
> -
> -	  /* Remove this block.  This should never fail.  If it does
> -	     something is really wrong.  */
> -	  if (__munmap (curr->stackblock, curr->stackblock_size) != 0)
> -	    abort ();
> -
> -	  /* Maybe we have freed enough.  */
> -	  if (GL (dl_stack_cache_actsize) <= limit)
> -	    break;
> -	}
> -    }
> -}
> -
> -/* Free all the stacks on cleanup.  */
> -void
> -__nptl_stacks_freeres (void)
> -{
> -  free_stacks (0);
> -}
> -
> -/* Add a stack frame which is not used anymore to the stack.  Must be
> -   called with the cache lock held.  */
> -static inline void
> -__attribute ((always_inline))
> -queue_stack (struct pthread *stack)
> -{
> -  /* We unconditionally add the stack to the list.  The memory may
> -     still be in use but it will not be reused until the kernel marks
> -     the stack as not used anymore.  */
> -  stack_list_add (&stack->list, &GL (dl_stack_cache));
> -
> -  GL (dl_stack_cache_actsize) += stack->stackblock_size;
> -  if (__glibc_unlikely (GL (dl_stack_cache_actsize) > stack_cache_maxsize))
> -    free_stacks (stack_cache_maxsize);
> -}
> -
>  /* Return the guard page position on allocated stack.  */
>  static inline char *
>  __attribute ((always_inline))

Ok.

> @@ -588,7 +476,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>  	  lll_lock (GL (dl_stack_cache_lock), LLL_PRIVATE);
>  
>  	  /* And add to the list of stacks in use.  */
> -	  stack_list_add (&pd->list, &GL (dl_stack_used));
> +	  __nptl_stack_list_add (&pd->list, &GL (dl_stack_used));
>  
>  	  lll_unlock (GL (dl_stack_cache_lock), LLL_PRIVATE);
>  
> @@ -630,7 +518,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>  	      lll_lock (GL (dl_stack_cache_lock), LLL_PRIVATE);
>  
>  	      /* Remove the thread from the list.  */
> -	      stack_list_del (&pd->list);
> +	      __nptl_stack_list_del (&pd->list);
>  
>  	      lll_unlock (GL (dl_stack_cache_lock), LLL_PRIVATE);
>  

Ok.

> @@ -731,26 +619,3 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>  
>    return 0;
>  }
> -
> -
> -void
> -__deallocate_stack (struct pthread *pd)
> -{
> -  lll_lock (GL (dl_stack_cache_lock), LLL_PRIVATE);
> -
> -  /* Remove the thread from the list of threads with user defined
> -     stacks.  */
> -  stack_list_del (&pd->list);
> -
> -  /* Not much to do.  Just free the mmap()ed memory.  Note that we do
> -     not reset the 'used' flag in the 'tid' field.  This is done by
> -     the kernel.  If no thread has been created yet this field is
> -     still zero.  */
> -  if (__glibc_likely (! pd->user_stack))
> -    (void) queue_stack (pd);
> -  else
> -    /* Free the memory associated with the ELF TLS.  */
> -    _dl_deallocate_tls (TLS_TPADJ (pd), false);
> -
> -  lll_unlock (GL (dl_stack_cache_lock), LLL_PRIVATE);
> -}

Ok.

> diff --git a/nptl/descr.h b/nptl/descr.h
> index d423a53bbf..3de9535449 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -416,5 +416,11 @@ struct pthread
>    (sizeof (struct pthread) - offsetof (struct pthread, end_padding))
>  } __attribute ((aligned (TCB_ALIGNMENT)));
>  
> +/* This yields the pointer that TLS support code calls the thread pointer.  */
> +#if TLS_TCB_AT_TP
> +# define TLS_TPADJ(pd) (pd)
> +#elif TLS_DTV_AT_TP
> +# define TLS_TPADJ(pd) ((struct pthread *)((char *) (pd) + TLS_PRE_TCB_SIZE))
> +#endif
>  
>  #endif	/* descr.h */

Ok.

> diff --git a/nptl/nptl-stack.c b/nptl/nptl-stack.c
> new file mode 100644
> index 0000000000..5ead9db198
> --- /dev/null
> +++ b/nptl/nptl-stack.c
> @@ -0,0 +1,130 @@
> +/* Stack cache management for NPTL.
> +   Copyright (C) 2002-2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.

I am aware about your reservation about removing the 'Contributed by' removal,
but I think for *newer* files it should *not* be included.

> +
> +   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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <nptl-stack.h>
> +#include <ldsodefs.h>
> +
> +/* Maximum size in kB of cache.  */
> +static size_t stack_cache_maxsize = 40 * 1024 * 1024; /* 40MiBi by default.  */

This line seems to be too long.

> +
> +void
> +__nptl_stack_list_del (list_t *elem)
> +{
> +  GL (dl_in_flight_stack) = (uintptr_t) elem;
> +
> +  atomic_write_barrier ();
> +
> +  list_del (elem);
> +
> +  atomic_write_barrier ();
> +
> +  GL (dl_in_flight_stack) = 0;
> +}
> +libc_hidden_def (__nptl_stack_list_del)
> +
> +void
> +__nptl_stack_list_add (list_t *elem, list_t *list)
> +{
> +  GL (dl_in_flight_stack) = (uintptr_t) elem | 1;
> +
> +  atomic_write_barrier ();
> +
> +  list_add (elem, list);
> +
> +  atomic_write_barrier ();
> +
> +  GL (dl_in_flight_stack) = 0;
> +}
> +libc_hidden_def (__nptl_stack_list_add)
> +
> +void
> +__nptl_free_stacks (size_t limit)
> +{
> +  /* We reduce the size of the cache.  Remove the last entries until
> +     the size is below the limit.  */
> +  list_t *entry;
> +  list_t *prev;
> +
> +  /* Search from the end of the list.  */
> +  list_for_each_prev_safe (entry, prev, &GL (dl_stack_cache))
> +    {
> +      struct pthread *curr;
> +
> +      curr = list_entry (entry, struct pthread, list);
> +      if (__nptl_stack_in_use (curr))
> +	{
> +	  /* Unlink the block.  */
> +	  __nptl_stack_list_del (entry);
> +
> +	  /* Account for the freed memory.  */
> +	  GL (dl_stack_cache_actsize) -= curr->stackblock_size;
> +
> +	  /* Free the memory associated with the ELF TLS.  */
> +	  _dl_deallocate_tls (TLS_TPADJ (curr), false);
> +
> +	  /* Remove this block.  This should never fail.  If it does
> +	     something is really wrong.  */
> +	  if (__munmap (curr->stackblock, curr->stackblock_size) != 0)
> +	    abort ();
> +
> +	  /* Maybe we have freed enough.  */
> +	  if (GL (dl_stack_cache_actsize) <= limit)
> +	    break;
> +	}
> +    }
> +}
> +
> +/* Add a stack frame which is not used anymore to the stack.  Must be
> +   called with the cache lock held.  */
> +static inline void
> +__attribute ((always_inline))
> +queue_stack (struct pthread *stack)
> +{
> +  /* We unconditionally add the stack to the list.  The memory may
> +     still be in use but it will not be reused until the kernel marks
> +     the stack as not used anymore.  */
> +  __nptl_stack_list_add (&stack->list, &GL (dl_stack_cache));
> +
> +  GL (dl_stack_cache_actsize) += stack->stackblock_size;
> +  if (__glibc_unlikely (GL (dl_stack_cache_actsize) > stack_cache_maxsize))
> +    __nptl_free_stacks (stack_cache_maxsize);
> +}
> +
> +void
> +__nptl_deallocate_stack (struct pthread *pd)
> +{
> +  lll_lock (GL (dl_stack_cache_lock), LLL_PRIVATE);
> +
> +  /* Remove the thread from the list of threads with user defined
> +     stacks.  */
> +  __nptl_stack_list_del (&pd->list);
> +
> +  /* Not much to do.  Just free the mmap()ed memory.  Note that we do
> +     not reset the 'used' flag in the 'tid' field.  This is done by
> +     the kernel.  If no thread has been created yet this field is
> +     still zero.  */
> +  if (__glibc_likely (! pd->user_stack))
> +    (void) queue_stack (pd);
> +  else
> +    /* Free the memory associated with the ELF TLS.  */
> +    _dl_deallocate_tls (TLS_TPADJ (pd), false);
> +
> +  lll_unlock (GL (dl_stack_cache_lock), LLL_PRIVATE);
> +}
> +libc_hidden_def (__nptl_deallocate_stack)

Ok.

> diff --git a/nptl/nptl-stack.h b/nptl/nptl-stack.h
> new file mode 100644
> index 0000000000..8631b61816
> --- /dev/null
> +++ b/nptl/nptl-stack.h
> @@ -0,0 +1,50 @@
> +/* Stack cache management for NPTL.
> +   Copyright (C) 2002-2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.

Same as before.

> +
> +   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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#ifndef _NPTL_STACK_H
> +#define _NPTL_STACK_H
> +
> +#include <descr.h>
> +#include <list.h>
> +#include <stdbool.h>
> +
> +/* Check whether the stack is still used or not.  */
> +static inline bool
> +__nptl_stack_in_use (struct pthread *pd)
> +{
> +  return pd->tid <= 0;
> +}
> +
> +/* Remove the stack ELEM from its list.  */
> +void __nptl_stack_list_del (list_t *elem);
> +libc_hidden_proto (__nptl_stack_list_del)
> +
> +/* Add ELEM to a stack list.  LIST can be either &GL (dl_stack_used)
> +   or &GL (dl_stack_cache).  */
> +void __nptl_stack_list_add (list_t *elem, list_t *list);
> +libc_hidden_proto (__nptl_stack_list_add)
> +
> +/* Free allocated stack.  */
> +extern void __nptl_deallocate_stack (struct pthread *pd);
> +libc_hidden_proto (__nptl_deallocate_stack)
> +
> +/* Free stacks until cache size is lower than LIMIT.  */
> +void __nptl_free_stacks (size_t limit) attribute_hidden;
> +
> +#endif /* _NPTL_STACK_H */

Ok.

> diff --git a/nptl/nptlfreeres.c b/nptl/nptlfreeres.c
> index 4833f04714..527b5ee103 100644
> --- a/nptl/nptlfreeres.c
> +++ b/nptl/nptlfreeres.c
> @@ -19,6 +19,7 @@
>  #include <set-hooks.h>
>  #include <libc-symbols.h>
>  #include <pthreadP.h>
> +#include <nptl-stack.h>
>  
>  /* Free libpthread.so resources.
>     Note: Caller ensures we are called only once.  */
> @@ -26,5 +27,5 @@ void
>  __libpthread_freeres (void)
>  {
>    call_function_static_weak (__default_pthread_attr_freeres);
> -  call_function_static_weak (__nptl_stacks_freeres);
> +  __nptl_free_stacks (0);
>  }

Ok.

> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 8466332248..505d0f00ff 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -203,7 +203,7 @@ libc_hidden_proto (__default_pthread_attr)
>  extern int __default_pthread_attr_lock;
>  libc_hidden_proto (__default_pthread_attr_lock)
>  /* Called from __libc_freeres to deallocate the default attribute.  */
> -extern void __default_pthread_attr_freeres (void);
> +extern void __default_pthread_attr_freeres (void) attribute_hidden;
>  
>  /* Size and alignment of static TLS block.  */
>  extern size_t __static_tls_size attribute_hidden;
> @@ -314,9 +314,6 @@ __do_cancel (void)
>     descriptor is still valid.  */
>  extern void __free_tcb (struct pthread *pd) attribute_hidden;
>  
> -/* Free allocated stack.  */
> -extern void __deallocate_stack (struct pthread *pd) attribute_hidden;
> -
>  /* Change the permissions of a thread stack.  Called from
>     _dl_make_stacks_executable and pthread_create.  */
>  int
> @@ -679,8 +676,6 @@ void __nptl_setxid_sighandler (int sig, siginfo_t *si, void *ctx);
>  libc_hidden_proto (__nptl_setxid_sighandler)
>  extern int __nptl_setxid (struct xid_command *cmdp) attribute_hidden;
>  
> -extern void __nptl_stacks_freeres (void) attribute_hidden;
> -
>  extern void __wait_lookup_done (void) attribute_hidden;
>  
>  /* Allocates the extension space for ATTR.  Returns an error code on

Ok.

> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index d19456d48b..fcaf440bb5 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -228,7 +228,7 @@ __free_tcb (struct pthread *pd)
>        /* Queue the stack memory block for reuse and exit the process.  The
>  	 kernel will signal via writing to the address returned by
>  	 QUEUE-STACK when the stack is available.  */
> -      __deallocate_stack (pd);
> +      __nptl_deallocate_stack (pd);
>      }
>  }
>  
> @@ -711,7 +711,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>  	    futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
>  
>  	  /* Free the resources.  */
> -	  __deallocate_stack (pd);
> +	  __nptl_deallocate_stack (pd);
>  	}
>  
>        /* We have to translate error codes.  */
> 

OK.