[Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Alex Barcelo]

This series of patches implements coroutines method with
sigaltstack.

The flow of creation and management of the coroutines is
quite similar to the coroutine-ucontext.c. The way to use
sigaltstack to achieve the needed stack manipulation is
done in a way quite similar to the GNU Portable Threads
(file pth_mctx.c, variant 2).

It's my first patch, I'm sure that there are things that I
have done wrong. Please, be kind :)

Thanks for your time

Alex Barcelo (3):
  coroutine: adding sigaltstack method (.c source)
  coroutine: adding control flags (enable/disable) for ucontext
    compilation
  coroutine: adding enable/disable options for sigaltstack method

 Makefile.objs           |    4 +
 configure               |   63 +++++++++-
 coroutine-sigaltstack.c |  337 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 401 insertions(+), 3 deletions(-)
 create mode 100644 coroutine-sigaltstack.c

-- 
1.7.5.4

[Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Alex Barcelo]

This file is based in both coroutine-ucontext.c and
pth_mctx.c (from the GNU Portable Threads library).

The mechanism used to change stacks is the sigaltstack
function (variant 2 of the pth library).

Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
---
 coroutine-sigaltstack.c |  337 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 337 insertions(+), 0 deletions(-)
 create mode 100644 coroutine-sigaltstack.c

diff --git a/coroutine-sigaltstack.c b/coroutine-sigaltstack.c
new file mode 100644
index 0000000..1d4f26d
--- /dev/null
+++ b/coroutine-sigaltstack.c
@@ -0,0 +1,337 @@
+/*
+ * sigaltstack coroutine initialization code
+ *
+ * Copyright (C) 2006  Anthony Liguori <anthony@codemonkey.ws>
+ * Copyright (C) 2011  Kevin Wolf <kwolf@redhat.com>
+ * Copyright (C) 2012  Alex Barcelo <abarcelo@ac.upc.edu>
+ *
+ * This 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.0 of the License, or (at your option) any later version.
+ *
+ * This 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 this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+** This file is partly based on pth_mctx.c, from the GNU Portable Threads
+**  Copyright (c) 1999-2006 Ralf S. Engelschall <rse@engelschall.com>
+**  Same license (version 2.1 or later)
+*/
+
+/* XXX Is there a nicer way to disable glibc's stack check for longjmp? */
+#ifdef _FORTIFY_SOURCE
+#undef _FORTIFY_SOURCE
+#endif
+#include <stdlib.h>
+#include <setjmp.h>
+#include <stdint.h>
+#include <pthread.h>
+#include <signal.h>
+#include "qemu-common.h"
+#include "qemu-coroutine-int.h"
+
+enum {
+    /* Maximum free pool size prevents holding too many freed coroutines */
+    POOL_MAX_SIZE = 64,
+};
+
+/** Free list to speed up creation */
+static QLIST_HEAD(, Coroutine) pool = QLIST_HEAD_INITIALIZER(pool);
+static unsigned int pool_size;
+
+typedef struct {
+    Coroutine base;
+    void *stack;
+    jmp_buf env;
+} CoroutineUContext;
+
+/**
+ * Per-thread coroutine bookkeeping
+ */
+typedef struct {
+    /** Currently executing coroutine */
+    Coroutine *current;
+
+    /** The default coroutine */
+    CoroutineUContext leader;
+} CoroutineThreadState;
+
+static pthread_key_t thread_state_key;
+
+/*
+ * the way to pass information to the signal handler (trampoline)
+ * It's not thread-safe, as can be seen, but there is no other simple way.
+ */
+static volatile jmp_buf      tr_reenter;
+static volatile sig_atomic_t tr_called;
+static void *ptr_for_handler;
+
+static CoroutineThreadState *coroutine_get_thread_state(void)
+{
+    CoroutineThreadState *s = pthread_getspecific(thread_state_key);
+
+    if (!s) {
+        s = g_malloc0(sizeof(*s));
+        s->current = &s->leader.base;
+        pthread_setspecific(thread_state_key, s);
+    }
+    return s;
+}
+
+static void qemu_coroutine_thread_cleanup(void *opaque)
+{
+    CoroutineThreadState *s = opaque;
+
+    g_free(s);
+}
+
+static void __attribute__((destructor)) coroutine_cleanup(void)
+{
+    Coroutine *co;
+    Coroutine *tmp;
+
+    QLIST_FOREACH_SAFE(co, &pool, pool_next, tmp) {
+        g_free(DO_UPCAST(CoroutineUContext, base, co)->stack);
+        g_free(co);
+    }
+}
+
+static void __attribute__((constructor)) coroutine_init(void)
+{
+    int ret;
+
+    ret = pthread_key_create(&thread_state_key, qemu_coroutine_thread_cleanup);
+    if (ret != 0) {
+        fprintf(stderr, "unable to create leader key: %s\n", strerror(errno));
+        abort();
+    }
+}
+
+/* "boot" function
+ * This is what starts the coroutine, is called from the trampoline
+ * (from the signal handler when it is not signal handling, read ahead
+ * for more information).
+ */
+static void coroutine_bootstrap(CoroutineUContext *self, Coroutine *co)
+{
+    /* Initialize longjmp environment and switch back the caller */
+    if (!setjmp(self->env)) {
+        longjmp(*(jmp_buf *)co->entry_arg, 1);
+    }
+
+    while (true) {
+        co->entry(co->entry_arg);
+        qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
+    }
+}
+
+/*
+ * This is used as the signal handler. This is called with the brand new stack
+ * (thanks to sigaltstack). We have to return, given that this is a signal
+ * handler and the sigmask and some other things are changed.
+ */
+static void coroutine_trampoline(int signal)
+{
+    CoroutineUContext *self;
+    Coroutine *co;
+
+    /* This will break on multithread or in any race condition */
+    self = ptr_for_handler;
+    tr_called = 1;
+    co = &self->base;
+
+    /*
+     * Here we have to do a bit of a ping pong between the caller, given that
+     * this is a signal handler and we have to do a return "soon". Then the
+     * caller can reestablish everything and do a longjmp here again.
+     */
+    if (!setjmp(*((jmp_buf *)&tr_reenter))) {
+        return;
+    }
+
+    /*
+     * Ok, the caller has longjmp'ed back to us, so now prepare
+     * us for the real machine state switching. We have to jump
+     * into another function here to get a new stack context for
+     * the auto variables (which have to be auto-variables
+     * because the start of the thread happens later). Else with
+     * PIC (i.e. Position Independent Code which is used when PTH
+     * is built as a shared library) most platforms would
+     * horrible core dump as experience showed.
+     */
+    coroutine_bootstrap(self, co);
+}
+
+static Coroutine *coroutine_new(void)
+{
+    const size_t stack_size = 1 << 20;
+    CoroutineUContext *co;
+    struct sigaction sa;
+    struct sigaction osa;
+    struct sigaltstack ss;
+    struct sigaltstack oss;
+    sigset_t sigs;
+    sigset_t osigs;
+    jmp_buf old_env;
+
+    /* The way to manipulate stack is with the sigaltstack function. We
+     * prepare a stack, with it delivering a signal to ourselves and then
+     * put setjmp/longjmp where needed.
+     * This has been done keeping coroutine-ucontext as a model and with the
+     * pth ideas (GNU Portable Threads). See coroutine-ucontext for the basics
+     * of the coroutines and see pth_mctx.c (from the pth project) for the
+     * sigaltstack way of manipulating stacks.
+     */
+
+    co = g_malloc0(sizeof(*co));
+    co->stack = g_malloc(stack_size);
+    co->base.entry_arg = &old_env; /* stash away our jmp_buf */
+
+    ptr_for_handler = co;
+
+    /*
+     * Preserve the SIGUSR1 signal state, block SIGUSR1,
+     * and establish our signal handler. The signal will
+     * later transfer control onto the signal stack.
+     */
+    sigemptyset(&sigs);
+    sigaddset(&sigs, SIGUSR1);
+    sigprocmask(SIG_BLOCK, &sigs, &osigs);
+    sa.sa_handler = coroutine_trampoline;
+    sigfillset(&sa.sa_mask);
+    sa.sa_flags = SA_ONSTACK;
+    if (sigaction(SIGUSR1, &sa, &osa) != 0) {
+        abort();
+    }
+
+    /*
+     * Set the new stack.
+     */
+    ss.ss_sp = co->stack;
+    ss.ss_size = stack_size;
+    ss.ss_flags = 0;
+    if (sigaltstack(&ss, &oss) < 0) {
+        abort();
+    }
+
+    /*
+     * Now transfer control onto the signal stack and set it up.
+     * It will return immediately via "return" after the setjmp()
+     * was performed. Be careful here with race conditions.  The
+     * signal can be delivered the first time sigsuspend() is
+     * called.
+     */
+    tr_called = 0;
+    kill(getpid(), SIGUSR1);
+    sigfillset(&sigs);
+    sigdelset(&sigs, SIGUSR1);
+    while (!tr_called) {
+        sigsuspend(&sigs);
+    }
+
+    /*
+     * Inform the system that we are back off the signal stack by
+     * removing the alternative signal stack. Be careful here: It
+     * first has to be disabled, before it can be removed.
+     */
+    sigaltstack(NULL, &ss);
+    ss.ss_flags = SS_DISABLE;
+    if (sigaltstack(&ss, NULL) < 0) {
+        abort();
+    }
+    sigaltstack(NULL, &ss);
+    if (!(oss.ss_flags & SS_DISABLE)) {
+        sigaltstack(&oss, NULL);
+    }
+
+    /*
+     * Restore the old SIGUSR1 signal handler and mask
+     */
+    sigaction(SIGUSR1, &osa, NULL);
+    sigprocmask(SIG_SETMASK, &osigs, NULL);
+
+    /*
+     * Now enter the trampoline again, but this time not as a signal
+     * handler. Instead we jump into it directly. The functionally
+     * redundant ping-pong pointer arithmentic is neccessary to avoid
+     * type-conversion warnings related to the `volatile' qualifier and
+     * the fact that `jmp_buf' usually is an array type.
+     */
+    if (!setjmp(old_env)) {
+        longjmp(*((jmp_buf *)&tr_reenter), 1);
+    }
+
+    /*
+     * Ok, we returned again, so now we're finished
+     */
+
+    return &co->base;
+}
+
+Coroutine *qemu_coroutine_new(void)
+{
+    Coroutine *co;
+
+    co = QLIST_FIRST(&pool);
+    if (co) {
+        QLIST_REMOVE(co, pool_next);
+        pool_size--;
+    } else {
+        co = coroutine_new();
+    }
+    return co;
+}
+
+void qemu_coroutine_delete(Coroutine *co_)
+{
+    CoroutineUContext *co = DO_UPCAST(CoroutineUContext, base, co_);
+
+    if (pool_size < POOL_MAX_SIZE) {
+        QLIST_INSERT_HEAD(&pool, &co->base, pool_next);
+        co->base.caller = NULL;
+        pool_size++;
+        return;
+    }
+
+    g_free(co->stack);
+    g_free(co);
+}
+
+CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
+                                      CoroutineAction action)
+{
+    CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
+    CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
+    CoroutineThreadState *s = coroutine_get_thread_state();
+    int ret;
+
+    s->current = to_;
+
+    ret = setjmp(from->env);
+    if (ret == 0) {
+        longjmp(to->env, action);
+    }
+    return ret;
+}
+
+Coroutine *qemu_coroutine_self(void)
+{
+    CoroutineThreadState *s = coroutine_get_thread_state();
+
+    return s->current;
+}
+
+bool qemu_in_coroutine(void)
+{
+    CoroutineThreadState *s = pthread_getspecific(thread_state_key);
+
+    return s && s->current->caller;
+}
+
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/3] coroutine: adding control flags (enable/disable) for ucontext compilation

[Alex Barcelo]

Configure tries, as a default, ucontext functions for the
coroutines. But now the user can force its use or disable
it at all (enable and disable flags)

Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
---
 configure |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index 763db24..ed40da8 100755
--- a/configure
+++ b/configure
@@ -190,6 +190,7 @@ opengl=""
 zlib="yes"
 guest_agent="yes"
 libiscsi=""
+ucontext=""
 
 # parse CC options first
 for opt do
@@ -798,6 +799,10 @@ for opt do
   ;;
   --disable-guest-agent) guest_agent="no"
   ;;
+  --enable-ucontext) ucontext="yes"
+  ;;
+  --disable-ucontext) ucontext="no"
+  ;;
   *) echo "ERROR: unknown option $opt"; show_help="yes"
   ;;
   esac
@@ -1084,6 +1089,8 @@ echo "  --disable-usb-redir      disable usb network redirection support"
 echo "  --enable-usb-redir       enable usb network redirection support"
 echo "  --disable-guest-agent    disable building of the QEMU Guest Agent"
 echo "  --enable-guest-agent     enable building of the QEMU Guest Agent"
+echo "  --disable-ucontext       disable ucontext functions for coroutines"
+echo "  --enable-ucontext        enable ucontext functions for coroutines"
 echo ""
 echo "NOTE: The object files are built at the place where configure is launched"
 exit 1
@@ -2717,13 +2724,23 @@ fi
 # check if we have makecontext
 
 ucontext_coroutine=no
-if test "$darwin" != "yes"; then
-  cat > $TMPC << EOF
+if test "$ucontext" != "no"; then
+  if test "$darwin" != "yes"; then
+    cat > $TMPC << EOF
 #include <ucontext.h>
 int main(void) { makecontext(0, 0, 0); return 0; }
 EOF
-  if compile_prog "" "" ; then
-      ucontext_coroutine=yes
+    if compile_prog "" "" ; then
+        ucontext_coroutine=yes
+    elif test "$ucontext" = "yes" ; then
+        echo
+        echo "Error: ucontext check failed"
+        echo "Make sure that ucontext.h and its funcionts are supported"
+        echo
+        exit 1
+    fi
+  else
+    echo "Silently ignoring ucontext coroutine method under darwin"
   fi
 fi
 
@@ -2918,6 +2935,7 @@ echo "usb net redir     $usb_redir"
 echo "OpenGL support    $opengl"
 echo "libiscsi support  $libiscsi"
 echo "build guest agent $guest_agent"
+echo "ucontext coroutine support    $ucontext_coroutine"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
-- 
1.7.5.4

[Qemu-devel] [PATCH 3/3] coroutine: adding enable/disable options for sigaltstack method

[Alex Barcelo]

It's possible to enable/disable sigaltstack, but it always has
less priority than ucontext method (to force sigaltstack,
ucontext has to be disabled).

Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
---
 Makefile.objs |    4 ++++
 configure     |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 391e524..8874825 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -17,8 +17,12 @@ coroutine-obj-y += qemu-coroutine-sleep.o
 ifeq ($(CONFIG_UCONTEXT_COROUTINE),y)
 coroutine-obj-$(CONFIG_POSIX) += coroutine-ucontext.o
 else
+ifeq ($(CONFIG_SIGALTSTACK_COROUTINE),y)
+coroutine-obj-$(CONFIG_POSIX) += coroutine-sigaltstack.o
+else
 coroutine-obj-$(CONFIG_POSIX) += coroutine-gthread.o
 endif
+endif
 coroutine-obj-$(CONFIG_WIN32) += coroutine-win32.o
 
 #######################################################################
diff --git a/configure b/configure
index ed40da8..e9c27f3 100755
--- a/configure
+++ b/configure
@@ -191,6 +191,7 @@ zlib="yes"
 guest_agent="yes"
 libiscsi=""
 ucontext=""
+sigaltstack=""
 
 # parse CC options first
 for opt do
@@ -803,6 +804,10 @@ for opt do
   ;;
   --disable-ucontext) ucontext="no"
   ;;
+  --enable-sigaltstack) sigaltstack="yes"
+  ;;
+  --disable-sigaltstack) sigaltstack="no"
+  ;;
   *) echo "ERROR: unknown option $opt"; show_help="yes"
   ;;
   esac
@@ -1091,6 +1096,8 @@ echo "  --disable-guest-agent    disable building of the QEMU Guest Agent"
 echo "  --enable-guest-agent     enable building of the QEMU Guest Agent"
 echo "  --disable-ucontext       disable ucontext functions for coroutines"
 echo "  --enable-ucontext        enable ucontext functions for coroutines"
+echo "  --disable-sigaltstack    disable sigaltstack functions for coroutines"
+echo "  --enable-sigaltstack    enable sigaltstack functions for coroutines"
 echo ""
 echo "NOTE: The object files are built at the place where configure is launched"
 exit 1
@@ -2745,6 +2752,33 @@ EOF
 fi
 
 ##########################################
+# check, if there is no ucontext, for
+# sigaltstack
+
+sigaltstack_coroutine=no
+if test "$ucontext_coroutine" = "no" -a "$sigaltstack" != "no" ; then
+  cat > $TMPC << EOF
+#include <signal.h>
+int main(void) {
+  stack_t ss;
+  ss.ss_size = SIGSTKSZ;
+  ss.ss_flags = 0;
+  sigaltstack(&ss, 0);
+}
+EOF
+  if compile_prog "" "" ; then
+    sigaltstack_coroutine=yes
+  elif test "$sigaltstack" = "yes"; then
+    echo
+    echo "Error: sigaltstack check failed"
+    echo "Make sure that sigaltstack is supported"
+    echo
+    exit 1
+  fi
+fi
+
+
+##########################################
 # check if we have open_by_handle_at
 
 open_by_hande_at=no
@@ -2936,6 +2970,7 @@ echo "OpenGL support    $opengl"
 echo "libiscsi support  $libiscsi"
 echo "build guest agent $guest_agent"
 echo "ucontext coroutine support    $ucontext_coroutine"
+echo "sigaltstack coroutine support $sigaltstack_coroutine"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -3259,6 +3294,10 @@ if test "$ucontext_coroutine" = "yes" ; then
   echo "CONFIG_UCONTEXT_COROUTINE=y" >> $config_host_mak
 fi
 
+if test "$sigaltstack_coroutine" = "yes" ; then
+  echo "CONFIG_SIGALTSTACK_COROUTINE=y" >> $config_host_mak
+fi
+
 if test "$open_by_handle_at" = "yes" ; then
   echo "CONFIG_OPEN_BY_HANDLE=y" >> $config_host_mak
 fi
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 3/3] coroutine: adding enable/disable options for sigaltstack method

[Daniel P. Berrange]

On Mon, Feb 13, 2012 at 03:42:30PM +0100, Alex Barcelo wrote:
> It's possible to enable/disable sigaltstack, but it always has
> less priority than ucontext method (to force sigaltstack,
> ucontext has to be disabled).
> 
> Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
> ---
>  Makefile.objs |    4 ++++
>  configure     |   39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 391e524..8874825 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -17,8 +17,12 @@ coroutine-obj-y += qemu-coroutine-sleep.o
>  ifeq ($(CONFIG_UCONTEXT_COROUTINE),y)
>  coroutine-obj-$(CONFIG_POSIX) += coroutine-ucontext.o
>  else
> +ifeq ($(CONFIG_SIGALTSTACK_COROUTINE),y)
> +coroutine-obj-$(CONFIG_POSIX) += coroutine-sigaltstack.o
> +else
>  coroutine-obj-$(CONFIG_POSIX) += coroutine-gthread.o
>  endif
> +endif
>  coroutine-obj-$(CONFIG_WIN32) += coroutine-win32.o
>  
>  #######################################################################
> diff --git a/configure b/configure
> index ed40da8..e9c27f3 100755
> --- a/configure
> +++ b/configure
> @@ -191,6 +191,7 @@ zlib="yes"
>  guest_agent="yes"
>  libiscsi=""
>  ucontext=""
> +sigaltstack=""
>  
>  # parse CC options first
>  for opt do
> @@ -803,6 +804,10 @@ for opt do
>    ;;
>    --disable-ucontext) ucontext="no"
>    ;;
> +  --enable-sigaltstack) sigaltstack="yes"
> +  ;;
> +  --disable-sigaltstack) sigaltstack="no"
> +  ;;
>    *) echo "ERROR: unknown option $opt"; show_help="yes"
>    ;;
>    esac
> @@ -1091,6 +1096,8 @@ echo "  --disable-guest-agent    disable building of the QEMU Guest Agent"
>  echo "  --enable-guest-agent     enable building of the QEMU Guest Agent"
>  echo "  --disable-ucontext       disable ucontext functions for coroutines"
>  echo "  --enable-ucontext        enable ucontext functions for coroutines"
> +echo "  --disable-sigaltstack    disable sigaltstack functions for coroutines"
> +echo "  --enable-sigaltstack    enable sigaltstack functions for coroutines"

Since the 3 different coroutine impls are mutually exclusive
choices, perhaps it'd be preferable to just have a single
configure argument like

   --with-couroutines=[ucontext|sigaltstack|gthread]

Thus avoiding the non-sensical scenario of the user specifying

   --enable-ucontext --enable-sigaltstack

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Peter Maydell]

On 13 February 2012 14:42, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> This series of patches implements coroutines method with
> sigaltstack.
>
> The flow of creation and management of the coroutines is
> quite similar to the coroutine-ucontext.c. The way to use
> sigaltstack to achieve the needed stack manipulation is
> done in a way quite similar to the GNU Portable Threads
> (file pth_mctx.c, variant 2).

So the obvious question here is why this should be a new
coroutine method rather than just replacing the ucontext one.
Having a tricky bit of code like the coroutine implementation
have multiple implementations is asking for the less-used
ones to bitrot, have undetected race conditions, etc. I would
much prefer it if we could have one standard implementation
that was used on all (unixy) platforms.

The ucontext implementation is problematic because makecontext
&co aren't implemented on all platforms (ARM Linux, and I think
at least one of the BSDs?). Is this sigaltstack approach
workable on a strict superset of the platforms that would
be able to use ucontext? Does it have any disadvantages that
would mean you wouldn't want to use it as a first choice
if you had ucontext?

-- PMM

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Paolo Bonzini]

On 02/13/2012 03:42 PM, Alex Barcelo wrote:
> This file is based in both coroutine-ucontext.c and
> pth_mctx.c (from the GNU Portable Threads library).
>
> The mechanism used to change stacks is the sigaltstack
> function (variant 2 of the pth library).
>
> Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
> ---
>  coroutine-sigaltstack.c |  337 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 337 insertions(+), 0 deletions(-)
>  create mode 100644 coroutine-sigaltstack.c
>
> diff --git a/coroutine-sigaltstack.c b/coroutine-sigaltstack.c
> new file mode 100644
> index 0000000..1d4f26d
> --- /dev/null
> +++ b/coroutine-sigaltstack.c
> @@ -0,0 +1,337 @@
> +/*
> + * sigaltstack coroutine initialization code
> + *
> + * Copyright (C) 2006  Anthony Liguori <anthony@codemonkey.ws>
> + * Copyright (C) 2011  Kevin Wolf <kwolf@redhat.com>
> + * Copyright (C) 2012  Alex Barcelo <abarcelo@ac.upc.edu>
> + *
> + * This 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.0 of the License, or (at your option) any later version.
> + *
> + * This 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 this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> +** This file is partly based on pth_mctx.c, from the GNU Portable Threads
> +**  Copyright (c) 1999-2006 Ralf S. Engelschall <rse@engelschall.com>
> +**  Same license (version 2.1 or later)
> +*/
> +
> +/* XXX Is there a nicer way to disable glibc's stack check for longjmp? */
> +#ifdef _FORTIFY_SOURCE
> +#undef _FORTIFY_SOURCE
> +#endif
> +#include <stdlib.h>
> +#include <setjmp.h>
> +#include <stdint.h>
> +#include <pthread.h>
> +#include <signal.h>
> +#include "qemu-common.h"
> +#include "qemu-coroutine-int.h"
> +
> +enum {
> +    /* Maximum free pool size prevents holding too many freed coroutines */
> +    POOL_MAX_SIZE = 64,
> +};
> +
> +/** Free list to speed up creation */
> +static QLIST_HEAD(, Coroutine) pool = QLIST_HEAD_INITIALIZER(pool);
> +static unsigned int pool_size;
> +
> +typedef struct {
> +    Coroutine base;
> +    void *stack;
> +    jmp_buf env;
> +} CoroutineUContext;
> +
> +/**
> + * Per-thread coroutine bookkeeping
> + */
> +typedef struct {
> +    /** Currently executing coroutine */
> +    Coroutine *current;
> +
> +    /** The default coroutine */
> +    CoroutineUContext leader;
> +} CoroutineThreadState;
> +
> +static pthread_key_t thread_state_key;
> +
> +/*
> + * the way to pass information to the signal handler (trampoline)
> + * It's not thread-safe, as can be seen, but there is no other simple way.
> + */
> +static volatile jmp_buf      tr_reenter;
> +static volatile sig_atomic_t tr_called;

Unlike pth, we can assume thread-local storage:   these should be placed 
in CoroutineThreadState and coroutine_get_thread_state() used to access 
them.

> +    /*
> +     * Preserve the SIGUSR1 signal state, block SIGUSR1,
> +     * and establish our signal handler. The signal will
> +     * later transfer control onto the signal stack.
> +     */

We're already using SIGUSR1.  Can you switch to SIGUSR2?

> +    sigemptyset(&sigs);
> +    sigaddset(&sigs, SIGUSR1);
> +    sigprocmask(SIG_BLOCK, &sigs, &osigs);

This should be pthread_sigmask.

> +    /*
> +     * Restore the old SIGUSR1 signal handler and mask
> +     */
> +    sigaction(SIGUSR1, &osa, NULL);
> +    sigprocmask(SIG_SETMASK, &osigs, NULL);
> +
> +    /*
> +     * Now enter the trampoline again, but this time not as a signal
> +     * handler. Instead we jump into it directly. The functionally
> +     * redundant ping-pong pointer arithmentic is neccessary to avoid
> +     * type-conversion warnings related to the `volatile' qualifier and
> +     * the fact that `jmp_buf' usually is an array type.
> +     */
> +    if (!setjmp(old_env)) {
> +        longjmp(*((jmp_buf *)&tr_reenter), 1);
> +    }

Use thread-local storage and you'll be able to remove this ugliness,
too.

Overall it looks good, however I think if it is good it should replace 
coroutine-ucontext.c altogether.  Other thoughts?

Paolo

Re: [Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Alex Barcelo]

On Mon, Feb 13, 2012 at 15:51, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 February 2012 14:42, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>> This series of patches implements coroutines method with
>> sigaltstack.
>>
>> The flow of creation and management of the coroutines is
>> quite similar to the coroutine-ucontext.c. The way to use
>> sigaltstack to achieve the needed stack manipulation is
>> done in a way quite similar to the GNU Portable Threads
>> (file pth_mctx.c, variant 2).
>
> So the obvious question here is why this should be a new
> coroutine method rather than just replacing the ucontext one.

Well, you are right. I did this because I needed something for an
environment which doesn't have ucontext support[1] (and the fallback
was awful).

> Having a tricky bit of code like the coroutine implementation
> have multiple implementations is asking for the less-used
> ones to bitrot, have undetected race conditions, etc. I would
> much prefer it if we could have one standard implementation
> that was used on all (unixy) platforms.

ucontext seems the "modern good way" (as far as I have read)... but
it's not standard enough. And not very multiplatform, as I have seen.

> The ucontext implementation is problematic because makecontext
> &co aren't implemented on all platforms (ARM Linux, and I think
> at least one of the BSDs?). Is this sigaltstack approach
> workable on a strict superset of the platforms that would
> be able to use ucontext? Does it have any disadvantages that
> would mean you wouldn't want to use it as a first choice
> if you had ucontext?

This new implementation... well, it seems to work (I have done an
ubuntu installation with a cdrom and a qcow drive, which seems to use
quite a lot of coroutines). Of course I have done the coroutine-test
and it was OK. But... I wasn't confident enough to propose it as a
"mature alternative". And I don't have any performance benchmark,
which would be interesting. So, I thought that the better option would
be to send this patch to the developers as an alternative to ucontext.

The Portable Threads library use ucontext as the first variant, and
then sigaltstack as a fallback. Their comment (not sure if it's
useful)
/*
 * VARIANT 1: THE STANDARDIZED SVR4/SUSv2 APPROACH
 *
 * This is the preferred variant, because it uses the standardized
 * SVR4/SUSv2 makecontext(2) and friends which is a facility intended
 * for user-space context switching. The thread creation therefore is
 * straight-foreward.
 */

/*
 * VARIANT 2: THE SIGNAL STACK TRICK
 *
 * ...
 * The ingenious fact is that this variant runs really on _all_ POSIX
 * compliant systems without special platform kludges.  But be _VERY_
 * carefully when you change something in the following code. The slightest
 * change or reordering can lead to horribly broken code.  Really every
 * function call in the following case is intended to be how it is, doubt
 * me...
 *
 * For more details we strongly recommend you to read the companion
 * paper ``Portable Multithreading -- The Signal Stack Trick for
 * User-Space Thread Creation'' from Ralf S. Engelschall. A copy of the
 * draft of this paper you can find in the file rse-pmt.ps inside the
 * GNU Pth distribution.
 */

The Pth is capable to use sigstack if no sigaltstack is found in the
host. I have not implemented it, but should be reasonably
straightforward.

[1] In fact, I was trying to run a powerpc-crosscompiled i386-softmmu
inside a qemu-ppc linux-user (on a i386 box). And ppc linux-user
doesn't support the needed syscall for ucontext functions. So it was a
first step.

Re: [Qemu-devel] [PATCH 3/3] coroutine: adding enable/disable options for sigaltstack method

[Alex Barcelo]

On Mon, Feb 13, 2012 at 15:49, Daniel P. Berrange <berrange@redhat.com> wrote:
> Since the 3 different coroutine impls are mutually exclusive
> choices, perhaps it'd be preferable to just have a single
> configure argument like
>
>   --with-couroutines=[ucontext|sigaltstack|gthread]
>
> Thus avoiding the non-sensical scenario of the user specifying
>
>   --enable-ucontext --enable-sigaltstack

Yes. Now that you mention it... it's the natural way. The v2 will be this way :)

Re: [Qemu-devel] [PATCH 2/3] coroutine: adding control flags (enable/disable) for ucontext compilation

[Kevin Wolf]

Am 13.02.2012 15:42, schrieb Alex Barcelo:
> Configure tries, as a default, ucontext functions for the
> coroutines. But now the user can force its use or disable
> it at all (enable and disable flags)
> 
> Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>

I think a better approach would be to have a
--coroutines=[ucontext|sigaltstack|gthread|windows] option, with the
appropriate default whereever it's possible to detect.

This would allow to build with gthread based coroutines even when
another option is available.

Kevin

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Andreas Färber]

Am 13.02.2012 15:42, schrieb Alex Barcelo:
> This file is based in both coroutine-ucontext.c and
> pth_mctx.c (from the GNU Portable Threads library).
> 
> The mechanism used to change stacks is the sigaltstack
> function (variant 2 of the pth library).
> 
> Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
> ---
>  coroutine-sigaltstack.c |  337 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 337 insertions(+), 0 deletions(-)
>  create mode 100644 coroutine-sigaltstack.c
> 
> diff --git a/coroutine-sigaltstack.c b/coroutine-sigaltstack.c
> new file mode 100644
> index 0000000..1d4f26d
> --- /dev/null
> +++ b/coroutine-sigaltstack.c
> @@ -0,0 +1,337 @@
> +/*
> + * sigaltstack coroutine initialization code
> + *
> + * Copyright (C) 2006  Anthony Liguori <anthony@codemonkey.ws>
> + * Copyright (C) 2011  Kevin Wolf <kwolf@redhat.com>
> + * Copyright (C) 2012  Alex Barcelo <abarcelo@ac.upc.edu>
> + *
> + * This 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.0 of the License, or (at your option) any later version.
> + *
> + * This 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 this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> +** This file is partly based on pth_mctx.c, from the GNU Portable Threads
> +**  Copyright (c) 1999-2006 Ralf S. Engelschall <rse@engelschall.com>
> +**  Same license (version 2.1 or later)
> +*/

You should (need to?) use version 2.1 or later above then, too. You can
then simply move this snippet up and drop the "Same license ..." line.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Alex Barcelo]

On Mon, Feb 13, 2012 at 16:57, Andreas Färber <afaerber@suse.de> wrote:
> You should (need to?) use version 2.1 or later above then, too. You can
> then simply move this snippet up and drop the "Same license ..." line.

I wanted to ask this, but it slipped my mind. So it's ok to change the
header to a newer GNU version. I will update it in the PATCH v2.

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Andreas Färber]

Am 13.02.2012 17:11, schrieb Alex Barcelo:
> On Mon, Feb 13, 2012 at 16:57, Andreas Färber <afaerber@suse.de> wrote:
>> You should (need to?) use version 2.1 or later above then, too. You can
>> then simply move this snippet up and drop the "Same license ..." line.
> 
> I wanted to ask this, but it slipped my mind. So it's ok to change the
> header to a newer GNU version. I will update it in the PATCH v2.

IANAL. It looked like you were adding a new file, so you can choose any
Open Source license that's compatible with the code that gets linked
together. Coroutines need to be compatible with parts of QEMU under
GPLv2, so LGPLv2.1 is the newest we can go AFAIU.

Further, there is no GNU Lesser General Public License 2.0, only 2.1:
http://www.gnu.org/licenses/lgpl-2.1.html

2.0 was the GNU Library General Public License:
http://www.gnu.org/licenses/old-licenses/lgpl-2.0.html

So this may even just be a typo somewhere.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Andreas Färber]

Am 13.02.2012 17:31, schrieb Andreas Färber:
> Further, there is no GNU Lesser General Public License 2.0, only 2.1:
> http://www.gnu.org/licenses/lgpl-2.1.html
> 
> 2.0 was the GNU Library General Public License:
> http://www.gnu.org/licenses/old-licenses/lgpl-2.0.html
> 
> So this may even just be a typo somewhere.

Actually FWIW, I noticed that the German version of the license page has
the s/Lesser/Library/ error while the English and Japanese ones do not:

http://www.gnu.org/licenses/old-licenses/lgpl-2.1.de.html

http://www.gnu.org/licenses/old-licenses/lgpl-2.1.en.html
http://www.gnu.org/licenses/old-licenses/lgpl-2.1.ja.html

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Stefan Hajnoczi]

On Mon, Feb 13, 2012 at 04:11:15PM +0100, Alex Barcelo wrote:
> On Mon, Feb 13, 2012 at 15:51, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 13 February 2012 14:42, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> >> This series of patches implements coroutines method with
> >> sigaltstack.
> >>
> >> The flow of creation and management of the coroutines is
> >> quite similar to the coroutine-ucontext.c. The way to use
> >> sigaltstack to achieve the needed stack manipulation is
> >> done in a way quite similar to the GNU Portable Threads
> >> (file pth_mctx.c, variant 2).
> >
> > So the obvious question here is why this should be a new
> > coroutine method rather than just replacing the ucontext one.
> 
> Well, you are right. I did this because I needed something for an
> environment which doesn't have ucontext support[1] (and the fallback
> was awful).

I agree that we should aim to have 1 thing that works really well
instead of multiple things that work okay sometimes.

> > The ucontext implementation is problematic because makecontext
> > &co aren't implemented on all platforms (ARM Linux, and I think
> > at least one of the BSDs?). Is this sigaltstack approach
> > workable on a strict superset of the platforms that would
> > be able to use ucontext? Does it have any disadvantages that
> > would mean you wouldn't want to use it as a first choice
> > if you had ucontext?
> 
> This new implementation... well, it seems to work (I have done an
> ubuntu installation with a cdrom and a qcow drive, which seems to use
> quite a lot of coroutines). Of course I have done the coroutine-test
> and it was OK. But... I wasn't confident enough to propose it as a
> "mature alternative". And I don't have any performance benchmark,
> which would be interesting. So, I thought that the better option would
> be to send this patch to the developers as an alternative to ucontext.

As a starting point, I suggest looking at
test-coroutine.c:perf_lifecycle().  It's a simple create-and-then-enter
benchmark which measures the latency of doing this.  I expect you will
find performance is identical to the ucontext version because the
coroutine should be pooled and created using sigaltstack only once.

The interesting thing would be to benchmark ucontext coroutine creation
against sigaltstack.  Even then it may not matter much as long as pooled
coroutines are used most of the time.

Stefan

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Stefan Hajnoczi]

On Mon, Feb 13, 2012 at 03:42:28PM +0100, Alex Barcelo wrote:
> +/*
> + * This is used as the signal handler. This is called with the brand new stack
> + * (thanks to sigaltstack). We have to return, given that this is a signal
> + * handler and the sigmask and some other things are changed.
> + */
> +static void coroutine_trampoline(int signal)
> +{
> +    CoroutineUContext *self;
> +    Coroutine *co;
> +
> +    /* This will break on multithread or in any race condition */
> +    self = ptr_for_handler;
> +    tr_called = 1;
> +    co = &self->base;
> +
> +    /*
> +     * Here we have to do a bit of a ping pong between the caller, given that
> +     * this is a signal handler and we have to do a return "soon". Then the
> +     * caller can reestablish everything and do a longjmp here again.
> +     */
> +    if (!setjmp(*((jmp_buf *)&tr_reenter))) {
> +        return;
> +    }

setjmp() followed by return is usually bad.  We're relying on the fact
that the return code path here does not clobber local variables 'self'
and 'co'.  Can't we longjmp out back to the coroutine_new() function
instead?

> +static Coroutine *coroutine_new(void)
> +{
> +    const size_t stack_size = 1 << 20;

This reminds me of a recent observation that QEMU is using a lot of
memory in a bursty pattern.  I wonder if coroutine stacks are the cause
for this behavior - they are pretty big.  Not a specific problem with
your implementation since we do the same for other coroutine
implementations.

> +    /*
> +     * Preserve the SIGUSR1 signal state, block SIGUSR1,
> +     * and establish our signal handler. The signal will
> +     * later transfer control onto the signal stack.
> +     */
> +    sigemptyset(&sigs);
> +    sigaddset(&sigs, SIGUSR1);
> +    sigprocmask(SIG_BLOCK, &sigs, &osigs);
> +    sa.sa_handler = coroutine_trampoline;
> +    sigfillset(&sa.sa_mask);
> +    sa.sa_flags = SA_ONSTACK;
> +    if (sigaction(SIGUSR1, &sa, &osa) != 0) {
> +        abort();
> +    }
> +
> +    /*
> +     * Set the new stack.
> +     */
> +    ss.ss_sp = co->stack;
> +    ss.ss_size = stack_size;
> +    ss.ss_flags = 0;
> +    if (sigaltstack(&ss, &oss) < 0) {
> +        abort();
> +    }
> +
> +    /*
> +     * Now transfer control onto the signal stack and set it up.
> +     * It will return immediately via "return" after the setjmp()
> +     * was performed. Be careful here with race conditions.  The
> +     * signal can be delivered the first time sigsuspend() is
> +     * called.
> +     */
> +    tr_called = 0;
> +    kill(getpid(), SIGUSR1);
> +    sigfillset(&sigs);
> +    sigdelset(&sigs, SIGUSR1);
> +    while (!tr_called) {
> +        sigsuspend(&sigs);
> +    }
> +
> +    /*
> +     * Inform the system that we are back off the signal stack by
> +     * removing the alternative signal stack. Be careful here: It
> +     * first has to be disabled, before it can be removed.
> +     */
> +    sigaltstack(NULL, &ss);

What happens when a vcpu thread creates a coroutine while another QEMU
thread raises SIG_IPI?  The SIG_IPI will be handled on the alternate
signal stack and could corrupt the coroutine if the signal is handled
between sigsuspend(2) and sigaltstack(2).

Stefan

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Paolo Bonzini]

On 02/14/2012 10:24 AM, Stefan Hajnoczi wrote:
> setjmp() followed by return is usually bad.  We're relying on the fact
> that the return code path here does not clobber local variables 'self'
> and 'co'.  Can't we longjmp out back to the coroutine_new() function
> instead?

http://www.gnu.org/software/pth/rse-pmt.ps covers this.  Basically, this 
turned out to be more portable than longjmp from a signal handler.

Paolo

Re: [Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Alex Barcelo]

On Tue, Feb 14, 2012 at 09:33, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Feb 13, 2012 at 04:11:15PM +0100, Alex Barcelo wrote:
>> This new implementation... well, it seems to work (I have done an
>> ubuntu installation with a cdrom and a qcow drive, which seems to use
>> quite a lot of coroutines). Of course I have done the coroutine-test
>> and it was OK. But... I wasn't confident enough to propose it as a
>> "mature alternative". And I don't have any performance benchmark,
>> which would be interesting. So, I thought that the better option would
>> be to send this patch to the developers as an alternative to ucontext.
>
> As a starting point, I suggest looking at
> test-coroutine.c:perf_lifecycle().  It's a simple create-and-then-enter
> benchmark which measures the latency of doing this.  I expect you will
> find performance is identical to the ucontext version because the
> coroutine should be pooled and created using sigaltstack only once.
>
> The interesting thing would be to benchmark ucontext coroutine creation
> against sigaltstack.  Even then it may not matter much as long as pooled
> coroutines are used most of the time.

Didn't see the performance mode for test-coroutine. Now a benchmark
test it's easy (it's half-done). The lifecycle is not a good
benchmark, because sigaltstack is only called once. (As you said, the
timing change in less than 1%).

I thought that it would be interesting to add a performance test for
nesting (which can be coroutine creation intensive). So I did it. I
will send as a patch, is simple but it works for this.

The preliminary results are:
ucontext (traditional) method:
MSG: Nesting 1000000 iterations of 100000 depth each: 0.452988 s

sigaltstack (new) method:
MSG: Nesting 1000000 iterations of 100000 depth each: 0.689649 s

The sigaltstack is worse (well, it doesn't surprise me, it's more
complicated and does more jumps and is a code flow more erratic). But
a loss in efficiency in coroutines should not be important (how many
coroutines are created in a typical qemu-system execution? I'm
thinking "one"). Also as you said ;) pooled coroutines are used most
of the time, in real qemu-system execution.

tl,dr; the longjmps are the same and equally good (or bad) either way,
so performance won't really aknowledge the change. Will it?

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Alex Barcelo]

On Tue, Feb 14, 2012 at 10:24, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Feb 13, 2012 at 03:42:28PM +0100, Alex Barcelo wrote:
>> +    if (!setjmp(*((jmp_buf *)&tr_reenter))) {
>> +        return;
>> +    }
>
> setjmp() followed by return is usually bad.  We're relying on the fact
> that the return code path here does not clobber local variables 'self'
> and 'co'.  Can't we longjmp out back to the coroutine_new() function
> instead?

Paolo has answered this question. But some lighter reading: man sigreturn

Somebody has to call sigreturn. The easiest way is to do a return from
the signal handler. Calling manually sigreturn it's... tricky (and "It
should *never* be called directly.", man's page dixit). Knowing that
qemu has to work in lots of platforms, I wouldn't bet on that. Or on
any clever procmask direct manipulation.

>> +static Coroutine *coroutine_new(void)
>> +{
>> +    const size_t stack_size = 1 << 20;
>
> This reminds me of a recent observation that QEMU is using a lot of
> memory in a bursty pattern.  I wonder if coroutine stacks are the cause
> for this behavior - they are pretty big.  Not a specific problem with
> your implementation since we do the same for other coroutine
> implementations.

Agree. But few coroutines are created (one or two in an average
qemu-system execution, as I have checked). So... well, not sure if
this is our guilty malloc. Anyway, as you say, the behaviour is the
same in ucontext than sigaltstack, so it should not be a matter for
this series of patches.

>> +    /*
>> +     * Preserve the SIGUSR1 signal state, block SIGUSR1,
>> +     * and establish our signal handler. The signal will
>> +     * later transfer control onto the signal stack.
>> +     */
>> +    sigemptyset(&sigs);
>> +    sigaddset(&sigs, SIGUSR1);
>> +    sigprocmask(SIG_BLOCK, &sigs, &osigs);
>> +    sa.sa_handler = coroutine_trampoline;
>> +    sigfillset(&sa.sa_mask);
>> +    sa.sa_flags = SA_ONSTACK;
>> +    if (sigaction(SIGUSR1, &sa, &osa) != 0) {
>> +        abort();
>> +    }
>> +
>> +    /*
>> +     * Set the new stack.
>> +     */
>> +    ss.ss_sp = co->stack;
>> +    ss.ss_size = stack_size;
>> +    ss.ss_flags = 0;
>> +    if (sigaltstack(&ss, &oss) < 0) {
>> +        abort();
>> +    }
>> +
>> +    /*
>> +     * Now transfer control onto the signal stack and set it up.
>> +     * It will return immediately via "return" after the setjmp()
>> +     * was performed. Be careful here with race conditions.  The
>> +     * signal can be delivered the first time sigsuspend() is
>> +     * called.
>> +     */
>> +    tr_called = 0;
>> +    kill(getpid(), SIGUSR1);
>> +    sigfillset(&sigs);
>> +    sigdelset(&sigs, SIGUSR1);
>> +    while (!tr_called) {
>> +        sigsuspend(&sigs);
>> +    }
>> +
>> +    /*
>> +     * Inform the system that we are back off the signal stack by
>> +     * removing the alternative signal stack. Be careful here: It
>> +     * first has to be disabled, before it can be removed.
>> +     */
>> +    sigaltstack(NULL, &ss);
>
> What happens when a vcpu thread creates a coroutine while another QEMU
> thread raises SIG_IPI?  The SIG_IPI will be handled on the alternate
> signal stack

mmm no, it won't. The sigaction is set for the SIGUSR1 only (yes I
have to change it to sigusr2, the V2 will have this changed). And only
this signal will be handled on an alternate stack (the sa.sa_flags is
the responsible).

I'm not really sure about that, did I miss something?

> and could corrupt the coroutine if the signal is handled
> between sigsuspend(2) and sigaltstack(2).
> Stefan

Re: [Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Stefan Hajnoczi]

On Tue, Feb 14, 2012 at 11:38 AM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> On Tue, Feb 14, 2012 at 09:33, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Mon, Feb 13, 2012 at 04:11:15PM +0100, Alex Barcelo wrote:
>>> This new implementation... well, it seems to work (I have done an
>>> ubuntu installation with a cdrom and a qcow drive, which seems to use
>>> quite a lot of coroutines). Of course I have done the coroutine-test
>>> and it was OK. But... I wasn't confident enough to propose it as a
>>> "mature alternative". And I don't have any performance benchmark,
>>> which would be interesting. So, I thought that the better option would
>>> be to send this patch to the developers as an alternative to ucontext.
>>
>> As a starting point, I suggest looking at
>> test-coroutine.c:perf_lifecycle().  It's a simple create-and-then-enter
>> benchmark which measures the latency of doing this.  I expect you will
>> find performance is identical to the ucontext version because the
>> coroutine should be pooled and created using sigaltstack only once.
>>
>> The interesting thing would be to benchmark ucontext coroutine creation
>> against sigaltstack.  Even then it may not matter much as long as pooled
>> coroutines are used most of the time.
>
> Didn't see the performance mode for test-coroutine. Now a benchmark
> test it's easy (it's half-done). The lifecycle is not a good
> benchmark, because sigaltstack is only called once. (As you said, the
> timing change in less than 1%).
>
> I thought that it would be interesting to add a performance test for
> nesting (which can be coroutine creation intensive). So I did it. I
> will send as a patch, is simple but it works for this.
>
> The preliminary results are:
> ucontext (traditional) method:
> MSG: Nesting 1000000 iterations of 100000 depth each: 0.452988 s
>
> sigaltstack (new) method:
> MSG: Nesting 1000000 iterations of 100000 depth each: 0.689649 s

Plase run the tests with more iterations.  The execution time should
be several seconds to reduce any scheduler impact or other hickups.  I
suggest scaling iterations up to around 10 seconds.

> The sigaltstack is worse (well, it doesn't surprise me, it's more
> complicated and does more jumps and is a code flow more erratic). But
> a loss in efficiency in coroutines should not be important (how many
> coroutines are created in a typical qemu-system execution? I'm
> thinking "one"). Also as you said ;) pooled coroutines are used most
> of the time, in real qemu-system execution.

No, a lot of coroutines are created - each parallel disk I/O request
involves a coroutine.  Coroutines are also being used in other
subsystems (e.g. virtfs).

Hopefully the number active coroutines is still <100 but it's definitely >1.

Stefan

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Stefan Hajnoczi]

On Tue, Feb 14, 2012 at 11:53 AM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> On Tue, Feb 14, 2012 at 10:24, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Mon, Feb 13, 2012 at 03:42:28PM +0100, Alex Barcelo wrote:
>>> +    /*
>>> +     * Preserve the SIGUSR1 signal state, block SIGUSR1,
>>> +     * and establish our signal handler. The signal will
>>> +     * later transfer control onto the signal stack.
>>> +     */
>>> +    sigemptyset(&sigs);
>>> +    sigaddset(&sigs, SIGUSR1);
>>> +    sigprocmask(SIG_BLOCK, &sigs, &osigs);
>>> +    sa.sa_handler = coroutine_trampoline;
>>> +    sigfillset(&sa.sa_mask);
>>> +    sa.sa_flags = SA_ONSTACK;
>>> +    if (sigaction(SIGUSR1, &sa, &osa) != 0) {
>>> +        abort();
>>> +    }
>>> +
>>> +    /*
>>> +     * Set the new stack.
>>> +     */
>>> +    ss.ss_sp = co->stack;
>>> +    ss.ss_size = stack_size;
>>> +    ss.ss_flags = 0;
>>> +    if (sigaltstack(&ss, &oss) < 0) {
>>> +        abort();
>>> +    }
>>> +
>>> +    /*
>>> +     * Now transfer control onto the signal stack and set it up.
>>> +     * It will return immediately via "return" after the setjmp()
>>> +     * was performed. Be careful here with race conditions.  The
>>> +     * signal can be delivered the first time sigsuspend() is
>>> +     * called.
>>> +     */
>>> +    tr_called = 0;
>>> +    kill(getpid(), SIGUSR1);
>>> +    sigfillset(&sigs);
>>> +    sigdelset(&sigs, SIGUSR1);
>>> +    while (!tr_called) {
>>> +        sigsuspend(&sigs);
>>> +    }
>>> +
>>> +    /*
>>> +     * Inform the system that we are back off the signal stack by
>>> +     * removing the alternative signal stack. Be careful here: It
>>> +     * first has to be disabled, before it can be removed.
>>> +     */
>>> +    sigaltstack(NULL, &ss);
>>
>> What happens when a vcpu thread creates a coroutine while another QEMU
>> thread raises SIG_IPI?  The SIG_IPI will be handled on the alternate
>> signal stack
>
> mmm no, it won't. The sigaction is set for the SIGUSR1 only (yes I
> have to change it to sigusr2, the V2 will have this changed). And only
> this signal will be handled on an alternate stack (the sa.sa_flags is
> the responsible).
>
> I'm not really sure about that, did I miss something?

What I meant is that there are other signals handlers installed and
the signals will be unblocked between the time when sigsuspend() has
returned and before sigaltstack(NULL, &ss) is executed.  This seems
like a race condition to me.

Stefan

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Stefan Hajnoczi]

On Tue, Feb 14, 2012 at 9:50 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 02/14/2012 10:24 AM, Stefan Hajnoczi wrote:
>>
>> setjmp() followed by return is usually bad.  We're relying on the fact
>> that the return code path here does not clobber local variables 'self'
>> and 'co'.  Can't we longjmp out back to the coroutine_new() function
>> instead?
>
>
> http://www.gnu.org/software/pth/rse-pmt.ps covers this.  Basically, this
> turned out to be more portable than longjmp from a signal handler.

I suggest adding a comment explaining this, since this is normally not
an okay thing to do.

Stefan

Re: [Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Paul Brook]

> This series of patches implements coroutines method with
> sigaltstack.
> 
> The flow of creation and management of the coroutines is
> quite similar to the coroutine-ucontext.c. The way to use
> sigaltstack to achieve the needed stack manipulation is
> done in a way quite similar to the GNU Portable Threads
> (file pth_mctx.c, variant 2).

Maybe I've missed something, but why not just use pth?

Paul

Re: [Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Alex Barcelo]

On Tue, Feb 14, 2012 at 13:17, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Feb 14, 2012 at 11:38 AM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>> On Tue, Feb 14, 2012 at 09:33, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> On Mon, Feb 13, 2012 at 04:11:15PM +0100, Alex Barcelo wrote:
>>>> This new implementation... well, it seems to work (I have done an
>>>> ubuntu installation with a cdrom and a qcow drive, which seems to use
>>>> quite a lot of coroutines). Of course I have done the coroutine-test
>>>> and it was OK. But... I wasn't confident enough to propose it as a
>>>> "mature alternative". And I don't have any performance benchmark,
>>>> which would be interesting. So, I thought that the better option would
>>>> be to send this patch to the developers as an alternative to ucontext.
>>>
>>> As a starting point, I suggest looking at
>>> test-coroutine.c:perf_lifecycle().  It's a simple create-and-then-enter
>>> benchmark which measures the latency of doing this.  I expect you will
>>> find performance is identical to the ucontext version because the
>>> coroutine should be pooled and created using sigaltstack only once.
>>>
>>> The interesting thing would be to benchmark ucontext coroutine creation
>>> against sigaltstack.  Even then it may not matter much as long as pooled
>>> coroutines are used most of the time.
>>
>> Didn't see the performance mode for test-coroutine. Now a benchmark
>> test it's easy (it's half-done). The lifecycle is not a good
>> benchmark, because sigaltstack is only called once. (As you said, the
>> timing change in less than 1%).
>>
>> I thought that it would be interesting to add a performance test for
>> nesting (which can be coroutine creation intensive). So I did it. I
>> will send as a patch, is simple but it works for this.
>>
>> The preliminary results are:
>> ucontext (traditional) method:
>> MSG: Nesting 1000000 iterations of 100000 depth each: 0.452988 s
>>
>> sigaltstack (new) method:
>> MSG: Nesting 1000000 iterations of 100000 depth each: 0.689649 s
>
> Plase run the tests with more iterations.  The execution time should
> be several seconds to reduce any scheduler impact or other hickups.  I
> suggest scaling iterations up to around 10 seconds.

Ok, 10.2s vs 10.5s (still wins the traditional ucontext, but it
doesn't seem relevant any more).

>> The sigaltstack is worse (well, it doesn't surprise me, it's more
>> complicated and does more jumps and is a code flow more erratic). But
>> a loss in efficiency in coroutines should not be important (how many
>> coroutines are created in a typical qemu-system execution? I'm
>> thinking "one"). Also as you said ;) pooled coroutines are used most
>> of the time, in real qemu-system execution.
>
> No, a lot of coroutines are created - each parallel disk I/O request
> involves a coroutine.  Coroutines are also being used in other
> subsystems (e.g. virtfs).
>
> Hopefully the number active coroutines is still <100 but it's definitely >1.

I put a "Hello world, look, I'm in a coroutine" printf inside the
coroutine creation function, and I have only seen it twice in a normal
qemu-system execution. And I was doubting.

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Alex Barcelo]

On Tue, Feb 14, 2012 at 13:20, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Feb 14, 2012 at 11:53 AM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>> On Tue, Feb 14, 2012 at 10:24, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> (...)
>>> What happens when a vcpu thread creates a coroutine while another QEMU
>>> thread raises SIG_IPI?  The SIG_IPI will be handled on the alternate
>>> signal stack
>>
>> mmm no, it won't. The sigaction is set for the SIGUSR1 only (yes I
>> have to change it to sigusr2, the V2 will have this changed). And only
>> this signal will be handled on an alternate stack (the sa.sa_flags is
>> the responsible).
>>
>> I'm not really sure about that, did I miss something?
>
> What I meant is that there are other signals handlers installed and
> the signals will be unblocked between the time when sigsuspend() has
> returned and before sigaltstack(NULL, &ss) is executed.  This seems
> like a race condition to me.

But nobody (in qemu) uses sa.sa_flags ONSTACK, so I see no problem. If
a signal is delivered, it will be attended as it should. If there is
some other sigaltstack (I looked for it, and found nothing) then you
are right. But if no other signal uses sigaltstack, then there is no
race condition between the sigaltstack and the sigsuspend. And we only
use a signal that should not be used anywhere else (I have to change
that, seems that SIGUSR1 is being used in some point). So no conflict
here.

Have I understood you? I'm not sure if this is what you were talking
about... if not, please, explain a bit more the race condition and the
exact problem.

Re: [Qemu-devel] [PATCH 0/3] New sigaltstack method for coroutine

[Stefan Hajnoczi]

On Tue, Feb 14, 2012 at 1:12 PM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> On Tue, Feb 14, 2012 at 13:17, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Feb 14, 2012 at 11:38 AM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>>> On Tue, Feb 14, 2012 at 09:33, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>> On Mon, Feb 13, 2012 at 04:11:15PM +0100, Alex Barcelo wrote:
>>>>> This new implementation... well, it seems to work (I have done an
>>>>> ubuntu installation with a cdrom and a qcow drive, which seems to use
>>>>> quite a lot of coroutines). Of course I have done the coroutine-test
>>>>> and it was OK. But... I wasn't confident enough to propose it as a
>>>>> "mature alternative". And I don't have any performance benchmark,
>>>>> which would be interesting. So, I thought that the better option would
>>>>> be to send this patch to the developers as an alternative to ucontext.
>>>>
>>>> As a starting point, I suggest looking at
>>>> test-coroutine.c:perf_lifecycle().  It's a simple create-and-then-enter
>>>> benchmark which measures the latency of doing this.  I expect you will
>>>> find performance is identical to the ucontext version because the
>>>> coroutine should be pooled and created using sigaltstack only once.
>>>>
>>>> The interesting thing would be to benchmark ucontext coroutine creation
>>>> against sigaltstack.  Even then it may not matter much as long as pooled
>>>> coroutines are used most of the time.
>>>
>>> Didn't see the performance mode for test-coroutine. Now a benchmark
>>> test it's easy (it's half-done). The lifecycle is not a good
>>> benchmark, because sigaltstack is only called once. (As you said, the
>>> timing change in less than 1%).
>>>
>>> I thought that it would be interesting to add a performance test for
>>> nesting (which can be coroutine creation intensive). So I did it. I
>>> will send as a patch, is simple but it works for this.
>>>
>>> The preliminary results are:
>>> ucontext (traditional) method:
>>> MSG: Nesting 1000000 iterations of 100000 depth each: 0.452988 s
>>>
>>> sigaltstack (new) method:
>>> MSG: Nesting 1000000 iterations of 100000 depth each: 0.689649 s
>>
>> Plase run the tests with more iterations.  The execution time should
>> be several seconds to reduce any scheduler impact or other hickups.  I
>> suggest scaling iterations up to around 10 seconds.
>
> Ok, 10.2s vs 10.5s (still wins the traditional ucontext, but it
> doesn't seem relevant any more).
>
>>> The sigaltstack is worse (well, it doesn't surprise me, it's more
>>> complicated and does more jumps and is a code flow more erratic). But
>>> a loss in efficiency in coroutines should not be important (how many
>>> coroutines are created in a typical qemu-system execution? I'm
>>> thinking "one"). Also as you said ;) pooled coroutines are used most
>>> of the time, in real qemu-system execution.
>>
>> No, a lot of coroutines are created - each parallel disk I/O request
>> involves a coroutine.  Coroutines are also being used in other
>> subsystems (e.g. virtfs).
>>
>> Hopefully the number active coroutines is still <100 but it's definitely >1.
>
> I put a "Hello world, look, I'm in a coroutine" printf inside the
> coroutine creation function, and I have only seen it twice in a normal
> qemu-system execution. And I was doubting.

Run a couple of dd if=/dev/vda of=/dev/null iflag=direct processes
inside the guest to get some parallel I/O requests going.

Stefan

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Stefan Hajnoczi]

On Tue, Feb 14, 2012 at 1:21 PM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> On Tue, Feb 14, 2012 at 13:20, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Feb 14, 2012 at 11:53 AM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>>> On Tue, Feb 14, 2012 at 10:24, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>> (...)
>>>> What happens when a vcpu thread creates a coroutine while another QEMU
>>>> thread raises SIG_IPI?  The SIG_IPI will be handled on the alternate
>>>> signal stack
>>>
>>> mmm no, it won't. The sigaction is set for the SIGUSR1 only (yes I
>>> have to change it to sigusr2, the V2 will have this changed). And only
>>> this signal will be handled on an alternate stack (the sa.sa_flags is
>>> the responsible).
>>>
>>> I'm not really sure about that, did I miss something?
>>
>> What I meant is that there are other signals handlers installed and
>> the signals will be unblocked between the time when sigsuspend() has
>> returned and before sigaltstack(NULL, &ss) is executed.  This seems
>> like a race condition to me.
>
> But nobody (in qemu) uses sa.sa_flags ONSTACK, so I see no problem. If
> a signal is delivered, it will be attended as it should. If there is
> some other sigaltstack (I looked for it, and found nothing) then you
> are right. But if no other signal uses sigaltstack, then there is no
> race condition between the sigaltstack and the sigsuspend. And we only
> use a signal that should not be used anywhere else (I have to change
> that, seems that SIGUSR1 is being used in some point). So no conflict
> here.
>
> Have I understood you? I'm not sure if this is what you were talking
> about... if not, please, explain a bit more the race condition and the
> exact problem.

No, you are right.  It's not a problem because SA_ONSTACK isn't used
elsewhere.  Sorry, I missed that you need to set it explicitly on a
per-signal basis.

There's no issue with other signals here.

Stefan

Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Peter Maydell]

On 14 February 2012 09:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 02/14/2012 10:24 AM, Stefan Hajnoczi wrote:
>>
>> setjmp() followed by return is usually bad.  We're relying on the fact
>> that the return code path here does not clobber local variables 'self'
>> and 'co'.  Can't we longjmp out back to the coroutine_new() function
>> instead?
>
> http://www.gnu.org/software/pth/rse-pmt.ps covers this.  Basically, this
> turned out to be more portable than longjmp from a signal handler.

...but still not as portable as you might like. See
https://bugs.launchpad.net/ubuntu/+source/gnupg2/+bug/599862
 -- if you compile with a newer glibc and FORTIFY_SOURCE (which
is default in ubuntu IIRC) then it will complain about the
longjump-down-the-stack that pth and this code both do.

-- PMM