Re: [Xen-devel] [PATCH 12/12] libxl: fork: Share SIGCHLD handler amongst ctxs

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [PATCH 12/12] libxl: fork: Share SIGCHLD handler 
amongst ctxs"):
> It's hard to believe but that seems to be the only comment I have, I
> think I actually grokked the whole locking via SIGCHLD deferral thing
> and it seems to make sense.
> 
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

Following a pub conversation, and a conversation with a friend of mine
last night over a glass of port, I think this fixup patch needs to be
applied too.

Jim, I'm going to look at your crash now.

Ian.

From 7a593aa6903f4a2e3b927e546da32582184843f5 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Date: Tue, 21 Jan 2014 14:22:41 +0000
Subject: [PATCH] libxl: fork: Fixup SIGCHLD sharing

* Use a mutex for defer_sigchld, to guard against concurrency between
  the thread calling defer_sigchld and an instance of the primary
  signal handler on another thread.

* libxl_sigchld_owner_libxl_always is incompatible with SIGCHLD sharing.
  Document this correctly.

Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
---
 tools/libxl/libxl_event.h |   14 ++++++++------
 tools/libxl/libxl_fork.c  |   26 ++++++++++++++++++++++++--
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
index f0703f6..ca43cb9 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -470,16 +470,18 @@ void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void 
*for_libxl)
  *
  *     libxl_sigchld_owner_libxl_always:
  *
- *       The application expects libxl to reap all of its children,
- *       and provides a callback to be notified of their exit
- *       statuses.  The application may have multiple libxl_ctxs
- *       configured this way.
+ *       The application expects this libxl ctx to reap all of the
+ *       process's children, and provides a callback to be notified of
+ *       their exit statuses.  The application must have only one
+ *       libxl_ctx configured this way.
  *
  *     libxl_sigchld_owner_libxl_always_selective_reap:
  *
  *       The application expects to reap all of its own children
- *       synchronously, and does not use SIGCHLD.  libxl is
- *       to install a SIGCHLD handler.
+ *       synchronously, and does not use SIGCHLD.  libxl is to install
+ *       a SIGCHLD handler.  The application may have multiple
+ *       libxl_ctxs configured this way; in which case all of its ctxs
+ *       must be so configured.
  */
 
 
diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
index 5b42bad..2432512 100644
--- a/tools/libxl/libxl_fork.c
+++ b/tools/libxl/libxl_fork.c
@@ -51,6 +51,7 @@ static LIBXL_LIST_HEAD(, libxl__carefd) carefds =
  * asynchronously by the signal handler) by sigchld_defer (see
  * below). */
 static bool sigchld_installed; /* 0 means not */
+static pthread_mutex_t sigchld_defer_mutex = PTHREAD_MUTEX_INITIALIZER;
 static LIBXL_LIST_HEAD(, libxl_ctx) sigchld_users =
     LIBXL_LIST_HEAD_INITIALIZER(sigchld_users);
 static struct sigaction sigchld_saved_action;
@@ -188,11 +189,17 @@ static void sigchld_handler(int signo)
     libxl_ctx *notify;
     int esave = errno;
 
+    int r = pthread_mutex_lock(&sigchld_defer_mutex);
+    assert(!r);
+
     LIBXL_LIST_FOREACH(notify, &sigchld_users, sigchld_users_entry) {
         int e = libxl__self_pipe_wakeup(notify->sigchld_selfpipe[1]);
         assert(!e); /* errors are probably EBADF, very bad */
     }
 
+    r = pthread_mutex_unlock(&sigchld_defer_mutex);
+    assert(!r);
+
     errno = esave;
 }
 
@@ -221,8 +228,10 @@ static void sigchld_sethandler_raw(void (*handler)(int), 
struct sigaction *old)
  * must be reentrant (see the comment in release_sigchld).
  *
  * Callers have the atfork_lock so there is no risk of concurrency
- * within these functions, aside obviously from the risk of being
- * interrupted by the signal.
+ * within these functions, aside from the risk of being interrupted by
+ * the signal.  We use sigchld_defer_mutex to guard against the
+ * possibility of the real signal handler being still running on
+ * another thread.
  */
 
 static volatile sig_atomic_t sigchld_occurred_while_deferred;
@@ -235,12 +244,25 @@ static void sigchld_handler_when_deferred(int signo)
 static void defer_sigchld(void)
 {
     assert(sigchld_installed);
+
     sigchld_sethandler_raw(sigchld_handler_when_deferred, 0);
+
+    /* Now _this thread_ cannot any longer be interrupted by the
+     * signal, so we can take the mutex without risk of deadlock.  If
+     * another thread is in the signal handler, either it or we will
+     * block and wait for the other. */
+
+    int r = pthread_mutex_lock(&sigchld_defer_mutex);
+    assert(!r);
 }
 
 static void release_sigchld(void)
 {
     assert(sigchld_installed);
+
+    int r = pthread_mutex_unlock(&sigchld_defer_mutex);
+    assert(!r);
+
     sigchld_sethandler_raw(sigchld_handler, 0);
     if (sigchld_occurred_while_deferred) {
         sigchld_occurred_while_deferred = 0;
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel