Re: SIGPROF + SIGCHLD and igc

[Pip Cet]

Pip Cet <pipcet@protonmail.com> writes:
> Thanks, I got the idea now!
>
> Yes, that would work, assuming SIGPROF is never blocked, and that no
> signal interrupting the SIGPROF handler attempts to take the arena lock.
> (Also, we assume the SIGPROF handler has finished by the time the next
> SIGPROF is sent.  I think this assumption is fixable, though, and we'd
> need to fix it for other signals).

Now I've had some time to think about it, I really like this idea, and
think it's our best bet.  Thanks!

Here's a sketch.  The idea is that all signals go through the signal
receiver thread, which serializes them, picks one, enters the arena,
tells the main thread that this signal is now safe to run, re-kills the
main thread, waits for confirmation, leaves the arena, waits for
confirmation AGAIN, then goes back to sleep.

However, the re-kill cannot happen just with the same signal, because we
need to block that in the main thread, in the case of SIGCHLD (and who
knows which other signals are level-triggered in this way, so just do it
for all of them).  Therefore, we kill the main thread twice, once with
SIGURG (no particular reason for that choice) to tell it to unblock
signals and then with the original signal to mark it pending.  (It would
be nicer to raise() the signal in its signal handler after blocking it,
but IIRC that doesn't work).

Assumes atomicity and visibility of stores, as does the rest of Emacs.

This also assumes the signal isn't supposed to be blocked by the time we
receive the SIGURG.  If we need that, we need a shadow signal mask, I
guess.

Pip

diff --git a/src/igc.c b/src/igc.c
index 4b59352681b..a07e331b792 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -814,8 +814,57 @@ IGC_DEFINE_LIST (igc_thread);
   /* The real signal mask we want to restore after handling pending
    * signals.  */
   sigset_t signal_mask;
+
+  pthread_t receiver_thread_id;
+  int signal_receiver_waiting;
+  int signal_receiver_pipe[2];
 };
 
+#define SIGNAL_HANDLER_STARTED 'A'
+#define SIGNAL_HANDLER_FINISHED 'Z'
+
+extern pthread_t main_thread_id;
+
+extern void ArenaEnter (mps_arena_t arena);
+extern void ArenaLeave (mps_arena_t arena);
+
+static void *signal_receiver_thread(void *gc_v)
+{
+  struct igc *gc = gc_v;
+  sigset_t full_mask;
+  sigfillset (&full_mask);
+  pthread_sigmask (SIG_SETMASK, &full_mask, NULL);
+
+  while (true)
+    {
+      sigset_t empty_mask;
+      sigemptyset (&empty_mask);
+      sigsuspend (&empty_mask);
+
+      while (gc->signals_pending)
+       {
+         gc->signals_pending = 0;
+
+         for (int i = 0; i < ARRAYELTS (gc->pending_signals); i++)
+           if (gc->pending_signals[i])
+             {
+               ArenaEnter (gc->arena);
+               gc->signal_receiver_waiting = i;
+               pthread_kill (main_thread_id, SIGURG);
+               pthread_kill (main_thread_id, i);
+               char c;
+               while (read (gc->signal_receiver_pipe[0], &c, 1) != 1);
+               if (c != SIGNAL_HANDLER_STARTED)
+                 emacs_abort ();
+               ArenaLeave (gc->arena);
+               while (read (gc->signal_receiver_pipe[0], &c, 1) != 1);
+               if (c != SIGNAL_HANDLER_FINISHED)
+                 emacs_abort ();
+             }
+       }
+    }
+}
+
 static bool process_one_message (struct igc *gc);
 
 /* The global registry.  */
@@ -4618,6 +4667,10 @@ make_igc (void)
   root_create_exact_ptr (gc, &current_thread);
   root_create_exact_ptr (gc, &all_threads);
 
+  if (pipe (gc->signal_receiver_pipe))
+    emacs_abort ();
+  pthread_create (&gc->receiver_thread_id, NULL, signal_receiver_thread, gc);
+
   enable_messages (gc, true);
   return gc;
 }
@@ -4921,36 +4974,55 @@ gc_signal_handler_can_run (int sig)
 {
   eassume (sig >= 0);
   eassert (sig < ARRAYELTS (global_igc->pending_signals));
-  if (igc_busy_p ())
+  if (pthread_equal (pthread_self (), global_igc->receiver_thread_id))
+    return false;
+  else if (pthread_equal (pthread_self (), main_thread_id))
     {
-      sigset_t sigs;
-      global_igc->signals_pending = 1;
-      global_igc->pending_signals[sig] = 1;
-      sigemptyset (&sigs);
-      sigaddset (&sigs, sig);
-      pthread_sigmask (SIG_BLOCK, &sigs, NULL);
-      /* We cannot raise (sig) here, because there are platforms where
-       * it doesn't work.  */
-      return false;
+      if (global_igc->signal_receiver_waiting != sig)
+       {
+         sigset_t sigs;
+         global_igc->signals_pending = 1;
+         global_igc->pending_signals[sig] = 1;
+         sigemptyset (&sigs);
+         sigaddset (&sigs, sig);
+         pthread_sigmask (SIG_BLOCK, &sigs, NULL);
+         pthread_kill (global_igc->receiver_thread_id, sig);
+         return false;
+       }
+      else if (sig == SIGURG)
+       {
+         sigset_t sigs;
+         sigemptyset (&sigs);
+         for (int i = 0; i < ARRAYELTS (global_igc->pending_signals); i++)
+           if (global_igc->pending_signals[i])
+             sigaddset (&sigs, i);
+         pthread_sigmask (SIG_UNBLOCK, &sigs, NULL);
+         return false;
+       }
+      else
+       global_igc->signal_receiver_waiting = 0;
+      char c = SIGNAL_HANDLER_STARTED;
+      while (write (global_igc->signal_receiver_pipe[1], &c, 1) != 1);
     }
   return true;
 }
 
+bool
+gc_signal_handler_finished (int sig)
+{
+  if (pthread_equal (pthread_self (), main_thread_id))
+    {
+      char c = SIGNAL_HANDLER_FINISHED;
+      while (write (global_igc->signal_receiver_pipe[1], &c, 1) != 1);
+    }
+
+  return true;
+}
+
 /* Called from `maybe_quit'.  This assumes no signals are blocked.  */
 void
 gc_maybe_quit (void)
 {
-  while (global_igc->signals_pending)
-    {
-      global_igc->signals_pending = 0;
-      for (int i = 0; i < ARRAYELTS (global_igc->pending_signals); i++)
-       if (global_igc->pending_signals[i])
-         {
-           global_igc->pending_signals[i] = 0;
-           raise (i);
-         }
-      pthread_sigmask (SIG_SETMASK, &global_igc->signal_mask, NULL);
-    }
 }
 
 DEFUN ("igc--add-extra-dependency", Figc__add_extra_dependency,
diff --git a/src/lisp.h b/src/lisp.h
index 48585c2d8a1..c6c7f772e43 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -48,6 +48,7 @@ #define EMACS_LISP_H
 union gc_header;
 extern void gc_maybe_quit (void);
 extern bool gc_signal_handler_can_run (int);
+extern bool gc_signal_handler_finished (int);
 #else
 INLINE void gc_maybe_quit (void)
 {
@@ -57,6 +58,10 @@ #define EMACS_LISP_H
 {
   return true;
 }
+INLINE bool gc_signal_handler_finished (int sig)
+{
+  return true;
+}
 union gc_header { };
 #endif
 
diff --git a/src/sysdep.c b/src/sysdep.c
index 9270bfa97d9..0eb1466b6a4 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -1719,7 +1719,7 @@ emacs_sigaction_init (struct sigaction *action, 
signal_handler_t handler)
 }
 
 #ifdef FORWARD_SIGNAL_TO_MAIN_THREAD
-static pthread_t main_thread_id;
+pthread_t main_thread_id;
 #endif
 
 /* SIG has arrived at the current process.  Deliver it to the main
@@ -1759,6 +1759,7 @@ deliver_process_signal (int sig, signal_handler_t handler)
     handler (sig);
 
   errno = old_errno;
+  gc_signal_handler_finished (sig);
 }
 
 /* Static location to save a fatal backtrace in a thread.