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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-01-21 at 14:40 +0000, Ian Jackson wrote:
> 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>

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

Including if you want to fold this into another patch.

> ---
>  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;



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