diff mbox

WIP: add GCoroutine support

Message ID 1420823990-31461-1-git-send-email-marcandre.lureau@gmail.com
State New
Headers show

Commit Message

Marc-André Lureau Jan. 9, 2015, 5:19 p.m. UTC
Learn to use the GCoroutine library instead of qemu own coroutine
implementation.

GCoroutine is hosted on github:
https://github.com/elmarco/gcoroutine

This allows to share the same coroutine implementation between various
projects (gtk-vnc and spice-gtk). It is related to the effort to push
coroutine support in GLib. See also:
https://bugzilla.gnome.org/show_bug.cgi?id=719362

Notes:
- there are no GCoroutine releases, the API isn't frozen
- this patch hasn't been thoroughly tested
- GCoroutine doesn't implement pools yet
- GCoroutine is missing sigaltstack based coroutines
- spice-gtk and gtk-vnc patches are being worked on
---
 Makefile.objs             |  11 +++-
 configure                 |  26 +++++++-
 coroutine-gcoroutine.c    | 154 ++++++++++++++++++++++++++++++++++++++++++++++
 include/block/coroutine.h |  15 +++++
 4 files changed, 201 insertions(+), 5 deletions(-)
 create mode 100644 coroutine-gcoroutine.c

Comments

Peter Maydell Jan. 9, 2015, 5:32 p.m. UTC | #1
On 9 January 2015 at 17:19, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Learn to use the GCoroutine library instead of qemu own coroutine
> implementation.
>
> GCoroutine is hosted on github:
> https://github.com/elmarco/gcoroutine
>
> This allows to share the same coroutine implementation between various
> projects (gtk-vnc and spice-gtk). It is related to the effort to push
> coroutine support in GLib. See also:
> https://bugzilla.gnome.org/show_bug.cgi?id=719362
>
> Notes:
> - there are no GCoroutine releases, the API isn't frozen
> - this patch hasn't been thoroughly tested
> - GCoroutine doesn't implement pools yet
> - GCoroutine is missing sigaltstack based coroutines
> - spice-gtk and gtk-vnc patches are being worked on

It's bad enough having four coroutine backends without adding
yet another one :-(  I'm not sure we should take this patch
unless/until GCoroutine is fully implemented and supported in a
common enough version of glib that we could delete at least
one of our other backends in favour of this...

-- PMM
Stefan Hajnoczi Jan. 15, 2015, 2:54 p.m. UTC | #2
On Fri, Jan 09, 2015 at 05:32:14PM +0000, Peter Maydell wrote:
> On 9 January 2015 at 17:19, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> > Learn to use the GCoroutine library instead of qemu own coroutine
> > implementation.
> >
> > GCoroutine is hosted on github:
> > https://github.com/elmarco/gcoroutine
> >
> > This allows to share the same coroutine implementation between various
> > projects (gtk-vnc and spice-gtk). It is related to the effort to push
> > coroutine support in GLib. See also:
> > https://bugzilla.gnome.org/show_bug.cgi?id=719362
> >
> > Notes:
> > - there are no GCoroutine releases, the API isn't frozen
> > - this patch hasn't been thoroughly tested
> > - GCoroutine doesn't implement pools yet
> > - GCoroutine is missing sigaltstack based coroutines
> > - spice-gtk and gtk-vnc patches are being worked on
> 
> It's bad enough having four coroutine backends without adding
> yet another one :-(  I'm not sure we should take this patch
> unless/until GCoroutine is fully implemented and supported in a
> common enough version of glib that we could delete at least
> one of our other backends in favour of this...

It's an RFC.  I see this more of a way to collaborate on GCoroutine than
something that needs to be merged into qemu.git today.

Stefan
Stefan Hajnoczi Jan. 15, 2015, 3:08 p.m. UTC | #3
On Fri, Jan 09, 2015 at 06:19:50PM +0100, Marc-André Lureau wrote:
> Learn to use the GCoroutine library instead of qemu own coroutine
> implementation.
> 
> GCoroutine is hosted on github:
> https://github.com/elmarco/gcoroutine
> 
> This allows to share the same coroutine implementation between various
> projects (gtk-vnc and spice-gtk). It is related to the effort to push
> coroutine support in GLib. See also:
> https://bugzilla.gnome.org/show_bug.cgi?id=719362
> 
> Notes:
> - there are no GCoroutine releases, the API isn't frozen
> - this patch hasn't been thoroughly tested
> - GCoroutine doesn't implement pools yet
> - GCoroutine is missing sigaltstack based coroutines
> - spice-gtk and gtk-vnc patches are being worked on

The GCoroutine API has a very direct mapping to QEMU's coroutine
interface:
https://github.com/elmarco/gcoroutine/blob/master/src/gcoroutine.h

I like that you added gpointer arguments and return values to
enter/yield.  When writing the QEMU coroutine interface I dropped them
because I felt we wouldn't need them, but they are appropriate in a
general-purpose coroutine library.

Kevin Lieven and Peter Lieven are currently measuring and improving
coroutine performance in the QEMU block layer.  I guess this will result
in code changes to QEMU's coroutines implementation over the next weeks.

Stefan
Kevin Wolf Jan. 15, 2015, 3:54 p.m. UTC | #4
Am 09.01.2015 um 18:19 hat Marc-André Lureau geschrieben:
> Learn to use the GCoroutine library instead of qemu own coroutine
> implementation.
> 
> GCoroutine is hosted on github:
> https://github.com/elmarco/gcoroutine
> 
> This allows to share the same coroutine implementation between various
> projects (gtk-vnc and spice-gtk). It is related to the effort to push
> coroutine support in GLib. See also:
> https://bugzilla.gnome.org/show_bug.cgi?id=719362

Hm, I guess there might be uses for holding references after the
coroutine has exited if there are external references to the coroutine,
but is it useful not to drop one reference after the coroutine exits? I
guess in most cases, and in qemu always, the caller isn't aware that the
coroutine ended, so requiring an explicit call isn't as nice as it could
be.

Also, you modified the copyright lines. Would you mind restoring the
author names as in the original code, as their license requires? I see
that you also converted MIT licensed code into LGPL, without keeping the
original license text as required. For my code, I'm fine with the
conversion, but you should either have asked or left the original text
in place.

Kevin
Marc-André Lureau Jan. 22, 2015, 1:46 a.m. UTC | #5
On Thu, Jan 15, 2015 at 4:54 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Hm, I guess there might be uses for holding references after the
> coroutine has exited if there are external references to the coroutine,
> but is it useful not to drop one reference after the coroutine exits? I
> guess in most cases, and in qemu always, the caller isn't aware that the
> coroutine ended, so requiring an explicit call isn't as nice as it could
> be.

I thought it would be better to make memory management explicit by the
caller. However, I can also see how inconvenient it is for fire and
forget cases. Unfortunately, the coroutine can't ref/unref() itself at
beginning and end since the "unfinished" warning would show up, also
it feels wrong to unref itself inside the coroutine. So I changed it
to take an implicit ref on resume (similar to g_task_run_in_thread for
ex) and unref automatically when the coroutine finishes. So unlike
qemu-coroutine, you still have a strong reference returned, and must
unref it at some point. This should be similar to GThread/GTask I
think.

Fire and forget fix:
https://github.com/elmarco/gcoroutine/commit/7c88e1f90bb812481883da512a5d7d3b1e7d6028

Copyright fix:
https://github.com/elmarco/gcoroutine/commit/4a513d92e1377a5cd3526eeb8ed974f0a3221636
diff mbox

Patch

diff --git a/Makefile.objs b/Makefile.objs
index abeb902..250c58a 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -14,10 +14,17 @@  block-obj-$(CONFIG_WIN32) += aio-win32.o
 block-obj-y += block/
 block-obj-y += qemu-io-cmds.o
 
-block-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
-block-obj-y += qemu-coroutine-sleep.o
 block-obj-y += coroutine-$(CONFIG_COROUTINE_BACKEND).o
 
+ifeq ($(CONFIG_COROUTINE_BACKEND),gcoroutine)
+coroutine-gcoroutine.o-cflags := $(GCOROUTINE_CFLAGS)
+coroutine-gcoroutine.o-libs := $(GCOROUTINE_LIBS)
+else
+block-obj-y += qemu-coroutine.o qemu-coroutine-lock.o
+endif
+block-obj-y += qemu-coroutine-io.o
+block-obj-y += qemu-coroutine-sleep.o
+
 block-obj-m = block/
 
 
diff --git a/configure b/configure
index cae588c..e36ee74 100755
--- a/configure
+++ b/configure
@@ -1381,7 +1381,7 @@  Advanced options (experts only):
   --disable-seccomp        disable seccomp support
   --enable-seccomp         enable seccomp support
   --with-coroutine=BACKEND coroutine backend. Supported options:
-                           gthread, ucontext, sigaltstack, windows
+                           gcoroutine, gthread, ucontext, sigaltstack, windows
   --disable-coroutine-pool disable coroutine freelist (worse performance)
   --enable-coroutine-pool  enable coroutine freelist (better performance)
   --enable-glusterfs       enable GlusterFS backend
@@ -3870,8 +3870,16 @@  EOF
   fi
 fi
 
+if $pkg_config gcoroutine-1.0 --exists; then
+  gcoroutine_cflags=`$pkg_config --cflags gcoroutine-1.0`
+  gcoroutine_libs=`$pkg_config --libs gcoroutine-1.0`
+  gcoroutine=yes
+fi
+
 if test "$coroutine" = ""; then
-  if test "$mingw32" = "yes"; then
+  if test "$gcoroutine" = "yes"; then
+    coroutine=gcoroutine
+  elif test "$mingw32" = "yes"; then
     coroutine=win32
   elif test "$ucontext_works" = "yes"; then
     coroutine=ucontext
@@ -3898,6 +3906,11 @@  else
       error_exit "only the 'windows' coroutine backend is valid for Windows"
     fi
     ;;
+  gcoroutine)
+    if test "$gcoroutine" != "yes"; then
+      feature_not_found "gcoroutine"
+    fi
+    ;;
   *)
     error_exit "unknown coroutine backend $coroutine"
     ;;
@@ -4213,7 +4226,7 @@  EOF
 fi
 
 # prepend pixman and ftd flags after all config tests are done
-QEMU_CFLAGS="$pixman_cflags $fdt_cflags $QEMU_CFLAGS"
+QEMU_CFLAGS="$pixman_cflags $fdt_cflags $gcoroutine_cflags $QEMU_CFLAGS"
 libs_softmmu="$pixman_libs $libs_softmmu"
 
 echo "Install prefix    $prefix"
@@ -4733,6 +4746,13 @@  else
   echo "CONFIG_COROUTINE_POOL=0" >> $config_host_mak
 fi
 
+if test "$coroutine" = "gcoroutine" ; then
+  echo "CONFIG_GCOROUTINE=1" >> $config_host_mak
+  echo "GCOROUTINE_LIBS=$gcoroutine_libs" >> $config_host_mak
+  echo "GCOROUTINE_CFLAGS=$gcoroutine_cflags" >> $config_host_mak
+fi
+
+
 if test "$open_by_handle_at" = "yes" ; then
   echo "CONFIG_OPEN_BY_HANDLE=y" >> $config_host_mak
 fi
diff --git a/coroutine-gcoroutine.c b/coroutine-gcoroutine.c
new file mode 100644
index 0000000..3d5b749
--- /dev/null
+++ b/coroutine-gcoroutine.c
@@ -0,0 +1,154 @@ 
+/*
+ * GCoroutine wrapper
+ *
+ * Copyright (C) 2014  Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * 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/>.
+ */
+
+#include <gcoroutine.h>
+#include "qemu-common.h"
+#include "block/coroutine_int.h"
+
+Coroutine *qemu_coroutine_self(void)
+{
+    return (Coroutine *)g_coroutine_self();
+}
+
+bool qemu_in_coroutine(void)
+{
+    return g_in_coroutine();
+}
+
+void qemu_co_queue_init(CoQueue *queue)
+{
+    g_co_queue_init(queue);
+}
+
+void coroutine_fn qemu_co_queue_wait(CoQueue *queue)
+{
+    g_co_queue_yield(queue, NULL);
+}
+
+bool coroutine_fn qemu_co_queue_next(CoQueue *queue)
+{
+    return g_co_queue_schedule(queue, 1) == 1;
+}
+
+void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue)
+{
+    g_co_queue_schedule(queue, -1);
+}
+
+bool qemu_co_enter_next(CoQueue *queue)
+{
+    if (g_co_queue_is_empty(queue))
+        return false;
+
+    g_co_queue_resume_head(queue, NULL);
+
+    return true;
+}
+
+bool qemu_co_queue_empty(CoQueue *queue)
+{
+    return g_co_queue_is_empty(queue);
+}
+
+void qemu_co_mutex_init(CoMutex *mutex)
+{
+    g_co_mutex_init(mutex);
+}
+
+void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex)
+{
+    g_co_mutex_lock(mutex);
+}
+
+void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
+{
+    g_co_mutex_unlock(mutex);
+}
+
+void qemu_co_rwlock_init(CoRwlock *lock)
+{
+    g_co_rw_lock_init(lock);
+}
+
+void qemu_co_rwlock_rdlock(CoRwlock *lock)
+{
+    g_co_rw_lock_reader_lock(lock);
+}
+
+void qemu_co_rwlock_unlock(CoRwlock *lock)
+{
+    if (lock->writer)
+        g_co_rw_lock_writer_unlock(lock);
+    else
+        g_co_rw_lock_reader_unlock(lock);
+}
+
+void qemu_co_rwlock_wrlock(CoRwlock *lock)
+{
+    g_co_rw_lock_writer_lock(lock);
+}
+
+static gboolean coroutine_end_cb(gpointer data)
+{
+    GCoroutine *co = data;
+
+    g_coroutine_unref(co);
+
+    return FALSE;
+}
+
+static gpointer coroutine_func(gpointer data)
+{
+    CoroutineEntry *entry = data;
+
+    data = g_coroutine_yield(NULL);
+
+    entry(data);
+
+    g_idle_add(coroutine_end_cb, g_coroutine_self());
+
+    return NULL;
+}
+
+Coroutine *qemu_coroutine_create(CoroutineEntry *entry)
+{
+    GCoroutine *co = g_coroutine_new(coroutine_func);
+
+    if (!co)
+        return NULL;
+
+    g_coroutine_resume(co, entry);
+
+    return (Coroutine*)co;
+}
+
+void qemu_coroutine_enter(Coroutine *co, void *opaque)
+{
+    g_coroutine_resume((GCoroutine*)co, opaque);
+}
+
+void coroutine_fn qemu_coroutine_yield(void)
+{
+    g_coroutine_yield(NULL);
+}
+
+void qemu_coroutine_adjust_pool_size(int n)
+{
+    g_warning("no pool yet");
+}
diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index 793df0e..cb1fbfc 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -19,6 +19,9 @@ 
 #include "qemu/typedefs.h"
 #include "qemu/queue.h"
 #include "qemu/timer.h"
+#ifdef CONFIG_GCOROUTINE
+#include <gcoroutine.h>
+#endif
 
 /**
  * Coroutines are a mechanism for stack switching and can be used for
@@ -103,9 +106,13 @@  bool qemu_in_coroutine(void);
  * them later. They provide the fundamental primitives on which coroutine locks
  * are built.
  */
+#ifdef CONFIG_GCOROUTINE
+typedef GCoQueue CoQueue;
+#else
 typedef struct CoQueue {
     QTAILQ_HEAD(, Coroutine) entries;
 } CoQueue;
+#endif
 
 /**
  * Initialise a CoQueue. This must be called before any other operation is used
@@ -145,10 +152,14 @@  bool qemu_co_queue_empty(CoQueue *queue);
 /**
  * Provides a mutex that can be used to synchronise coroutines
  */
+#ifdef CONFIG_GCOROUTINE
+typedef GCoMutex CoMutex;
+#else
 typedef struct CoMutex {
     bool locked;
     CoQueue queue;
 } CoMutex;
+#endif
 
 /**
  * Initialises a CoMutex. This must be called before any other operation is used
@@ -168,11 +179,15 @@  void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex);
  */
 void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex);
 
+#ifdef CONFIG_GCOROUTINE
+typedef GCoRWLock CoRwlock;
+#else
 typedef struct CoRwlock {
     bool writer;
     int reader;
     CoQueue queue;
 } CoRwlock;
+#endif
 
 /**
  * Initialises a CoRwlock. This must be called before any other operation