[PATCH] nptl: Provide a way to block all signals for the timer helper thread
Carlos O'Donell
carlos@redhat.com
Tue May 12 19:09:21 GMT 2020
On 5/12/20 11:02 AM, Florian Weimer wrote:
> timer_create needs to create threads with all signals blocked,
> including SIGTIMER (which happens to equal SIGCANCEL). Add a new
> internal interface which provides an explicit way to achieve that.
>
> Fixes commit b3cae39dcbfa2432b3f3aa28854d8ac57f0de1b8 ("nptl: Start
> new threads with all signals blocked [BZ #25098]").
Please post v2.
Testing is clean for x86_64 and i686, the tst-timer-sigmask failure is gone,
and I confirm that logically now we don't unblock SIGCANCEL (whose semantic
overlap with SIGTIMER is the root cause of this failure e.g. poor design).
My request below is that we need document the internal interface quirk and
we need to explain why it's there and why __pthread_create_internal has this
quirk.
I agree with Adhemerval that a future cleanup would be good to somehow decouple
SIGTIMER from SIGCANCEL, but I don't want to block this regression fix.
Tested-by: Carlos O'Donell <carlos@redhat.com>
> Tested on x86_64-linux-gnu.
>
> -----
Needs to be 3 dashes, otherwise this is included in the commit message
by git-am. Please adjust your scripts.
> nptl/Versions | 1 +
> nptl/pthreadP.h | 10 +++++++++
> nptl/pthread_create.c | 36 +++++++++++++++++++++++---------
> sysdeps/unix/sysv/linux/timer_routines.c | 16 ++++++--------
> 4 files changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/nptl/Versions b/nptl/Versions
> index f7140277f5..17a711dfb1 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -285,5 +285,6 @@ libpthread {
> __pthread_barrier_init; __pthread_barrier_wait;
> __shm_directory;
> __libpthread_freeres;
> + __pthread_create_internal;
OK. Provide new __pthread_create_internal@GLIBC_PRIVATE that takes a signal mask.
> }
> }
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index c4e72f57a9..4e065ace58 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -339,6 +339,16 @@ extern void __pthread_cleanup_upto (__jmp_buf target, char *targetframe);
> hidden_proto (__pthread_cleanup_upto)
> #endif
>
> +/* Exactly like pthread_create if NEW_SIGMASK == NULL. Otherwise, do
> + not use the current signal mask for the new thread, but set it to
> + *NEW_SIGMASK instead (without unblocking internal signals). */
> +extern int __pthread_create_internal (pthread_t *newthread,
> + const pthread_attr_t *attr,
> + void *(*start_routine) (void *),
> + void *arg, const sigset_t *new_sigmask);
> +#if IS_IN (libpthread)
> +hidden_proto (__pthread_create_internal)
> +#endif
OK.
>
> /* Functions with versioned interfaces. */
> extern int __pthread_create_2_1 (pthread_t *newthread,
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index afd379e89a..2430d65723 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -603,10 +603,10 @@ report_thread_creation (struct pthread *pd)
> return false;
> }
>
> -
> int
> -__pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
> - void *(*start_routine) (void *), void *arg)
Could you please provide a detailed explanation of why we have a signal
mask here?
Future reviewers of this code will be left wondering why the interface was
designed as it is, and that's difficult to explain without a more thorough
code audit.
I would like to see a detailed paragraph here that explains the arguments
(most of which are trivial) and particularly new_sigmask and why it's
needed.
If you don't want to explain this here, because you think this is generic
infrastructure, then we need to explain this at the call site that uses
new_sigmask != NULL.
> +__pthread_create_internal (pthread_t *newthread, const pthread_attr_t *attr,
> + void *(*start_routine) (void *), void *arg,
> + const sigset_t *new_sigmask)
> {
> STACK_VARIABLES;
>
> @@ -762,14 +762,21 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
> sigset_t original_sigmask;
> __libc_signal_block_all (&original_sigmask);
OK. At this point all signals are blocked. SIGCANCEL and SIGSETXID. Later
on we will call _libc_signal_restore_set (&pd->sigmask); from START_THREAD_DEFN
to restore the signal mask (and unblock signals, potentially delivering some).
>
> - /* Conceptually, the new thread needs to inherit the signal mask of
> - this thread. Therefore, it needs to restore the saved signal
> - mask of this thread, so save it in the startup information. */
> - pd->sigmask = original_sigmask;
> + if (new_sigmask != NULL)
> + /* The caller supplied the signal mask for the new thread. */
> + pd->sigmask = *new_sigmask;
OK. The if clause does the new work, which is just to set the signal mask
exactly as expected and don't play with SIGCANCEL, since this is not a
normal thread.
> + else
> + {
OK. We have the else clause do the normal work.
> + /* Conceptually, the new thread needs to inherit the signal mask
> + of this thread. Therefore, it needs to restore the saved
> + signal mask of this thread, so save it in the startup
> + information. */
> + pd->sigmask = original_sigmask;
OK.
>
> - /* Reset the cancellation signal mask in case this thread is running
> - cancellation. */
> - __sigdelset (&pd->sigmask, SIGCANCEL);
> + /* Reset the cancellation signal mask in case this thread is
> + running cancellation. */
> + __sigdelset (&pd->sigmask, SIGCANCEL);
OK.
> + }
>
> /* Start the thread. */
> if (__glibc_unlikely (report_thread_creation (pd)))
> @@ -873,6 +880,15 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>
> return retval;
> }
> +hidden_def (__pthread_create_internal)
> +
> +int
> +__pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
> + void *(*start_routine) (void *), void *arg)
> +{
> + return __pthread_create_internal (newthread, attr, start_routine, arg,
> + false);
My preference is to use NULL with an informative cast.
Unless you can show existing practice. I'm not a fan of using false like this.
> +}
> versioned_symbol (libpthread, __pthread_create_2_1, pthread_create, GLIBC_2_1);
>
>
> diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c
> index 63083f6f91..7a5fa3dbb2 100644
> --- a/sysdeps/unix/sysv/linux/timer_routines.c
> +++ b/sysdeps/unix/sysv/linux/timer_routines.c
> @@ -136,23 +136,19 @@ __start_helper_thread (void)
> (void) pthread_attr_init (&attr);
> (void) pthread_attr_setstacksize (&attr, __pthread_get_minstack (&attr));
>
> - /* Block all signals in the helper thread but SIGSETXID. To do this
> - thoroughly we temporarily have to block all signals here. The
> - helper can lose wakeups if SIGTIMER is not blocked throughout. */
> - sigset_t ss;
> - __libc_signal_block_app (&ss);
> - __libc_signal_block_sigtimer (NULL);
OK. Yup, we don't need to do any of this work because we have fixed
__pthread_create_internal to handle the sigmaks case for us (a good refactoring)
and reduces signal latency in calling thread.
> + /* Block all signals in the helper thread but SIGSETXID. */
> + sigset_t new_sigmask;
> + __sigfillset (&new_sigmask);
> + __sigdelset (&new_sigmask, SIGSETXID);
OK. SIGTIMER/SIGCANCEL is blocked. SIGSETXID is not for setxid calls.
The helper thread synchronously calls sigwaitinfo to retrieve the pending
signal.
> /* Create the helper thread for this timer. */
> pthread_t th;
> - int res = pthread_create (&th, &attr, timer_helper_thread, NULL);
> + int res = __pthread_create_internal (&th, &attr, timer_helper_thread, NULL,
> + &new_sigmask);
> if (res == 0)
> /* We managed to start the helper thread. */
> __helper_tid = ((struct pthread *) th)->tid;
>
> - /* Restore the signal mask. */
> - __libc_signal_restore_set (&ss);
OK.
> -
> /* No need for the attribute anymore. */
> (void) pthread_attr_destroy (&attr);
>
>
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list