[glibc/azanella/posix_spawn-optimizations] linux: posix_spawn signal handling optimization

[Adhemerval Zanella <azanella at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=d91b61793810089850ac365cbe99ed3bd7ea32a1

commit d91b61793810089850ac365cbe99ed3bd7ea32a1
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Wed May 8 15:13:18 2019 -0300

    linux: posix_spawn signal handling optimization
    
    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 when a signal action
    different than SIG_DFL or SIG_IGN is used on a per-process variable.  It
    allows to replace the sigprocmask on posix_spawn with an iteration over the
    only non default set signals.  It allows to call sigaction only for the
    signals that has been actually changed by the process.  It assumes the usual
    usage as either not use signal handlers at all or use a limited set at
    program start, so only new actions are tracked.
    
    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.

Diff:
---
 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(-)

diff --git a/include/atomic.h b/include/atomic.h
index ee1978e..72609ef 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 1ac41ad..131ae05 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 0000000..466e66f
--- /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 ddeeb0b..9cae117 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 3562011..385442f 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 52722b0..7b61a42 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 handlers have been changed from default or ignore
+	 so posix_spawn helper process knows exactly which one to reset.  This
+	 is minor optimization to avoid NSIG sigaction calls to query the
+	 signal handler status for each posix_spawn call.
+	 It assumes that the signal handling set/reset in process setup
+	 and usually not changed often, so it does not bother to track
+	 when signal handler is reset to default or ignore.  */
+      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 713d484..6c98c83 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 92b6a3e..e964ebb 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -130,17 +130,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)