Re: [PATCH 4/5] linux: Optimize posix_spawn spurious sigaction calls

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

Ping.

On 31/07/2019 15:31, Adhemerval Zanella wrote:
> The child helper process on Linux posix_spawn child must ensure that no signal
> handler are enabled, so the signal disposition must be either SIG_DFL or
> SIG_IGN.  However, it requires a sigprocmask to obtain the current signal mask
> and at least _NSIG sigaction calls to reset the signal handlers for each
> posix_spawn call.
> 
> This patch optimizes it by tracking on sigaction implementation when a signal
> action is set different than SIG_DFL or SIG_IGN.  It allows remove a
> sigprocmask call and isse sigaction to reset the disposition only the signals
> that has non-default actions set.
> 
> It might incur in false positive, since it not easy to remove bits from the
> mask without race conditions, but it does not allow false negative since the
> mask is updated atomically prior the syscall.  The false positive incur in
> just extra sigactions on posix_spawn.
> 
> Checked on x86_64 and i686.
> 
> 	* include/atomic.h (atomic_fetch_or_seq_cst, atomic_fetch_or_seq_cst):
> 	New macros.
> 	* posix/Makefile (tests): Add tst-spawn6.
> 	* posix/tst-spawn6.c: New file.
> 	* sysdeps/generic/sigsetops.h (__sigorset_atomic): New macro.
> 	* sysdeps/unix/sysv/linux/internal-signals.h (__get_sighandler_set):
> 	New prototype.
> 	* sysdeps/unix/sysv/linux/sigaction.c (__get_sighandler_set): New
> 	function.
> 	(__libc_sigaction): Set the internal handler_set for a new action.
> 	* sysdeps/unix/sysv/linux/sigsetops.h (__sigorset_atomic,
> 	__sigaddset_atomic): New macros.
> 	* sysdeps/unix/sysv/linux/spawni.c (spawni_child): Replace
> 	__sigprocmask with __get_sighandler_set.
> ---
>  include/atomic.h                           |  10 +
>  posix/Makefile                             |   4 +-
>  posix/tst-spawn6.c                         | 220 +++++++++++++++++++++
>  sysdeps/generic/sigsetops.h                |   7 +
>  sysdeps/unix/sysv/linux/internal-signals.h |   3 +
>  sysdeps/unix/sysv/linux/sigaction.c        |  17 ++
>  sysdeps/unix/sysv/linux/sigsetops.h        |  15 ++
>  sysdeps/unix/sysv/linux/spawni.c           |   9 +-
>  8 files changed, 278 insertions(+), 7 deletions(-)
>  create mode 100644 posix/tst-spawn6.c
> 
> diff --git a/include/atomic.h b/include/atomic.h
> index ee1978eb3b..72609efde9 100644
> --- a/include/atomic.h
> +++ b/include/atomic.h
> @@ -646,6 +646,9 @@ void __atomic_link_error (void);
>  # define atomic_fetch_or_release(mem, operand) \
>    ({ __atomic_check_size((mem));					      \
>    __atomic_fetch_or ((mem), (operand), __ATOMIC_RELEASE); })
> +# define atomic_fetch_or_seq_cst(mem, operand) \
> +  ({ __atomic_check_size((mem));					      \
> +  __atomic_fetch_or ((mem), (operand), __ATOMIC_SEQ_CST); })
>  
>  # define atomic_fetch_xor_release(mem, operand) \
>    ({ __atomic_check_size((mem));					      \
> @@ -791,6 +794,13 @@ void __atomic_link_error (void);
>     ({ atomic_thread_fence_release ();					      \
>     atomic_fetch_or_acquire ((mem), (operand)); })
>  # endif
> +# ifndef atomic_fetch_or_seq_cst
> +#  define atomic_fetch_or_seq_cst(mem, operand) \
> +   ({ atomic_thread_fence_acquire ();					      \
> +   atomic_fetch_or_relaxed ((mem), (operand));				      \
> +   atomic_thread_fence_release (); })
> +# endif
> +
>  
>  # ifndef atomic_fetch_xor_release
>  /* Failing the atomic_compare_exchange_weak_release reloads the value in
> diff --git a/posix/Makefile b/posix/Makefile
> index 1ac41ad85a..131ae052fd 100644
> --- a/posix/Makefile
> +++ b/posix/Makefile
> @@ -102,7 +102,8 @@ tests		:= test-errno tstgetopt testfnm runtests runptests \
>  		   tst-sysconf-empty-chroot tst-glob_symlinks tst-fexecve \
>  		   tst-glob-tilde test-ssize-max tst-spawn4 bug-regex37 \
>  		   bug-regex38 tst-regcomp-truncated tst-spawn-chdir \
> -		   tst-spawn5
> +		   tst-spawn5 \
> +		   tst-spawn6
>  tests-internal	:= bug-regex5 bug-regex20 bug-regex33 \
>  		   tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3 \
>  		   tst-glob_lstat_compat tst-spawn4-compat
> @@ -255,6 +256,7 @@ tst-exec-ARGS = -- $(host-test-program-cmd)
>  tst-exec-static-ARGS = $(tst-exec-ARGS)
>  tst-execvpe5-ARGS = -- $(host-test-program-cmd)
>  tst-spawn-ARGS = -- $(host-test-program-cmd)
> +tst-spawn6-ARGS = -- $(host-test-program-cmd)
>  tst-spawn-static-ARGS = $(tst-spawn-ARGS)
>  tst-spawn5-ARGS = -- $(host-test-program-cmd)
>  tst-dir-ARGS = `pwd` `cd $(common-objdir)/$(subdir); pwd` `cd $(common-objdir); pwd` $(objpfx)tst-dir
> diff --git a/posix/tst-spawn6.c b/posix/tst-spawn6.c
> new file mode 100644
> index 0000000000..466e66f104
> --- /dev/null
> +++ b/posix/tst-spawn6.c
> @@ -0,0 +1,220 @@
> +/* Tests for posix_spawn signal handling.
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <getopt.h>
> +#include <spawn.h>
> +#include <sys/wait.h>
> +
> +#include <support/check.h>
> +#include <support/xunistd.h>
> +#include <support/support.h>
> +#include <array_length.h>
> +
> +/* Nonzero if the program gets called via `exec'.  */
> +static int restart;
> +#define CMDLINE_OPTIONS \
> +  { "restart", no_argument, &restart, 1 },
> +
> +enum spawn_test_t
> +{
> +  SPAWN_SETSIGMASK,
> +  SPAWN_SETSIGDEF
> +};
> +
> +static int signal_to_check[] =
> +{
> +  SIGHUP, SIGINT, SIGALRM, SIGUSR2
> +};
> +
> +/* Called on process re-execution.  */
> +static int
> +handle_restart (enum spawn_test_t test)
> +{
> +  switch (test)
> +    {
> +    case SPAWN_SETSIGMASK:
> +      {
> +	sigset_t mask;
> +	sigprocmask (SIG_BLOCK, NULL, &mask);
> +	for (int i = 0; i < array_length (signal_to_check); i++)
> +	  if (sigismember (&mask, signal_to_check[i]) != 1)
> +	    exit (EXIT_FAILURE);
> +      }
> +      break;
> +    case SPAWN_SETSIGDEF:
> +      {
> +	for (int i = 0; i < array_length (signal_to_check); i++)
> +	  {
> +	    struct sigaction act;
> +	    if (sigaction (signal_to_check[i], NULL, &act) != 0)
> +	      exit (EXIT_FAILURE);
> +	    if (act.sa_handler != SIG_DFL)
> +	      exit (EXIT_FAILURE);
> +	  }
> +      }
> +      break;
> +    }
> +
> +  return 0;
> +}
> +
> +/* Common argument used for process re-execution.  */
> +static char *initial_spargv[5];
> +static size_t initial_spargv_size;
> +
> +/* Re-execute the test process with both '--direct', '--restart', and the
> +   TEST (as integer value) as arguments.  */
> +static void
> +reexecute (enum spawn_test_t test, const posix_spawnattr_t *attrp)
> +{
> +  char *spargv[8];
> +  int i;
> +
> +  for (i = 0; i < initial_spargv_size; i++)
> +    spargv[i] = initial_spargv[i];
> +  /* Three digits per byte plus null terminator.  */
> +  char teststr[3 * sizeof (test) + 1];
> +  snprintf (teststr, array_length (teststr), "%d", test);
> +  spargv[i++] = teststr;
> +  spargv[i] = NULL;
> +  TEST_VERIFY (i < 8);
> +
> +  pid_t pid;
> +  int status;
> +
> +  TEST_COMPARE (posix_spawn (&pid, spargv[0], NULL, attrp, spargv, environ),
> +		0);
> +  TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
> +  TEST_VERIFY (WIFEXITED (status));
> +  TEST_VERIFY (!WIFSIGNALED (status));
> +  TEST_COMPARE (WEXITSTATUS (status), 0);
> +}
> +
> +/* Test if POSIX_SPAWN_SETSIGMASK change the spawn process signal mask to
> +   the value blocked signals defined by SIGNAL_TO_CHECK signals.  */
> +static void
> +do_test_setsigmask (void)
> +{
> +  posix_spawnattr_t attr;
> +  /* posix_spawnattr_init does not fail.  */
> +  posix_spawnattr_init (&attr);
> +
> +  {
> +    sigset_t mask;
> +    TEST_COMPARE (sigemptyset (&mask), 0);
> +    for (int i = 0; i < array_length (signal_to_check); i++)
> +      TEST_COMPARE (sigaddset (&mask, signal_to_check[i]), 0);
> +    TEST_COMPARE (posix_spawnattr_setsigmask (&attr, &mask), 0);
> +    TEST_COMPARE (posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSIGMASK), 0);
> +  }
> +
> +  /* Change current mask to be different than the one asked for spawned
> +     process.  */
> +  {
> +    sigset_t empty_mask, current_mask;
> +    TEST_COMPARE (sigemptyset (&empty_mask), 0);
> +    TEST_COMPARE (sigprocmask (SIG_BLOCK, &empty_mask, &current_mask), 0);
> +
> +    reexecute (SPAWN_SETSIGMASK, &attr);
> +
> +    TEST_COMPARE (sigprocmask (SIG_SETMASK, &current_mask, NULL), 0);
> +  }
> +}
> +
> +/* Test if POSIX_SPAWN_SETSIGDEF change the spawn process signal actions
> +   defined by SIGNAL_TO_CHECK signals to default actions.  */
> +static void
> +do_test_setsigdef (void)
> +{
> +  posix_spawnattr_t attr;
> +  /* posix_spawnattr_init does not fail.  */
> +  posix_spawnattr_init (&attr);
> +
> +  {
> +    sigset_t mask;
> +    TEST_COMPARE (sigemptyset (&mask), 0);
> +    for (int i = 0; i < array_length (signal_to_check); i++)
> +      TEST_COMPARE (sigaddset (&mask, signal_to_check[i]), 0);
> +    TEST_COMPARE (posix_spawnattr_setsigdefault (&attr, &mask), 0);
> +    TEST_COMPARE (posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSIGDEF), 0);
> +  }
> +
> +  /* Change current signal disposition to be different than the one asked for
> +     spawned process.  */
> +  struct sigaction default_act[array_length (signal_to_check)];
> +  {
> +    sigset_t empty_mask;
> +    TEST_COMPARE (sigemptyset (&empty_mask), 0);
> +    for (int i = 0; i < array_length (signal_to_check); i++)
> +      TEST_COMPARE (sigaction (signal_to_check[i],
> +			       &((struct sigaction) { .sa_handler = SIG_IGN,
> +						      .sa_mask = empty_mask,
> +						      .sa_flags = 0 }),
> +			       &default_act[i]),
> +		    0);
> +  }
> +
> +  reexecute (SPAWN_SETSIGDEF, &attr);
> +
> +  /* Restore signal dispositions.  */
> +  for (int i = 0; i < array_length (signal_to_check); i++)
> +    TEST_COMPARE (sigaction (signal_to_check[i], &default_act[i], NULL), 0);
> +}
> +
> +static int
> +do_test (int argc, char *argv[])
> +{
> +  /* We must have one or four parameters left if called initially:
> +       + path for ld.so		optional
> +       + "--library-path"	optional
> +       + the library path	optional
> +       + the application name
> +
> +     Plus one parameter to indicate which test to execute through
> +     re-execution.
> +
> +     So for default usage without --enable-hardcoded-path-in-tests, it
> +     will be called initially with 5 arguments and later with 2.  For
> +     --enable-hardcoded-path-in-tests it will be called with 2 arguments
> +     regardless.  */
> +
> +  if (argc != (restart ? 2 : 5) && argc != 2)
> +    FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
> +
> +  if (restart)
> +    return handle_restart (atoi (argv[1]));
> +
> +  {
> +    int i;
> +    for (i = 0; i < (argc == 5 ? 4 : 1); i++)
> +      initial_spargv[i] = argv[i + 1];
> +    initial_spargv[i++] = (char *) "--direct";
> +    initial_spargv[i++] = (char *) "--restart";
> +    initial_spargv_size = i;
> +  }
> +
> +  do_test_setsigmask ();
> +  do_test_setsigdef ();
> +
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>
> diff --git a/sysdeps/generic/sigsetops.h b/sysdeps/generic/sigsetops.h
> index ddeeb0b0d5..9cae11771b 100644
> --- a/sysdeps/generic/sigsetops.h
> +++ b/sysdeps/generic/sigsetops.h
> @@ -66,6 +66,13 @@
>      0;						\
>    }))
>  
> +# define __sigorset_atomic(set)			\
> +  (__extension__ ({				\
> +    __sigset_t __mask = __sigmask (sig);	\
> +    atomic_fetch_or_seq_cst (set, mask);	\
> +    0;						\
> +  }))
> +
>  # define __sigdelset(set, sig)			\
>    (__extension__ ({				\
>      __sigset_t __mask = __sigmask (sig);	\
> diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
> index 3562011d21..385442f81e 100644
> --- a/sysdeps/unix/sysv/linux/internal-signals.h
> +++ b/sysdeps/unix/sysv/linux/internal-signals.h
> @@ -88,4 +88,7 @@ __libc_signal_restore_set (const sigset_t *set)
>  /* Used to communicate with signal handler.  */
>  extern struct xid_command *__xidcmd attribute_hidden;
>  
> +/* Used to obtained the modified signal handlers.  */
> +extern void __get_sighandler_set (sigset_t *set) attribute_hidden;
> +
>  #endif
> diff --git a/sysdeps/unix/sysv/linux/sigaction.c b/sysdeps/unix/sysv/linux/sigaction.c
> index 52722b08ae..3bcf3946ab 100644
> --- a/sysdeps/unix/sysv/linux/sigaction.c
> +++ b/sysdeps/unix/sysv/linux/sigaction.c
> @@ -20,6 +20,7 @@
>  #include <string.h>
>  
>  #include <sysdep.h>
> +#include <sigsetops.h>
>  #include <sys/syscall.h>
>  
>  /* New ports should not define the obsolete SA_RESTORER, however some
> @@ -36,6 +37,13 @@
>  # define STUB(act, sigsetsize) (sigsetsize)
>  #endif
>  
> +static sigset_t handler_set;
> +
> +void __get_sighandler_set (sigset_t *set)
> +{
> +  *set = handler_set;
> +}
> +
>  /* If ACT is not NULL, change the action for SIG to *ACT.
>     If OACT is not NULL, put the old action for SIG in *OACT.  */
>  int
> @@ -47,6 +55,15 @@ __libc_sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
>  
>    if (act)
>      {
> +      /* Tracks which signal had a signal handler set different from default
> +	 (SIG_DFL/SIG_IGN).  It allows optimize posix_spawn to reset only
> +	 those signals.  It might incur in false positive, since it not easy
> +	 to remove bits from the mask without race conditions, but it does not
> +	 allow false negative since the mask is updated atomically prior the
> +	 syscall.  The false positive incur in just extra sigactions on
> +	 posix_spawn.  */
> +      if (act->sa_handler != SIG_DFL && act->sa_handler != SIG_IGN)
> +	__sigaddset_atomic (&handler_set, sig);
>        kact.k_sa_handler = act->sa_handler;
>        memcpy (&kact.sa_mask, &act->sa_mask, sizeof (sigset_t));
>        kact.sa_flags = act->sa_flags;
> diff --git a/sysdeps/unix/sysv/linux/sigsetops.h b/sysdeps/unix/sysv/linux/sigsetops.h
> index 713d4840d8..6c98c83e42 100644
> --- a/sysdeps/unix/sysv/linux/sigsetops.h
> +++ b/sysdeps/unix/sysv/linux/sigsetops.h
> @@ -20,6 +20,7 @@
>  #define _SIGSETOPS_H 1
>  
>  #include <signal.h>
> +#include <atomic.h>
>  
>  /* Return a mask that includes the bit for SIG only.  */
>  # define __sigmask(sig) \
> @@ -80,6 +81,12 @@
>      (void)0;							\
>    }))
>  
> +# define __sigorset_atomic(dest, left, right)	\
> +  (__extension__ ({				\
> +     atomic_fetch_or_seq_cst (dest, left, right); \
> +    0;						\
> +  }))
> +
>  /* These macros needn't check for a bogus signal number;
>     error checking is done in the non-__ versions.  */
>  # define __sigismember(set, sig)				\
> @@ -97,6 +104,14 @@
>      (void)0;							\
>    }))
>  
> +# define __sigaddset_atomic(set, sig)				\
> +  (__extension__ ({						\
> +    unsigned long int __mask = __sigmask (sig);			\
> +    unsigned long int __word = __sigword (sig);			\
> +    atomic_fetch_or_seq_cst (&((set)->__val[__word]), __mask);	\
> +    (void)0;							\
> +  }))
> +
>  # define __sigdelset(set, sig)					\
>    (__extension__ ({						\
>      unsigned long int __mask = __sigmask (sig);			\
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index 0f7a8ca5df..264edd09c6 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -132,17 +132,14 @@ spawni_child (void *arguments)
>    const posix_spawnattr_t *restrict attr = args->attr;
>    const posix_spawn_file_actions_t *file_actions = args->fa;
>  
> -  /* The child must ensure that no signal handler are enabled because it shared
> +  /* The child must ensure that no signal handler are enabled because it share
>       memory with parent, so the signal disposition must be either SIG_DFL or
> -     SIG_IGN.  It does by iterating over all signals and although it could
> -     possibly be more optimized (by tracking which signal potentially have a
> -     signal handler), it might requires system specific solutions (since the
> -     sigset_t data type can be very different on different architectures).  */
> +     SIG_IGN.  */
>    struct sigaction sa;
>    memset (&sa, '\0', sizeof (sa));
>  
>    sigset_t hset;
> -  __sigprocmask (SIG_BLOCK, 0, &hset);
> +  __get_sighandler_set (&hset);
>    for (int sig = 1; sig < _NSIG; ++sig)
>      {
>        if ((attr->__flags & POSIX_SPAWN_SETSIGDEF)
>