Re: [PATCH v3 14/37] nptl: Move __nptl_deallocate_tsd into libc

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

On 16/03/2021 14:29, Florian Weimer via Libc-alpha wrote:
> This prepares moving pthread_exit, and later the pthread_key_create
> infrastructure.

LGTM.

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

> ---
>  nptl/Makefile                       |   1 +
>  nptl/Versions                       |   1 +
>  nptl/nptl-init.c                    |   1 -
>  nptl/nptl_deallocate_tsd.c          | 112 ++++++++++++++++++++++++++++
>  nptl/pthreadP.h                     |   3 +-
>  nptl/pthread_create.c               |  94 -----------------------
>  sysdeps/nptl/libc_start_call_main.h |   5 --
>  sysdeps/nptl/pthread-functions.h    |   1 -
>  8 files changed, 116 insertions(+), 102 deletions(-)
>  create mode 100644 nptl/nptl_deallocate_tsd.c
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index c0ae3593ef..e21db8b138 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -45,6 +45,7 @@ routines = \
>    libc_multiple_threads \
>    libc_pthread_init \
>    lowlevellock \
> +  nptl_deallocate_tsd \
>    nptl_nthreads \
>    old_pthread_cond_destroy \
>    old_pthread_cond_init \

Ok.

> diff --git a/nptl/Versions b/nptl/Versions
> index 13198d1e89..72909b961c 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -107,6 +107,7 @@ libc {
>      __lll_lock_wait_private;
>      __lll_trylock_elision;
>      __lll_unlock_elision;
> +    __nptl_deallocate_tsd;
>      __nptl_nthreads;
>      __pthread_attr_copy;
>      __pthread_attr_destroy;

Ok.

> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index a09d6ed306..54d9d206b2 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -94,7 +94,6 @@ static const struct pthread_functions pthread_functions =
>      .ptr___pthread_key_create = __pthread_key_create,
>      .ptr___pthread_getspecific = __pthread_getspecific,
>      .ptr___pthread_setspecific = __pthread_setspecific,
> -    .ptr__nptl_deallocate_tsd = __nptl_deallocate_tsd,
>      .ptr__nptl_setxid = __nptl_setxid,
>      .ptr_set_robust = __nptl_set_robust
>    };

Ok.

> diff --git a/nptl/nptl_deallocate_tsd.c b/nptl/nptl_deallocate_tsd.c
> new file mode 100644
> index 0000000000..c665f4a08a
> --- /dev/null
> +++ b/nptl/nptl_deallocate_tsd.c
> @@ -0,0 +1,112 @@
> +/* Deallocation thread-specific data structures related to pthread_key_create.

I think this line is too long.

> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
> +
> +   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 <pthreadP.h>
> +
> +/* Deallocate POSIX thread-local-storage.  */
> +void
> +__nptl_deallocate_tsd (void)
> +{
> +  struct pthread *self = THREAD_SELF;
> +
> +  /* Maybe no data was ever allocated.  This happens often so we have
> +     a flag for this.  */
> +  if (THREAD_GETMEM (self, specific_used))
> +    {
> +      size_t round;
> +      size_t cnt;
> +
> +      round = 0;
> +      do
> +        {
> +          size_t idx;
> +
> +          /* So far no new nonzero data entry.  */
> +          THREAD_SETMEM (self, specific_used, false);
> +
> +          for (cnt = idx = 0; cnt < PTHREAD_KEY_1STLEVEL_SIZE; ++cnt)
> +            {
> +              struct pthread_key_data *level2;
> +
> +              level2 = THREAD_GETMEM_NC (self, specific, cnt);
> +
> +              if (level2 != NULL)
> +                {
> +                  size_t inner;
> +
> +                  for (inner = 0; inner < PTHREAD_KEY_2NDLEVEL_SIZE;
> +                       ++inner, ++idx)
> +                    {
> +                      void *data = level2[inner].data;
> +
> +                      if (data != NULL)
> +                        {
> +                          /* Always clear the data.  */
> +                          level2[inner].data = NULL;
> +
> +                          /* Make sure the data corresponds to a valid
> +                             key.  This test fails if the key was
> +                             deallocated and also if it was
> +                             re-allocated.  It is the user's
> +                             responsibility to free the memory in this
> +                             case.  */
> +                          if (level2[inner].seq
> +                              == __pthread_keys[idx].seq
> +                              /* It is not necessary to register a destructor
> +                                 function.  */
> +                              && __pthread_keys[idx].destr != NULL)
> +                            /* Call the user-provided destructor.  */
> +                            __pthread_keys[idx].destr (data);
> +                        }
> +                    }
> +                }
> +              else
> +                idx += PTHREAD_KEY_1STLEVEL_SIZE;
> +            }
> +
> +          if (THREAD_GETMEM (self, specific_used) == 0)
> +            /* No data has been modified.  */
> +            goto just_free;
> +        }
> +      /* We only repeat the process a fixed number of times.  */
> +      while (__builtin_expect (++round < PTHREAD_DESTRUCTOR_ITERATIONS, 0));
> +
> +      /* Just clear the memory of the first block for reuse.  */
> +      memset (&THREAD_SELF->specific_1stblock, '\0',
> +              sizeof (self->specific_1stblock));
> +
> +    just_free:
> +      /* Free the memory for the other blocks.  */
> +      for (cnt = 1; cnt < PTHREAD_KEY_1STLEVEL_SIZE; ++cnt)
> +        {
> +          struct pthread_key_data *level2;
> +
> +          level2 = THREAD_GETMEM_NC (self, specific, cnt);
> +          if (level2 != NULL)
> +            {
> +              /* The first block is allocated as part of the thread
> +                 descriptor.  */
> +              free (level2);
> +              THREAD_SETMEM_NC (self, specific, cnt, NULL);
> +            }
> +        }
> +
> +      THREAD_SETMEM (self, specific_used, false);
> +    }
> +}
> +libc_hidden_def (__nptl_deallocate_tsd)

Ok.

> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index d28610150c..4f814287ae 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -680,7 +680,8 @@ extern void _pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
>  extern void _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer,
>  					  int execute);
>  
> -extern void __nptl_deallocate_tsd (void) attribute_hidden;
> +extern void __nptl_deallocate_tsd (void);
> +libc_hidden_proto (__nptl_deallocate_tsd)
>  
>  extern void __nptl_setxid_error (struct xid_command *cmdp, int error)
>    attribute_hidden;

Ok.

> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 350bf03f5d..58e10e7741 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -242,100 +242,6 @@ __find_in_stack_list (struct pthread *pd)
>  }
>  
>  
> -/* Deallocate POSIX thread-local-storage.  */
> -void
> -attribute_hidden
> -__nptl_deallocate_tsd (void)
> -{
> -  struct pthread *self = THREAD_SELF;
> -
> -  /* Maybe no data was ever allocated.  This happens often so we have
> -     a flag for this.  */
> -  if (THREAD_GETMEM (self, specific_used))
> -    {
> -      size_t round;
> -      size_t cnt;
> -
> -      round = 0;
> -      do
> -	{
> -	  size_t idx;
> -
> -	  /* So far no new nonzero data entry.  */
> -	  THREAD_SETMEM (self, specific_used, false);
> -
> -	  for (cnt = idx = 0; cnt < PTHREAD_KEY_1STLEVEL_SIZE; ++cnt)
> -	    {
> -	      struct pthread_key_data *level2;
> -
> -	      level2 = THREAD_GETMEM_NC (self, specific, cnt);
> -
> -	      if (level2 != NULL)
> -		{
> -		  size_t inner;
> -
> -		  for (inner = 0; inner < PTHREAD_KEY_2NDLEVEL_SIZE;
> -		       ++inner, ++idx)
> -		    {
> -		      void *data = level2[inner].data;
> -
> -		      if (data != NULL)
> -			{
> -			  /* Always clear the data.  */
> -			  level2[inner].data = NULL;
> -
> -			  /* Make sure the data corresponds to a valid
> -			     key.  This test fails if the key was
> -			     deallocated and also if it was
> -			     re-allocated.  It is the user's
> -			     responsibility to free the memory in this
> -			     case.  */
> -			  if (level2[inner].seq
> -			      == __pthread_keys[idx].seq
> -			      /* It is not necessary to register a destructor
> -				 function.  */
> -			      && __pthread_keys[idx].destr != NULL)
> -			    /* Call the user-provided destructor.  */
> -			    __pthread_keys[idx].destr (data);
> -			}
> -		    }
> -		}
> -	      else
> -		idx += PTHREAD_KEY_1STLEVEL_SIZE;
> -	    }
> -
> -	  if (THREAD_GETMEM (self, specific_used) == 0)
> -	    /* No data has been modified.  */
> -	    goto just_free;
> -	}
> -      /* We only repeat the process a fixed number of times.  */
> -      while (__builtin_expect (++round < PTHREAD_DESTRUCTOR_ITERATIONS, 0));
> -
> -      /* Just clear the memory of the first block for reuse.  */
> -      memset (&THREAD_SELF->specific_1stblock, '\0',
> -	      sizeof (self->specific_1stblock));
> -
> -    just_free:
> -      /* Free the memory for the other blocks.  */
> -      for (cnt = 1; cnt < PTHREAD_KEY_1STLEVEL_SIZE; ++cnt)
> -	{
> -	  struct pthread_key_data *level2;
> -
> -	  level2 = THREAD_GETMEM_NC (self, specific, cnt);
> -	  if (level2 != NULL)
> -	    {
> -	      /* The first block is allocated as part of the thread
> -		 descriptor.  */
> -	      free (level2);
> -	      THREAD_SETMEM_NC (self, specific, cnt, NULL);
> -	    }
> -	}
> -
> -      THREAD_SETMEM (self, specific_used, false);
> -    }
> -}
> -
> -
>  /* Deallocate a thread's stack after optionally making sure the thread
>     descriptor is still valid.  */
>  void

Ok.

> diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h
> index 112cc20a7a..c579c65f78 100644
> --- a/sysdeps/nptl/libc_start_call_main.h
> +++ b/sysdeps/nptl/libc_start_call_main.h
> @@ -60,12 +60,7 @@ __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    else
>      {
>        /* Remove the thread-local data.  */
> -# ifdef SHARED
> -      PTHFCT_CALL (ptr__nptl_deallocate_tsd, ());
> -# else
> -      extern void __nptl_deallocate_tsd (void) __attribute ((weak));
>        __nptl_deallocate_tsd ();
> -# endif
>  
>        /* One less thread.  Decrement the counter.  If it is zero we
>           terminate the entire process.  */

Ok.

> diff --git a/sysdeps/nptl/pthread-functions.h b/sysdeps/nptl/pthread-functions.h
> index 2fa698b1da..b28b47ab0d 100644
> --- a/sysdeps/nptl/pthread-functions.h
> +++ b/sysdeps/nptl/pthread-functions.h
> @@ -55,7 +55,6 @@ struct pthread_functions
>    int (*ptr___pthread_key_create) (pthread_key_t *, void (*) (void *));
>    void *(*ptr___pthread_getspecific) (pthread_key_t);
>    int (*ptr___pthread_setspecific) (pthread_key_t, const void *);
> -  void (*ptr__nptl_deallocate_tsd) (void);
>    int (*ptr__nptl_setxid) (struct xid_command *);
>    void (*ptr_set_robust) (struct pthread *);
>  };
> 

Ok.