Re: [PATCH 28/34] Linux: Move timer helper routines from librt to libc

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

On 17/06/2021 15:59, Florian Weimer via Libc-alpha wrote:
> This adds several temporary GLIBC_PRIVATE exports.  The symbol names
> are changed so that they all start with __timer_.
> 
> It is now possible to invoke the fork handler directly, so
> pthread_atfork is no longer necessary.  The associated error cannot
> happen anymore, and cancellation handling can be removed from
> the helper thread routine.

LGTM, thanks.

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


> ---
>  sysdeps/nptl/Makefile                         |  2 +-
>  sysdeps/nptl/fork.c                           |  2 +
>  sysdeps/unix/sysv/linux/Versions              |  5 ++
>  sysdeps/unix/sysv/linux/kernel-posix-timers.h | 22 ++++---
>  sysdeps/unix/sysv/linux/timer_create.c        | 14 ++---
>  sysdeps/unix/sysv/linux/timer_delete.c        | 10 ++--
>  sysdeps/unix/sysv/linux/timer_routines.c      | 59 +++++++++----------
>  7 files changed, 64 insertions(+), 50 deletions(-)
> 
> diff --git a/sysdeps/nptl/Makefile b/sysdeps/nptl/Makefile
> index 0707f130eb..27aa41af3c 100644
> --- a/sysdeps/nptl/Makefile
> +++ b/sysdeps/nptl/Makefile
> @@ -17,7 +17,7 @@
>  # <https://www.gnu.org/licenses/>.
>  
>  ifeq ($(subdir),rt)
> -librt-sysdep_routines += timer_routines
> +sysdep_routines += timer_routines
>  
>  tests += tst-mqueue8x
>  CFLAGS-tst-mqueue8x.c += -fexceptions

Ok.

> diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c
> index d6a0996b79..930716c403 100644
> --- a/sysdeps/nptl/fork.c
> +++ b/sysdeps/nptl/fork.c
> @@ -37,6 +37,7 @@
>  #include <sys/single_threaded.h>
>  #include <list.h>
>  #include <mqueue.h>
> +#include <kernel-posix-timers.h>
>  
>  static void
>  fresetlockfiles (void)
> @@ -231,6 +232,7 @@ __libc_fork (void)
>  	  _IO_list_resetlock ();
>  
>  	  call_function_static_weak (__mq_notify_fork_subprocess);
> +	  call_function_static_weak (__timer_fork_subprocess);
>  
>  	  call_function_static_weak (__nss_database_fork_subprocess,
>  				     &nss_database_data);

Ok.

> diff --git a/sysdeps/unix/sysv/linux/Versions b/sysdeps/unix/sysv/linux/Versions
> index 051ecf9390..47d4357b9f 100644
> --- a/sysdeps/unix/sysv/linux/Versions
> +++ b/sysdeps/unix/sysv/linux/Versions
> @@ -282,6 +282,11 @@ libc {
>      __pread64_nocancel;
>      __close_nocancel;
>      __sigtimedwait;
> +    __timer_active_sigev_thread;
> +    __timer_active_sigev_thread_lock;
> +    __timer_helper_once;
> +    __timer_helper_tid;
> +    __timer_start_helper_thread;
>      # functions used by nscd
>      __netlink_assert_response;
>    }

Ok.

> diff --git a/sysdeps/unix/sysv/linux/kernel-posix-timers.h b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
> index 959a81a4d2..17fc32d48f 100644
> --- a/sysdeps/unix/sysv/linux/kernel-posix-timers.h
> +++ b/sysdeps/unix/sysv/linux/kernel-posix-timers.h
> @@ -26,19 +26,27 @@
>  extern int __no_posix_timers attribute_hidden;
>  
>  /* Callback to start helper thread.  */
> -extern void __start_helper_thread (void) attribute_hidden;
> +extern void __timer_start_helper_thread (void);
> +libc_hidden_proto (__timer_start_helper_thread)
>  
>  /* Control variable for helper thread creation.  */
> -extern pthread_once_t __helper_once attribute_hidden;
> +extern pthread_once_t __timer_helper_once;
> +libc_hidden_proto (__timer_helper_once)
> +
> +/* Called from fork so that the new subprocess re-creates the
> +   notification thread if necessary.  */
> +void __timer_fork_subprocess (void) attribute_hidden;
>  
>  /* TID of the helper thread.  */
> -extern pid_t __helper_tid attribute_hidden;
> +extern pid_t __timer_helper_tid;
> +libc_hidden_proto (__timer_helper_tid)
>  
>  /* List of active SIGEV_THREAD timers.  */
> -extern struct timer *__active_timer_sigev_thread attribute_hidden;
> -/* Lock for the __active_timer_sigev_thread.  */
> -extern pthread_mutex_t __active_timer_sigev_thread_lock attribute_hidden;
> -
> +extern struct timer *__timer_active_sigev_thread;
> +libc_hidden_proto (__timer_active_sigev_thread)
> +/* Lock for __timer_active_sigev_thread.  */
> +extern pthread_mutex_t __timer_active_sigev_thread_lock;
> +libc_hidden_proto (__timer_active_sigev_thread_lock)
>  
>  /* Type of timers in the kernel.  */
>  typedef int kernel_timer_t;

Ok.

> diff --git a/sysdeps/unix/sysv/linux/timer_create.c b/sysdeps/unix/sysv/linux/timer_create.c
> index 1ea0086487..b21b0ca949 100644
> --- a/sysdeps/unix/sysv/linux/timer_create.c
> +++ b/sysdeps/unix/sysv/linux/timer_create.c
> @@ -74,8 +74,8 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
>      else
>        {
>  	/* Create the helper thread.  */
> -	pthread_once (&__helper_once, __start_helper_thread);
> -	if (__helper_tid == 0)
> +	pthread_once (&__timer_helper_once, __timer_start_helper_thread);
> +	if (__timer_helper_tid == 0)
>  	  {
>  	    /* No resources to start the helper thread.  */
>  	    __set_errno (EAGAIN);

Ok.

> @@ -118,7 +118,7 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
>  	  { .sigev_value.sival_ptr = newp,
>  	    .sigev_signo = SIGTIMER,
>  	    .sigev_notify = SIGEV_SIGNAL | SIGEV_THREAD_ID,
> -	    ._sigev_un = { ._pad = { [0] = __helper_tid } } };
> +	    ._sigev_un = { ._pad = { [0] = __timer_helper_tid } } };
>  
>  	/* Create the timer.  */
>  	int res;
> @@ -132,10 +132,10 @@ timer_create (clockid_t clock_id, struct sigevent *evp, timer_t *timerid)
>  	  }
>  
>  	/* Add to the queue of active timers with thread delivery.  */
> -	pthread_mutex_lock (&__active_timer_sigev_thread_lock);
> -	newp->next = __active_timer_sigev_thread;
> -	__active_timer_sigev_thread = newp;
> -	pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
> +	pthread_mutex_lock (&__timer_active_sigev_thread_lock);
> +	newp->next = __timer_active_sigev_thread;
> +	__timer_active_sigev_thread = newp;
> +	pthread_mutex_unlock (&__timer_active_sigev_thread_lock);
>  
>  	*timerid = timer_to_timerid (newp);
>        }

Ok.

> diff --git a/sysdeps/unix/sysv/linux/timer_delete.c b/sysdeps/unix/sysv/linux/timer_delete.c
> index 50aac8fc27..a7a183591e 100644
> --- a/sysdeps/unix/sysv/linux/timer_delete.c
> +++ b/sysdeps/unix/sysv/linux/timer_delete.c
> @@ -42,12 +42,12 @@ timer_delete (timer_t timerid)
>  	  struct timer *kt = timerid_to_timer (timerid);
>  
>  	  /* Remove the timer from the list.  */
> -	  pthread_mutex_lock (&__active_timer_sigev_thread_lock);
> -	  if (__active_timer_sigev_thread == kt)
> -	    __active_timer_sigev_thread = kt->next;
> +	  pthread_mutex_lock (&__timer_active_sigev_thread_lock);
> +	  if (__timer_active_sigev_thread == kt)
> +	    __timer_active_sigev_thread = kt->next;
>  	  else
>  	    {
> -	      struct timer *prevp = __active_timer_sigev_thread;
> +	      struct timer *prevp = __timer_active_sigev_thread;
>  	      while (prevp->next != NULL)
>  		if (prevp->next == kt)
>  		  {

Ok.

> @@ -57,7 +57,7 @@ timer_delete (timer_t timerid)
>  		else
>  		  prevp = prevp->next;
>  	    }
> -	  pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
> +	  pthread_mutex_unlock (&__timer_active_sigev_thread_lock);
>  
>  	  free (kt);
>  	}

Ok.

> diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c
> index 4098da8a5f..8d8c1a1e76 100644
> --- a/sysdeps/unix/sysv/linux/timer_routines.c
> +++ b/sysdeps/unix/sysv/linux/timer_routines.c
> @@ -26,10 +26,13 @@
>  
>  
>  /* List of active SIGEV_THREAD timers.  */
> -struct timer *__active_timer_sigev_thread;
> -/* Lock for the __active_timer_sigev_thread.  */
> -pthread_mutex_t __active_timer_sigev_thread_lock = PTHREAD_MUTEX_INITIALIZER;
> +struct timer *__timer_active_sigev_thread __attribute__ ((nocommon));
> +libc_hidden_data_def (__timer_active_sigev_thread)
>  
> +/* Lock for _timer_active_sigev_thread.  */
> +pthread_mutex_t __timer_active_sigev_thread_lock __attribute__ ((nocommon))
> +  = PTHREAD_MUTEX_INITIALIZER;
> +libc_hidden_data_def (__timer_active_sigev_thread_lock)
>  
>  struct thread_start_data
>  {

Ok.  I think we should either use a macro initializer for global without
an initialization or add a attribute macro on cdef.h for 
__attribute__ ((nocommon).

> @@ -59,7 +62,7 @@ timer_sigev_thread (void *arg)
>  
>  
>  /* Helper function to support starting threads for SIGEV_THREAD.  */
> -static void *
> +static _Noreturn void *
>  timer_helper_thread (void *arg)
>  {
>    /* Endless loop of waiting for signals.  The loop is only ended when

Ok.

> @@ -68,16 +71,16 @@ timer_helper_thread (void *arg)
>      {
>        siginfo_t si;
>  
> -      while (sigwaitinfo (&sigtimer_set, &si) < 0);
> +      while (__sigwaitinfo (&sigtimer_set, &si) < 0);
>        if (si.si_code == SI_TIMER)
>  	{
>  	  struct timer *tk = (struct timer *) si.si_ptr;
>  
>  	  /* Check the timer is still used and will not go away
>  	     while we are reading the values here.  */
> -	  pthread_mutex_lock (&__active_timer_sigev_thread_lock);
> +	  __pthread_mutex_lock (&__timer_active_sigev_thread_lock);
>  
> -	  struct timer *runp = __active_timer_sigev_thread;
> +	  struct timer *runp = __timer_active_sigev_thread;
>  	  while (runp != NULL)
>  	    if (runp == tk)
>  	      break;

Ok.

> @@ -96,45 +99,44 @@ timer_helper_thread (void *arg)
>  		  td->sival = tk->sival;
>  
>  		  pthread_t th;
> -		  pthread_create (&th, &tk->attr, timer_sigev_thread, td);
> +		  __pthread_create (&th, &tk->attr, timer_sigev_thread, td);
>  		}
>  	    }
>  
> -	  pthread_mutex_unlock (&__active_timer_sigev_thread_lock);
> +	  __pthread_mutex_unlock (&__timer_active_sigev_thread_lock);
>  	}
> -      else if (si.si_code == SI_TKILL)
> -	/* The thread is canceled.  */
> -	pthread_exit (NULL);
>      }
>  }
>  
>  
>  /* Control variable for helper thread creation.  */
> -pthread_once_t __helper_once attribute_hidden;
> +pthread_once_t __timer_helper_once __attribute__ ((nocommon))
> +  = PTHREAD_ONCE_INIT;
> +libc_hidden_data_def (__timer_helper_once)
>  
>  
>  /* TID of the helper thread.  */
> -pid_t __helper_tid attribute_hidden;
> +pid_t __timer_helper_tid __attribute__ ((nocommon));
> +libc_hidden_data_def (__timer_helper_tid)
>  
>  
>  /* Reset variables so that after a fork a new helper thread gets started.  */
> -static void
> -reset_helper_control (void)
> +void
> +__timer_fork_subprocess (void)
>  {
> -  __helper_once = PTHREAD_ONCE_INIT;
> -  __helper_tid = 0;
> +  __timer_helper_once = PTHREAD_ONCE_INIT;
> +  __timer_helper_tid = 0;
>  }
>  
>  

Ok.

>  void
> -attribute_hidden
> -__start_helper_thread (void)
> +__timer_start_helper_thread (void)
>  {
>    /* The helper thread needs only very little resources
>       and should go away automatically when canceled.  */
>    pthread_attr_t attr;
> -  (void) pthread_attr_init (&attr);
> -  (void) pthread_attr_setstacksize (&attr, __pthread_get_minstack (&attr));
> +  (void) __pthread_attr_init (&attr);
> +  (void) __pthread_attr_setstacksize (&attr, __pthread_get_minstack (&attr));
>  
>    /* Block all signals in the helper thread but SIGSETXID.  */
>    sigset_t ss;

No need the extra casts here.

> @@ -143,21 +145,18 @@ __start_helper_thread (void)
>    int res = __pthread_attr_setsigmask_internal (&attr, &ss);
>    if (res != 0)
>      {
> -      pthread_attr_destroy (&attr);
> +      __pthread_attr_destroy (&attr);
>        return;
>      }
>  
>    /* Create the helper thread for this timer.  */
>    pthread_t th;
> -  res = pthread_create (&th, &attr, timer_helper_thread, NULL);
> +  res = __pthread_create (&th, &attr, timer_helper_thread, NULL);
>    if (res == 0)
>      /* We managed to start the helper thread.  */
> -    __helper_tid = ((struct pthread *) th)->tid;
> +    __timer_helper_tid = ((struct pthread *) th)->tid;
>  
>    /* No need for the attribute anymore.  */
> -  (void) pthread_attr_destroy (&attr);
> -
> -  /* We have to make sure that after fork()ing a new helper thread can
> -     be created.  */
> -  pthread_atfork (NULL, NULL, reset_helper_control);
> +  (void) __pthread_attr_destroy (&attr);
>  }
> +libc_hidden_def (__timer_start_helper_thread)
> 

Ok.