Re: [PATCH 6/8] nptl: Move __free_tcb into libc

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

On 10/05/2021 09:38, Florian Weimer via Libc-alpha wrote:
> Under the name __nptl_free_tcb.

LGTM (the 'Contribute-by' is not a blocker, so we can follow up on the
previous patch).

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

> ---
>  nptl/Makefile              |  1 +
>  nptl/Versions              |  1 +
>  nptl/nptl_free_tcb.c       | 46 ++++++++++++++++++++++++++++++++++++++
>  nptl/pthreadP.h            |  3 ++-
>  nptl/pthread_create.c      | 27 +---------------------
>  nptl/pthread_detach.c      |  2 +-
>  nptl/pthread_join_common.c |  2 +-
>  7 files changed, 53 insertions(+), 29 deletions(-)
>  create mode 100644 nptl/nptl_free_tcb.c
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 3b83817163..82dc6f466d 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -46,6 +46,7 @@ routines = \
>    lowlevellock \
>    nptl-stack \
>    nptl_deallocate_tsd \
> +  nptl_free_tcb \
>    nptl_nthreads \
>    nptl_setxid \
>    nptlfreeres \

Ok.

> diff --git a/nptl/Versions b/nptl/Versions
> index 93219d2657..da610a4803 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -321,6 +321,7 @@ libc {
>      __mutex_aconf;
>      __nptl_deallocate_stack;
>      __nptl_deallocate_tsd;
> +    __nptl_free_tcb;
>      __nptl_nthreads;
>      __nptl_setxid_sighandler;
>      __nptl_stack_list_add;

Ok.

> diff --git a/nptl/nptl_free_tcb.c b/nptl/nptl_free_tcb.c
> new file mode 100644
> index 0000000000..65c4d3bfa7
> --- /dev/null
> +++ b/nptl/nptl_free_tcb.c
> @@ -0,0 +1,46 @@
> +/* TCB deallocation 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.
> +
> +   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 <pthreadP.h>
> +#include <stdlib.h>
> +
> +void
> +__nptl_free_tcb (struct pthread *pd)
> +{
> +  /* The thread is exiting now.  */
> +  if (__builtin_expect (atomic_bit_test_set (&pd->cancelhandling,
> +                                             TERMINATED_BIT) == 0, 1))

Maybe use __glibc_likely here or just remove it, since it does not make
to use in this short function.

> +    {
> +      /* Free TPP data.  */
> +      if (__glibc_unlikely (pd->tpp != NULL))

Same here, the loop is short enough that it should not matter.

> +        {
> +          struct priority_protection_data *tpp = pd->tpp;
> +
> +          pd->tpp = NULL;
> +          free (tpp);
> +        }
> +
> +      /* 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.  */
> +      __nptl_deallocate_stack (pd);
> +    }
> +}
> +libc_hidden_def (__nptl_free_tcb)
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 505d0f00ff..d580f71a38 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -312,7 +312,8 @@ __do_cancel (void)
>  
>  /* Deallocate a thread's stack after optionally making sure the thread
>     descriptor is still valid.  */
> -extern void __free_tcb (struct pthread *pd) attribute_hidden;
> +extern void __nptl_free_tcb (struct pthread *pd);
> +libc_hidden_proto (__nptl_free_tcb)
>  
>  /* Change the permissions of a thread stack.  Called from
>     _dl_make_stacks_executable and pthread_create.  */

Ok.

> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index fcaf440bb5..770656453d 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -207,31 +207,6 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
>  
>  #include <createthread.c>
>  
> -/* Deallocate a thread's stack after optionally making sure the thread
> -   descriptor is still valid.  */
> -void
> -__free_tcb (struct pthread *pd)
> -{
> -  /* The thread is exiting now.  */
> -  if (__builtin_expect (atomic_bit_test_set (&pd->cancelhandling,
> -					     TERMINATED_BIT) == 0, 1))
> -    {
> -      /* Free TPP data.  */
> -      if (__glibc_unlikely (pd->tpp != NULL))
> -	{
> -	  struct priority_protection_data *tpp = pd->tpp;
> -
> -	  pd->tpp = NULL;
> -	  free (tpp);
> -	}
> -
> -      /* 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.  */
> -      __nptl_deallocate_stack (pd);
> -    }
> -}
> -
>  /* Local function to start thread and handle cleanup.
>     createthread.c defines the macro START_THREAD_DEFN to the
>     declaration that its create_thread function will refer to, and

Ok.

> @@ -444,7 +419,7 @@ START_THREAD_DEFN
>    /* If the thread is detached free the TCB.  */
>    if (IS_DETACHED (pd))
>      /* Free the TCB.  */
> -    __free_tcb (pd);
> +    __nptl_free_tcb (pd);
>  
>    /* We cannot call '_exit' here.  '_exit' will terminate the process.
>  

Ok.

> diff --git a/nptl/pthread_detach.c b/nptl/pthread_detach.c
> index 57293ec950..410255bbe1 100644
> --- a/nptl/pthread_detach.c
> +++ b/nptl/pthread_detach.c
> @@ -49,7 +49,7 @@ __pthread_detach (pthread_t th)
>      if ((pd->cancelhandling & EXITING_BITMASK) != 0)
>        /* Note that the code in __free_tcb makes sure each thread
>  	 control block is freed only once.  */
> -      __free_tcb (pd);
> +      __nptl_free_tcb (pd);
>  
>    return result;
>  }

Ok.

> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index a99c26e27e..e87801b5a3 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -122,7 +122,7 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
>  	*thread_return = pd_result;
>  
>        /* Free the TCB.  */
> -      __free_tcb (pd);
> +      __nptl_free_tcb (pd);
>      }
>    else
>      pd->joinid = NULL;
> 

Ok.