[runtime] Rework process waiting. (#5175)
authorZoltan Varga <vargaz@gmail.com>
Thu, 6 Jul 2017 20:23:35 +0000 (16:23 -0400)
committerGitHub <noreply@github.com>
Thu, 6 Jul 2017 20:23:35 +0000 (16:23 -0400)
Instead of using fragile lockless code in the sigchld signal handler, wake up the finalizer thread and make it call back
into the process code to signal finished processes.

mono/metadata/gc.c
mono/metadata/w32process-unix.c
mono/metadata/w32process-win32.c
mono/metadata/w32process.h

index f561dc15b4cb01fd6956f23df5c277339b0fdd08..ab983e7be53b994d893a55407cb72f67a97ade11 100644 (file)
@@ -32,6 +32,7 @@
 #include <mono/metadata/marshal.h> /* for mono_delegate_free_ftnptr () */
 #include <mono/metadata/attach.h>
 #include <mono/metadata/console-io.h>
+#include <mono/metadata/w32process.h>
 #include <mono/utils/mono-os-semaphore.h>
 #include <mono/utils/mono-memory-model.h>
 #include <mono/utils/mono-counters.h>
@@ -707,6 +708,7 @@ static volatile gboolean finished;
  *
  *   Notify the finalizer thread that finalizers etc.
  * are available to be processed.
+ * This is async signal safe.
  */
 void
 mono_gc_finalize_notify (void)
@@ -886,6 +888,8 @@ finalizer_thread (gpointer unused)
 
                reference_queue_proccess_all ();
 
+               mono_w32process_signal_finished ();
+
                hazard_free_queue_pump ();
 
                /* Avoid posting the pending done event until there are pending finalizers */
index 972942af2b60817e509e536df483ad7b5015cc99..57340e9ed5ae5420cc572ab06bc88cfee7d0150d 100644 (file)
@@ -130,9 +130,7 @@ typedef struct {
 /*
  * Process describes processes we create.
  * It contains a semaphore that can be waited on in order to wait
- * for process termination. It's accessed in our SIGCHLD handler,
- * when status is updated (and pid cleared, to not clash with
- * subsequent processes that may get executed).
+ * for process termination.
  */
 typedef struct _Process {
        pid_t pid; /* the pid of the process. This value is only valid until the process has exited. */
@@ -143,7 +141,6 @@ typedef struct _Process {
         * the process has exited, so that the information there isn't lost.
         */
        gpointer handle;
-       gboolean freeable;
        gboolean signalled;
        struct _Process *next;
 } Process;
@@ -530,18 +527,6 @@ static mono_lazy_init_t process_sig_chld_once = MONO_LAZY_INIT_STATUS_NOT_INITIA
 
 static gchar *cli_launcher;
 
-/* The signal-safe logic to use processes goes like this:
- * - The list must be safe to traverse for the signal handler at all times.
- *   It's safe to: prepend an entry (which is a single store to 'processes'),
- *   unlink an entry (assuming the unlinked entry isn't freed and doesn't
- *   change its 'next' pointer so that it can still be traversed).
- * When cleaning up we first unlink an entry, then we verify that
- * the read lock isn't locked. Then we can free the entry, since
- * we know that nobody is using the old version of the list (including
- * the unlinked entry).
- * We also need to lock when adding and cleaning up so that those two
- * operations don't mess with eachother. (This lock is not used in the
- * signal handler) */
 static Process *processes;
 static mono_mutex_t processes_mutex;
 
@@ -723,8 +708,6 @@ processes_cleanup (void)
        static gint32 cleaning_up;
        Process *process;
        Process *prev = NULL;
-       GSList *finished = NULL;
-       GSList *l;
 
        mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s", __func__);
 
@@ -732,6 +715,9 @@ processes_cleanup (void)
        if (InterlockedCompareExchange (&cleaning_up, 1, 0) != 0)
                return;
 
+       /*
+        * This needs to be done outside the lock but atomically, hence the CAS above.
+        */
        for (process = processes; process; process = process->next) {
                if (process->signalled && process->handle) {
                        /* This process has exited and we need to remove the artifical ref
@@ -741,45 +727,27 @@ processes_cleanup (void)
                }
        }
 
-       /*
-        * Remove processes which exited from the processes list.
-        * We need to synchronize with the sigchld handler here, which runs
-        * asynchronously. The handler requires that the processes list
-        * remain valid.
-        */
        mono_os_mutex_lock (&processes_mutex);
 
-       for (process = processes; process; process = process->next) {
-               if (process->handle_count == 0 && process->freeable) {
+       for (process = processes; process;) {
+               Process *next = process->next;
+               if (process->handle_count == 0 && process->signalled) {
                        /*
                         * Unlink the entry.
-                        * This code can run parallel with the sigchld handler, but the
-                        * modifications it makes are safe.
                         */
                        if (process == processes)
                                processes = process->next;
                        else
                                prev->next = process->next;
-                       finished = g_slist_prepend (finished, process);
+
+                       mono_os_sem_destroy (&process->exit_sem);
+                       g_free (process);
                } else {
                        prev = process;
                }
+               process = next;
        }
 
-       mono_memory_barrier ();
-
-       for (l = finished; l; l = l->next) {
-               /*
-                * All the entries in the finished list are unlinked from processes, and
-                * they have the 'finished' flag set, which means the sigchld handler is done
-                * accessing them.
-                */
-               process = (Process *)l->data;
-               mono_os_sem_destroy (&process->exit_sem);
-               g_free (process);
-       }
-       g_slist_free (finished);
-
        mono_os_mutex_unlock (&processes_mutex);
 
        mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "%s done", __func__);
@@ -1359,6 +1327,39 @@ switch_dir_separators (char *path)
 #if HAVE_SIGACTION
 
 MONO_SIGNAL_HANDLER_FUNC (static, mono_sigchld_signal_handler, (int _dummy, siginfo_t *info, void *context))
+{
+       /*
+        * Don't want to do any complicated processing here so just wake up the finalizer thread which will call
+        * mono_w32process_signal_finished ().
+        */
+       int old_errno = errno;
+
+       mono_gc_finalize_notify ();
+
+       errno = old_errno;
+}
+
+static void
+process_add_sigchld_handler (void)
+{
+       struct sigaction sa;
+
+       sa.sa_sigaction = mono_sigchld_signal_handler;
+       sigemptyset (&sa.sa_mask);
+       sa.sa_flags = SA_NOCLDSTOP | SA_SIGINFO | SA_RESTART;
+       g_assert (sigaction (SIGCHLD, &sa, NULL) != -1);
+       mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "Added SIGCHLD handler");
+}
+
+#endif
+
+/*
+ * mono_w32process_signal_finished:
+ *
+ *   Signal the exit semaphore for processes which have finished.
+ */
+void
+mono_w32process_signal_finished (void)
 {
        int status;
        int pid;
@@ -1372,9 +1373,8 @@ MONO_SIGNAL_HANDLER_FUNC (static, mono_sigchld_signal_handler, (int _dummy, sigi
                if (pid <= 0)
                        break;
 
-               /*
-                * This can run concurrently with the code in the rest of this module.
-                */
+               mono_os_mutex_lock (&processes_mutex);
+
                for (process = processes; process; process = process->next) {
                        if (process->pid != pid)
                                continue;
@@ -1384,28 +1384,13 @@ MONO_SIGNAL_HANDLER_FUNC (static, mono_sigchld_signal_handler, (int _dummy, sigi
                        process->signalled = TRUE;
                        process->status = status;
                        mono_os_sem_post (&process->exit_sem);
-                       mono_memory_barrier ();
-                       /* Mark this as freeable, the pointer becomes invalid afterwards */
-                       process->freeable = TRUE;
                        break;
                }
-       } while (1);
-}
 
-static void
-process_add_sigchld_handler (void)
-{
-       struct sigaction sa;
-
-       sa.sa_sigaction = mono_sigchld_signal_handler;
-       sigemptyset (&sa.sa_mask);
-       sa.sa_flags = SA_NOCLDSTOP | SA_SIGINFO;
-       g_assert (sigaction (SIGCHLD, &sa, NULL) != -1);
-       mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_IO_LAYER, "Added SIGCHLD handler");
+               mono_os_mutex_unlock (&processes_mutex);
+       } while (1);
 }
 
-#endif
-
 static gboolean
 is_readable_or_executable (const char *prog)
 {
index 53cf95bc358e93a57c23737f7da1b6cb0d82388c..24fb97ce4d499e116c6361dd080ee65cc5611fc0 100644 (file)
@@ -48,6 +48,11 @@ mono_w32process_cleanup (void)
 {
 }
 
+void
+mono_w32process_signal_finished (void)
+{
+}
+
 #if G_HAVE_API_SUPPORT(HAVE_CLASSIC_WINAPI_SUPPORT)
 HANDLE
 ves_icall_System_Diagnostics_Process_GetProcess_internal (guint32 pid)
index 7a33f2a6c6b69271eaaed8f707322b2c0e8da770..0565ac1eb684291504ba3ba1269c154930c7b86a 100644 (file)
@@ -76,6 +76,9 @@ mono_w32process_init (void);
 void
 mono_w32process_cleanup (void);
 
+void
+mono_w32process_signal_finished (void);
+
 #ifndef HOST_WIN32
 
 void