[Qemu-devel] [PATCH 0/2] coroutine: dataplane support

[Qemu-devel] [PATCH 0/2] coroutine: dataplane support

[Stefan Hajnoczi]

There is ongoing work to enable multiple event loop threads.  This will allow
QEMU itself to take advantage of SMP and reduce Big QEMU Lock (BQL) contention.
This series is one step in that effort.

These patches make coroutines safe in a multi-event loop/multi-threaded world.
I have successfully tested them running qcow2 in a dataplane thread (further
patches are required which I'll be sending soon).

Patch 1 protects the global coroutine freelist with a lock :).

Patch 2 drops the CoQueue dependency on AioContext.  This allows CoMutex and
CoRwlock to operate in a dataplane thread whereas previously we always
scheduled coroutines in the QEMU iothread.

Stefan Hajnoczi (2):
  coroutine: protect global pool with a mutex
  coroutine: stop using AioContext in CoQueue

 include/block/coroutine_int.h |  4 ++++
 qemu-coroutine-lock.c         | 56 ++++++++++++++++---------------------------
 qemu-coroutine.c              | 23 ++++++++++++++++--
 trace-events                  |  2 +-
 4 files changed, 47 insertions(+), 38 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] coroutine: protect global pool with a mutex

[Stefan Hajnoczi]

The coroutine freelist is a global pool of unused coroutines.  It avoids
the setup/teardown overhead associated with the coroutine lifecycle.
Since the pool is global, we need to synchronize access so that
coroutines can be used outside the BQL.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-coroutine.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 25a14c6..60ac79e 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -14,6 +14,7 @@
 
 #include "trace.h"
 #include "qemu-common.h"
+#include "qemu/thread.h"
 #include "block/coroutine.h"
 #include "block/coroutine_int.h"
 
@@ -23,6 +24,7 @@ enum {
 };
 
 /** Free list to speed up creation */
+static QemuMutex pool_lock;
 static QSLIST_HEAD(, Coroutine) pool = QSLIST_HEAD_INITIALIZER(pool);
 static unsigned int pool_size;
 
@@ -30,11 +32,15 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
 {
     Coroutine *co;
 
+    qemu_mutex_lock(&pool_lock);
     co = QSLIST_FIRST(&pool);
     if (co) {
         QSLIST_REMOVE_HEAD(&pool, pool_next);
         pool_size--;
-    } else {
+    }
+    qemu_mutex_unlock(&pool_lock);
+
+    if (!co) {
         co = qemu_coroutine_new();
     }
 
@@ -44,17 +50,25 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
 
 static void coroutine_delete(Coroutine *co)
 {
+    qemu_mutex_lock(&pool_lock);
     if (pool_size < POOL_MAX_SIZE) {
         QSLIST_INSERT_HEAD(&pool, co, pool_next);
         co->caller = NULL;
         pool_size++;
+        qemu_mutex_unlock(&pool_lock);
         return;
     }
+    qemu_mutex_unlock(&pool_lock);
 
     qemu_coroutine_delete(co);
 }
 
-static void __attribute__((destructor)) coroutine_cleanup(void)
+static void __attribute__((constructor)) coroutine_pool_init(void)
+{
+    qemu_mutex_init(&pool_lock);
+}
+
+static void __attribute__((destructor)) coroutine_pool_cleanup(void)
 {
     Coroutine *co;
     Coroutine *tmp;
@@ -63,6 +77,8 @@ static void __attribute__((destructor)) coroutine_cleanup(void)
         QSLIST_REMOVE_HEAD(&pool, pool_next);
         qemu_coroutine_delete(co);
     }
+
+    qemu_mutex_destroy(&pool_lock);
 }
 
 static void coroutine_swap(Coroutine *from, Coroutine *to)
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/2] coroutine: stop using AioContext in CoQueue

[Stefan Hajnoczi]

qemu_co_queue_next(&queue) arranges that the next queued coroutine is
run at a later point in time.  This deferred restart is useful because
the caller may not want to transfer control yet.

This behavior was implemented using QEMUBH in the past, which meant that
CoQueue (and hence CoMutex and CoRwlock) had a dependency on the
AioContext event loop.  This hidden dependency causes trouble when we
move to a world with multiple event loops - now qemu_co_queue_next()
needs to know which event loop to schedule the QEMUBH in.

After pondering how to stash AioContext I realized the best solution is
to not use AioContext at all.  This patch implements the deferred
restart behavior purely in terms of coroutines and no longer uses
QEMUBH.

Here is how it works:

Each Coroutine has a wakeup queue that starts out empty.  When
qemu_co_queue_next() is called, the next coroutine is added to our
wakeup queue.  The wakeup queue is processed when we yield or terminate.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/block/coroutine_int.h |  4 ++++
 qemu-coroutine-lock.c         | 56 ++++++++++++++++---------------------------
 qemu-coroutine.c              |  3 +++
 trace-events                  |  2 +-
 4 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
index 17eb71e..f133d65 100644
--- a/include/block/coroutine_int.h
+++ b/include/block/coroutine_int.h
@@ -38,6 +38,9 @@ struct Coroutine {
     void *entry_arg;
     Coroutine *caller;
     QSLIST_ENTRY(Coroutine) pool_next;
+
+    /* Coroutines that should be woken up when we yield or terminate */
+    QTAILQ_HEAD(, Coroutine) co_queue_wakeup;
     QTAILQ_ENTRY(Coroutine) co_queue_next;
 };
 
@@ -45,5 +48,6 @@ Coroutine *qemu_coroutine_new(void);
 void qemu_coroutine_delete(Coroutine *co);
 CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
                                       CoroutineAction action);
+void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
 
 #endif
diff --git a/qemu-coroutine-lock.c b/qemu-coroutine-lock.c
index 86efe1f..d9fea49 100644
--- a/qemu-coroutine-lock.c
+++ b/qemu-coroutine-lock.c
@@ -26,39 +26,11 @@
 #include "block/coroutine.h"
 #include "block/coroutine_int.h"
 #include "qemu/queue.h"
-#include "block/aio.h"
 #include "trace.h"
 
-/* Coroutines are awoken from a BH to allow the current coroutine to complete
- * its flow of execution.  The BH may run after the CoQueue has been destroyed,
- * so keep BH data in a separate heap-allocated struct.
- */
-typedef struct {
-    QEMUBH *bh;
-    QTAILQ_HEAD(, Coroutine) entries;
-} CoQueueNextData;
-
-static void qemu_co_queue_next_bh(void *opaque)
-{
-    CoQueueNextData *data = opaque;
-    Coroutine *next;
-
-    trace_qemu_co_queue_next_bh();
-    while ((next = QTAILQ_FIRST(&data->entries))) {
-        QTAILQ_REMOVE(&data->entries, next, co_queue_next);
-        qemu_coroutine_enter(next, NULL);
-    }
-
-    qemu_bh_delete(data->bh);
-    g_slice_free(CoQueueNextData, data);
-}
-
 void qemu_co_queue_init(CoQueue *queue)
 {
     QTAILQ_INIT(&queue->entries);
-
-    /* This will be exposed to callers once there are multiple AioContexts */
-    queue->ctx = qemu_get_aio_context();
 }
 
 void coroutine_fn qemu_co_queue_wait(CoQueue *queue)
@@ -77,23 +49,37 @@ void coroutine_fn qemu_co_queue_wait_insert_head(CoQueue *queue)
     assert(qemu_in_coroutine());
 }
 
+/**
+ * qemu_co_queue_run_restart:
+ *
+ * Enter each coroutine that was previously marked for restart by
+ * qemu_co_queue_next() or qemu_co_queue_restart_all().  This function is
+ * invoked by the core coroutine code when the current coroutine yields or
+ * terminates.
+ */
+void qemu_co_queue_run_restart(Coroutine *co)
+{
+    Coroutine *next;
+
+    trace_qemu_co_queue_run_restart(co);
+    while ((next = QTAILQ_FIRST(&co->co_queue_wakeup))) {
+        QTAILQ_REMOVE(&co->co_queue_wakeup, next, co_queue_next);
+        qemu_coroutine_enter(next, NULL);
+    }
+}
+
 static bool qemu_co_queue_do_restart(CoQueue *queue, bool single)
 {
+    Coroutine *self = qemu_coroutine_self();
     Coroutine *next;
-    CoQueueNextData *data;
 
     if (QTAILQ_EMPTY(&queue->entries)) {
         return false;
     }
 
-    data = g_slice_new(CoQueueNextData);
-    data->bh = aio_bh_new(queue->ctx, qemu_co_queue_next_bh, data);
-    QTAILQ_INIT(&data->entries);
-    qemu_bh_schedule(data->bh);
-
     while ((next = QTAILQ_FIRST(&queue->entries)) != NULL) {
         QTAILQ_REMOVE(&queue->entries, next, co_queue_next);
-        QTAILQ_INSERT_TAIL(&data->entries, next, co_queue_next);
+        QTAILQ_INSERT_TAIL(&self->co_queue_wakeup, next, co_queue_next);
         trace_qemu_co_queue_next(next);
         if (single) {
             break;
diff --git a/qemu-coroutine.c b/qemu-coroutine.c
index 60ac79e..423430d 100644
--- a/qemu-coroutine.c
+++ b/qemu-coroutine.c
@@ -45,6 +45,7 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
     }
 
     co->entry = entry;
+    QTAILQ_INIT(&co->co_queue_wakeup);
     return co;
 }
 
@@ -87,6 +88,8 @@ static void coroutine_swap(Coroutine *from, Coroutine *to)
 
     ret = qemu_coroutine_switch(from, to, COROUTINE_YIELD);
 
+    qemu_co_queue_run_restart(to);
+
     switch (ret) {
     case COROUTINE_YIELD:
         return;
diff --git a/trace-events b/trace-events
index c03b9cb..389105c 100644
--- a/trace-events
+++ b/trace-events
@@ -825,7 +825,7 @@ qemu_coroutine_yield(void *from, void *to) "from %p to %p"
 qemu_coroutine_terminate(void *co) "self %p"
 
 # qemu-coroutine-lock.c
-qemu_co_queue_next_bh(void) ""
+qemu_co_queue_run_restart(void *co) "co %p"
 qemu_co_queue_next(void *nxt) "next %p"
 qemu_co_mutex_lock_entry(void *mutex, void *self) "mutex %p self %p"
 qemu_co_mutex_lock_return(void *mutex, void *self) "mutex %p self %p"
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 0/2] coroutine: dataplane support

[Kevin Wolf]

Am 17.05.2013 um 15:51 hat Stefan Hajnoczi geschrieben:
> There is ongoing work to enable multiple event loop threads.  This will allow
> QEMU itself to take advantage of SMP and reduce Big QEMU Lock (BQL) contention.
> This series is one step in that effort.
> 
> These patches make coroutines safe in a multi-event loop/multi-threaded world.
> I have successfully tested them running qcow2 in a dataplane thread (further
> patches are required which I'll be sending soon).
> 
> Patch 1 protects the global coroutine freelist with a lock :).
> 
> Patch 2 drops the CoQueue dependency on AioContext.  This allows CoMutex and
> CoRwlock to operate in a dataplane thread whereas previously we always
> scheduled coroutines in the QEMU iothread.
> 
> Stefan Hajnoczi (2):
>   coroutine: protect global pool with a mutex
>   coroutine: stop using AioContext in CoQueue
> 
>  include/block/coroutine_int.h |  4 ++++
>  qemu-coroutine-lock.c         | 56 ++++++++++++++++---------------------------
>  qemu-coroutine.c              | 23 ++++++++++++++++--
>  trace-events                  |  2 +-
>  4 files changed, 47 insertions(+), 38 deletions(-)

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] coroutine: dataplane support

[Peter Maydell]

On 17 May 2013 14:51, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> There is ongoing work to enable multiple event loop threads.  This will allow
> QEMU itself to take advantage of SMP and reduce Big QEMU Lock (BQL) contention.
> This series is one step in that effort.
>
> These patches make coroutines safe in a multi-event loop/multi-threaded world.
> I have successfully tested them running qcow2 in a dataplane thread (further
> patches are required which I'll be sending soon).

Did you test the sigaltstack backend as well as ucontext? I know
in theory they should be the same but still :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] coroutine: dataplane support

[Stefan Hajnoczi]

On Fri, May 17, 2013 at 04:08:36PM +0100, Peter Maydell wrote:
> On 17 May 2013 14:51, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > There is ongoing work to enable multiple event loop threads.  This will allow
> > QEMU itself to take advantage of SMP and reduce Big QEMU Lock (BQL) contention.
> > This series is one step in that effort.
> >
> > These patches make coroutines safe in a multi-event loop/multi-threaded world.
> > I have successfully tested them running qcow2 in a dataplane thread (further
> > patches are required which I'll be sending soon).
> 
> Did you test the sigaltstack backend as well as ucontext? I know
> in theory they should be the same but still :-)

Yes, I have tested sigaltstack successfully with make check and
qemu-iotests.

Stefan

Re: [Qemu-devel] [PATCH 0/2] coroutine: dataplane support

[Stefan Hajnoczi]

On Fri, May 17, 2013 at 03:51:24PM +0200, Stefan Hajnoczi wrote:
> There is ongoing work to enable multiple event loop threads.  This will allow
> QEMU itself to take advantage of SMP and reduce Big QEMU Lock (BQL) contention.
> This series is one step in that effort.
> 
> These patches make coroutines safe in a multi-event loop/multi-threaded world.
> I have successfully tested them running qcow2 in a dataplane thread (further
> patches are required which I'll be sending soon).
> 
> Patch 1 protects the global coroutine freelist with a lock :).
> 
> Patch 2 drops the CoQueue dependency on AioContext.  This allows CoMutex and
> CoRwlock to operate in a dataplane thread whereas previously we always
> scheduled coroutines in the QEMU iothread.
> 
> Stefan Hajnoczi (2):
>   coroutine: protect global pool with a mutex
>   coroutine: stop using AioContext in CoQueue
> 
>  include/block/coroutine_int.h |  4 ++++
>  qemu-coroutine-lock.c         | 56 ++++++++++++++++---------------------------
>  qemu-coroutine.c              | 23 ++++++++++++++++--
>  trace-events                  |  2 +-
>  4 files changed, 47 insertions(+), 38 deletions(-)
> 
> -- 
> 1.8.1.4
> 
> 

Applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan