[Qemu-devel] [PATCH v5 0/5] coroutine-lock: polymorphic CoQueue

[Qemu-devel] [PATCH v5 0/5] coroutine-lock: polymorphic CoQueue

[Paolo Bonzini]

There are cases in which a queued coroutine must be restarted from
non-coroutine context (with qemu_co_enter_next).  In this cases,
qemu_co_enter_next also needs to be thread-safe, but it cannot use a
CoMutex and so cannot qemu_co_queue_wait.  This happens in curl (which
right now is rolling its own list of Coroutines) and will happen in
Fam's NVMe driver as well.

This series extracts the idea of a polymorphic lockable object
from my "scoped lock guard" proposal, and applies it to CoQueue.
The implementation of QemuLockable is similar to C11 _Generic, but
redone using the preprocessor and GCC builtins for compatibility.

In general, while a bit on the esoteric side, the functionality used
to emulate _Generic is fairly old in GCC, and the builtins are already
used by include/qemu/atomic.h; the series was tested with Fedora 27 (boot
Damn Small Linux via http) and CentOS 6 (compiled only).

Paolo

v4->v5: fix checkpatch complaints

v3->v4: fix -O0 compilation [Fam]
        typos and copyright dates [Eric, Fam]
        improve CoQueue comments [Stefan]

Paolo Bonzini (5):
  test-coroutine: add simple CoMutex test
  lockable: add QemuLockable
  coroutine-lock: convert CoQueue to use QemuLockable
  coroutine-lock: make qemu_co_enter_next thread-safe
  curl: convert to CoQueue

 block/curl.c                | 20 ++--------
 fsdev/qemu-fsdev-throttle.c |  4 +-
 include/qemu/compiler.h     | 40 +++++++++++++++++++
 include/qemu/coroutine.h    | 29 +++++++++-----
 include/qemu/lockable.h     | 96 +++++++++++++++++++++++++++++++++++++++++++++
 include/qemu/thread.h       |  5 +--
 include/qemu/typedefs.h     |  4 ++
 tests/test-coroutine.c      | 75 ++++++++++++++++++++++++++++++++++-
 util/qemu-coroutine-lock.c  | 22 +++++++----
 9 files changed, 256 insertions(+), 39 deletions(-)
 create mode 100644 include/qemu/lockable.h

-- 
2.14.3

[Qemu-devel] [PATCH 1/5] test-coroutine: add simple CoMutex test

[Paolo Bonzini]

In preparation for adding a similar test using QemuLockable, add a very
simple testcase that has two interleaved calls to lock and unlock.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/test-coroutine.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
index 76c646107e..010cb95ad6 100644
--- a/tests/test-coroutine.c
+++ b/tests/test-coroutine.c
@@ -175,7 +175,7 @@ static void coroutine_fn c1_fn(void *opaque)
     qemu_coroutine_enter(c2);
 }
 
-static void test_co_queue(void)
+static void test_no_dangling_access(void)
 {
     Coroutine *c1;
     Coroutine *c2;
@@ -195,6 +195,51 @@ static void test_co_queue(void)
     *c1 = tmp;
 }
 
+static bool locked;
+static int done;
+
+static void coroutine_fn mutex_fn(void *opaque)
+{
+    CoMutex *m = opaque;
+    qemu_co_mutex_lock(m);
+    assert(!locked);
+    locked = true;
+    qemu_coroutine_yield();
+    locked = false;
+    qemu_co_mutex_unlock(m);
+    done++;
+}
+
+static void do_test_co_mutex(CoroutineEntry *entry, void *opaque)
+{
+    Coroutine *c1 = qemu_coroutine_create(entry, opaque);
+    Coroutine *c2 = qemu_coroutine_create(entry, opaque);
+
+    done = 0;
+    qemu_coroutine_enter(c1);
+    g_assert(locked);
+    qemu_coroutine_enter(c2);
+
+    /* Unlock queues c2.  It is then started automatically when c1 yields or
+     * terminates.
+     */
+    qemu_coroutine_enter(c1);
+    g_assert_cmpint(done, ==, 1);
+    g_assert(locked);
+
+    qemu_coroutine_enter(c2);
+    g_assert_cmpint(done, ==, 2);
+    g_assert(!locked);
+}
+
+static void test_co_mutex(void)
+{
+    CoMutex m;
+
+    qemu_co_mutex_init(&m);
+    do_test_co_mutex(mutex_fn, &m);
+}
+
 /*
  * Check that creation, enter, and return work
  */
@@ -422,7 +467,7 @@ int main(int argc, char **argv)
      * crash, so skip it.
      */
     if (CONFIG_COROUTINE_POOL) {
-        g_test_add_func("/basic/co_queue", test_co_queue);
+        g_test_add_func("/basic/no-dangling-access", test_no_dangling_access);
     }
 
     g_test_add_func("/basic/lifecycle", test_lifecycle);
@@ -432,6 +477,7 @@ int main(int argc, char **argv)
     g_test_add_func("/basic/entered", test_entered);
     g_test_add_func("/basic/in_coroutine", test_in_coroutine);
     g_test_add_func("/basic/order", test_order);
+    g_test_add_func("/locking/co-mutex", test_co_mutex);
     if (g_test_perf()) {
         g_test_add_func("/perf/lifecycle", perf_lifecycle);
         g_test_add_func("/perf/nesting", perf_nesting);
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/5] test-coroutine: add simple CoMutex test

[Richard Henderson]

On 02/03/2018 07:39 AM, Paolo Bonzini wrote:
> In preparation for adding a similar test using QemuLockable, add a very
> simple testcase that has two interleaved calls to lock and unlock.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/test-coroutine.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Paolo Bonzini]

QemuLockable is a polymorphic lock type that takes an object and
knows which function to use for locking and unlocking.  The
implementation could use C11 _Generic, but since the support is
not very widespread I am instead using __builtin_choose_expr and
__builtin_types_compatible_p, which are already used by
include/qemu/atomic.h.

QemuLockable can be used to implement lock guards, or to pass around
a lock in such a way that a function can release it and re-acquire it.
The next patch will do this for CoQueue.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/compiler.h  | 40 ++++++++++++++++++++
 include/qemu/coroutine.h |  4 +-
 include/qemu/lockable.h  | 96 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/qemu/thread.h    |  5 +--
 include/qemu/typedefs.h  |  4 ++
 tests/test-coroutine.c   | 25 +++++++++++++
 6 files changed, 169 insertions(+), 5 deletions(-)
 create mode 100644 include/qemu/lockable.h

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 340e5fdc09..5179bedb1e 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -111,4 +111,44 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+/* Implement C11 _Generic via GCC builtins.  Example:
+ *
+ *    QEMU_GENERIC(x, (float, sinf), (long double, sinl), sin) (x)
+ *
+ * The first argument is the discriminator.  The last is the default value.
+ * The middle ones are tuples in "(type, expansion)" format.
+ */
+
+/* First, find out the number of generic cases.  */
+#define QEMU_GENERIC(x, ...) \
+    QEMU_GENERIC_(typeof(x), __VA_ARGS__, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+
+/* There will be extra arguments, but they are not used.  */
+#define QEMU_GENERIC_(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, count, ...) \
+    QEMU_GENERIC##count(x, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
+
+/* Two more helper macros, this time to extract items from a parenthesized
+ * list.
+ */
+#define QEMU_FIRST_(a, b) a
+#define QEMU_SECOND_(a, b) b
+
+/* ... and a final one for the common part of the "recursion".  */
+#define QEMU_GENERIC_IF(x, type_then, else_)                                   \
+    __builtin_choose_expr(__builtin_types_compatible_p(x,                      \
+                                                       QEMU_FIRST_ type_then), \
+                          QEMU_SECOND_ type_then, else_)
+
+/* CPP poor man's "recursion".  */
+#define QEMU_GENERIC1(x, a0, ...) (a0)
+#define QEMU_GENERIC2(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC1(x, __VA_ARGS__))
+#define QEMU_GENERIC3(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC2(x, __VA_ARGS__))
+#define QEMU_GENERIC4(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC3(x, __VA_ARGS__))
+#define QEMU_GENERIC5(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC4(x, __VA_ARGS__))
+#define QEMU_GENERIC6(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC5(x, __VA_ARGS__))
+#define QEMU_GENERIC7(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC6(x, __VA_ARGS__))
+#define QEMU_GENERIC8(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC7(x, __VA_ARGS__))
+#define QEMU_GENERIC9(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC8(x, __VA_ARGS__))
+#define QEMU_GENERIC10(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC9(x, __VA_ARGS__))
+
 #endif /* COMPILER_H */
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index ce2eb73670..8a5129741c 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -121,7 +121,7 @@ bool qemu_coroutine_entered(Coroutine *co);
  * Provides a mutex that can be used to synchronise coroutines
  */
 struct CoWaitRecord;
-typedef struct CoMutex {
+struct CoMutex {
     /* Count of pending lockers; 0 for a free mutex, 1 for an
      * uncontended mutex.
      */
@@ -142,7 +142,7 @@ typedef struct CoMutex {
     unsigned handoff, sequence;
 
     Coroutine *holder;
-} CoMutex;
+};
 
 /**
  * Initialises a CoMutex. This must be called before any other operation is used
diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
new file mode 100644
index 0000000000..826ac3f675
--- /dev/null
+++ b/include/qemu/lockable.h
@@ -0,0 +1,96 @@
+/*
+ * Polymorphic locking functions (aka poor man templates)
+ *
+ * Copyright Red Hat, Inc. 2017, 2018
+ *
+ * Author: Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef QEMU_LOCKABLE_H
+#define QEMU_LOCKABLE_H
+
+#include "qemu/coroutine.h"
+#include "qemu/thread.h"
+
+typedef void QemuLockUnlockFunc(void *);
+
+struct QemuLockable {
+    void *object;
+    QemuLockUnlockFunc *lock;
+    QemuLockUnlockFunc *unlock;
+};
+
+/* This function gives an error if an invalid, non-NULL pointer type is passed
+ * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code elimination
+ * from the compiler, and give the errors already at link time.
+ */
+#ifdef __OPTIMIZE__
+void unknown_lock_type(void *);
+#else
+static inline void unknown_lock_type(void *unused)
+{
+    abort();
+}
+#endif
+
+static inline __attribute__((__always_inline__)) QemuLockable *
+qemu_make_lockable(void *x, QemuLockable *lockable)
+{
+    /* We cannot test this in a macro, otherwise we get compiler
+     * warnings like "the address of 'm' will always evaluate as 'true'".
+     */
+    return x ? lockable : NULL;
+}
+
+/* Auxiliary macros to simplify QEMU_MAKE_LOCABLE.  */
+#define QEMU_LOCK_FUNC(x) ((QemuLockUnlockFunc *)    \
+    QEMU_GENERIC(x,                                  \
+                 (QemuMutex *, qemu_mutex_lock),     \
+                 (CoMutex *, qemu_co_mutex_lock),    \
+                 (QemuSpin *, qemu_spin_lock),       \
+                 unknown_lock_type))
+
+#define QEMU_UNLOCK_FUNC(x) ((QemuLockUnlockFunc *)  \
+    QEMU_GENERIC(x,                                  \
+                 (QemuMutex *, qemu_mutex_unlock),   \
+                 (CoMutex *, qemu_co_mutex_unlock),  \
+                 (QemuSpin *, qemu_spin_unlock),     \
+                 unknown_lock_type))
+
+/* In C, compound literals have the lifetime of an automatic variable.
+ * In C++ it would be different, but then C++ wouldn't need QemuLockable
+ * either...
+ */
+#define QEMU_MAKE_LOCKABLE_(x) qemu_make_lockable((x), &(QemuLockable) {    \
+        .object = (x),                               \
+        .lock = QEMU_LOCK_FUNC(x),                   \
+        .unlock = QEMU_UNLOCK_FUNC(x),               \
+    })
+
+/* QEMU_MAKE_LOCKABLE - Make a polymorphic QemuLockable
+ *
+ * @x: a lock object (currently one of QemuMutex, CoMutex, QemuSpin).
+ *
+ * Returns a QemuLockable object that can be passed around
+ * to a function that can operate with locks of any kind.
+ */
+#define QEMU_MAKE_LOCKABLE(x)                        \
+    QEMU_GENERIC(x,                                  \
+                 (QemuLockable *, (x)),              \
+                 QEMU_MAKE_LOCKABLE_(x))
+
+static inline void qemu_lockable_lock(QemuLockable *x)
+{
+    x->lock(x->object);
+}
+
+static inline void qemu_lockable_unlock(QemuLockable *x)
+{
+    x->unlock(x->object);
+}
+
+#endif
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 9af4e945aa..ef7bd16123 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -4,7 +4,6 @@
 #include "qemu/processor.h"
 #include "qemu/atomic.h"
 
-typedef struct QemuMutex QemuMutex;
 typedef struct QemuCond QemuCond;
 typedef struct QemuSemaphore QemuSemaphore;
 typedef struct QemuEvent QemuEvent;
@@ -97,9 +96,9 @@ struct Notifier;
 void qemu_thread_atexit_add(struct Notifier *notifier);
 void qemu_thread_atexit_remove(struct Notifier *notifier);
 
-typedef struct QemuSpin {
+struct QemuSpin {
     int value;
-} QemuSpin;
+};
 
 static inline void qemu_spin_init(QemuSpin *spin)
 {
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 9bd7a834ba..5923849cdd 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -19,6 +19,7 @@ typedef struct BusClass BusClass;
 typedef struct BusState BusState;
 typedef struct Chardev Chardev;
 typedef struct CompatProperty CompatProperty;
+typedef struct CoMutex CoMutex;
 typedef struct CPUAddressSpace CPUAddressSpace;
 typedef struct CPUState CPUState;
 typedef struct DeviceListener DeviceListener;
@@ -86,9 +87,12 @@ typedef struct QEMUBH QEMUBH;
 typedef struct QemuConsole QemuConsole;
 typedef struct QemuDmaBuf QemuDmaBuf;
 typedef struct QEMUFile QEMUFile;
+typedef struct QemuLockable QemuLockable;
+typedef struct QemuMutex QemuMutex;
 typedef struct QemuOpt QemuOpt;
 typedef struct QemuOpts QemuOpts;
 typedef struct QemuOptsList QemuOptsList;
+typedef struct QemuSpin QemuSpin;
 typedef struct QEMUSGList QEMUSGList;
 typedef struct QEMUTimer QEMUTimer;
 typedef struct QEMUTimerListGroup QEMUTimerListGroup;
diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
index 010cb95ad6..6e4842bfda 100644
--- a/tests/test-coroutine.c
+++ b/tests/test-coroutine.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "qemu/coroutine.h"
 #include "qemu/coroutine_int.h"
+#include "qemu/lockable.h"
 
 /*
  * Check that qemu_in_coroutine() works
@@ -210,6 +211,18 @@ static void coroutine_fn mutex_fn(void *opaque)
     done++;
 }
 
+static void coroutine_fn lockable_fn(void *opaque)
+{
+    QemuLockable *x = opaque;
+    qemu_lockable_lock(x);
+    assert(!locked);
+    locked = true;
+    qemu_coroutine_yield();
+    locked = false;
+    qemu_lockable_unlock(x);
+    done++;
+}
+
 static void do_test_co_mutex(CoroutineEntry *entry, void *opaque)
 {
     Coroutine *c1 = qemu_coroutine_create(entry, opaque);
@@ -240,6 +253,17 @@ static void test_co_mutex(void)
     do_test_co_mutex(mutex_fn, &m);
 }
 
+static void test_co_mutex_lockable(void)
+{
+    CoMutex m;
+    CoMutex *null_pointer = NULL;
+
+    qemu_co_mutex_init(&m);
+    do_test_co_mutex(lockable_fn, QEMU_MAKE_LOCKABLE(&m));
+
+    g_assert(QEMU_MAKE_LOCKABLE(null_pointer) == NULL);
+}
+
 /*
  * Check that creation, enter, and return work
  */
@@ -478,6 +502,7 @@ int main(int argc, char **argv)
     g_test_add_func("/basic/in_coroutine", test_in_coroutine);
     g_test_add_func("/basic/order", test_order);
     g_test_add_func("/locking/co-mutex", test_co_mutex);
+    g_test_add_func("/locking/co-mutex/lockable", test_co_mutex_lockable);
     if (g_test_perf()) {
         g_test_add_func("/perf/lifecycle", perf_lifecycle);
         g_test_add_func("/perf/nesting", perf_nesting);
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Richard Henderson]

On 02/03/2018 07:39 AM, Paolo Bonzini wrote:
> +/* This function gives an error if an invalid, non-NULL pointer type is passed
> + * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code elimination
> + * from the compiler, and give the errors already at link time.
> + */
> +#ifdef __OPTIMIZE__
> +void unknown_lock_type(void *);
> +#else
> +static inline void unknown_lock_type(void *unused)
> +{
> +    abort();
> +}

Since you're using __builtin_choose_expr, I'm surprised you would need to test
__OPTIMZE__.  The nature of the builtin is such that it *must* eliminate the
other branch; otherwise the types don't even match up.

It might be worth using __attribute__((error("message"))) on the function too.

Otherwise,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable

[Paolo Bonzini]

On 03/02/2018 21:44, Richard Henderson wrote:
> On 02/03/2018 07:39 AM, Paolo Bonzini wrote:
>> +/* This function gives an error if an invalid, non-NULL pointer type is passed
>> + * to QEMU_MAKE_LOCKABLE.  For optimized builds, we can rely on dead-code elimination
>> + * from the compiler, and give the errors already at link time.
>> + */
>> +#ifdef __OPTIMIZE__
>> +void unknown_lock_type(void *);
>> +#else
>> +static inline void unknown_lock_type(void *unused)
>> +{
>> +    abort();
>> +}
> 
> Since you're using __builtin_choose_expr, I'm surprised you would need to test
> __OPTIMZE__.  The nature of the builtin is such that it *must* eliminate the
> other branch; otherwise the types don't even match up.

__builtin_choose_expr works fine.  However we also have

	return x ? __builtin_choose_expr(..., unknown_lock_type) : NULL;

and if "x" is NULL then its type is a void*, and the LHS will refer to
unknown_lock_type.  I was expecting __always_inline__ to be enough to
avoid this, but apparently this is not the case.

Paolo

> It might be worth using __attribute__((error("message"))) on the function too.
> 
> Otherwise,
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~
> 

[Qemu-devel] [PATCH 3/5] coroutine-lock: convert CoQueue to use QemuLockable

[Paolo Bonzini]

There are cases in which a queued coroutine must be restarted from
non-coroutine context (with qemu_co_enter_next).  In this cases,
qemu_co_enter_next also needs to be thread-safe, but it cannot use
a CoMutex and so cannot qemu_co_queue_wait.  Use QemuLockable so
that the CoQueue can interchangeably use CoMutex or QemuMutex.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/coroutine.h   |  6 +++++-
 util/qemu-coroutine-lock.c | 12 +++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 8a5129741c..1e5f0957e6 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -183,7 +183,9 @@ void qemu_co_queue_init(CoQueue *queue);
  * caller of the coroutine.  The mutex is unlocked during the wait and
  * locked again afterwards.
  */
-void coroutine_fn qemu_co_queue_wait(CoQueue *queue, CoMutex *mutex);
+#define qemu_co_queue_wait(queue, lock) \
+    qemu_co_queue_wait_impl(queue, QEMU_MAKE_LOCKABLE(lock))
+void coroutine_fn qemu_co_queue_wait_impl(CoQueue *queue, QemuLockable *lock);
 
 /**
  * Restarts the next coroutine in the CoQueue and removes it from the queue.
@@ -271,4 +273,6 @@ void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
  */
 void coroutine_fn yield_until_fd_readable(int fd);
 
+#include "qemu/lockable.h"
+
 #endif /* QEMU_COROUTINE_H */
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 846ff9167f..2a66fc1467 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -40,13 +40,13 @@ void qemu_co_queue_init(CoQueue *queue)
     QSIMPLEQ_INIT(&queue->entries);
 }
 
-void coroutine_fn qemu_co_queue_wait(CoQueue *queue, CoMutex *mutex)
+void coroutine_fn qemu_co_queue_wait_impl(CoQueue *queue, QemuLockable *lock)
 {
     Coroutine *self = qemu_coroutine_self();
     QSIMPLEQ_INSERT_TAIL(&queue->entries, self, co_queue_next);
 
-    if (mutex) {
-        qemu_co_mutex_unlock(mutex);
+    if (lock) {
+        qemu_lockable_unlock(lock);
     }
 
     /* There is no race condition here.  Other threads will call
@@ -60,9 +60,11 @@ void coroutine_fn qemu_co_queue_wait(CoQueue *queue, CoMutex *mutex)
     /* TODO: OSv implements wait morphing here, where the wakeup
      * primitive automatically places the woken coroutine on the
      * mutex's queue.  This avoids the thundering herd effect.
+     * This could be implemented for CoMutexes, but not really for
+     * other cases of QemuLockable.
      */
-    if (mutex) {
-        qemu_co_mutex_lock(mutex);
+    if (lock) {
+        qemu_lockable_lock(lock);
     }
 }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH 3/5] coroutine-lock: convert CoQueue to use QemuLockable

[Richard Henderson]

On 02/03/2018 07:39 AM, Paolo Bonzini wrote:
> There are cases in which a queued coroutine must be restarted from
> non-coroutine context (with qemu_co_enter_next).  In this cases,
> qemu_co_enter_next also needs to be thread-safe, but it cannot use
> a CoMutex and so cannot qemu_co_queue_wait.  Use QemuLockable so
> that the CoQueue can interchangeably use CoMutex or QemuMutex.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qemu/coroutine.h   |  6 +++++-
>  util/qemu-coroutine-lock.c | 12 +++++++-----
>  2 files changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe

[Paolo Bonzini]

qemu_co_queue_next does not need to release and re-acquire the mutex,
because the queued coroutine does not run immediately.  However, this
does not hold for qemu_co_enter_next.  Now that qemu_co_queue_wait
can synchronize (via QemuLockable) with code that is not running in
coroutine context, it's important that code using qemu_co_enter_next
can easily use a standardized locking idiom.

First of all, qemu_co_enter_next must use aio_co_wake to restart the
coroutine.  Second, the function gains a second argument, a QemuLockable*,
and the comments of qemu_co_queue_next and qemu_co_queue_restart_all
are adjusted to clarify the difference.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 fsdev/qemu-fsdev-throttle.c |  4 ++--
 include/qemu/coroutine.h    | 19 +++++++++++++------
 util/qemu-coroutine-lock.c  | 10 ++++++++--
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 49eebb5412..1dc07fbc12 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -20,13 +20,13 @@
 static void fsdev_throttle_read_timer_cb(void *opaque)
 {
     FsThrottle *fst = opaque;
-    qemu_co_enter_next(&fst->throttled_reqs[false]);
+    qemu_co_enter_next(&fst->throttled_reqs[false], NULL);
 }
 
 static void fsdev_throttle_write_timer_cb(void *opaque)
 {
     FsThrottle *fst = opaque;
-    qemu_co_enter_next(&fst->throttled_reqs[true]);
+    qemu_co_enter_next(&fst->throttled_reqs[true], NULL);
 }
 
 void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 1e5f0957e6..3afee7169b 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -188,21 +188,28 @@ void qemu_co_queue_init(CoQueue *queue);
 void coroutine_fn qemu_co_queue_wait_impl(CoQueue *queue, QemuLockable *lock);
 
 /**
- * Restarts the next coroutine in the CoQueue and removes it from the queue.
- *
- * Returns true if a coroutine was restarted, false if the queue is empty.
+ * Removes the next coroutine from the CoQueue, and wake it up.
+ * Returns true if a coroutine was removed, false if the queue is empty.
  */
 bool coroutine_fn qemu_co_queue_next(CoQueue *queue);
 
 /**
- * Restarts all coroutines in the CoQueue and leaves the queue empty.
+ * Empties the CoQueue; all coroutines are woken up.
  */
 void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue);
 
 /**
- * Enter the next coroutine in the queue
+ * Removes the next coroutine from the CoQueue, and wake it up.  Unlike
+ * qemu_co_queue_next, this function releases the lock during aio_co_wake
+ * because it is meant to be used outside coroutine context; in that case, the
+ * coroutine is entered immediately, before qemu_co_enter_next returns.
+ *
+ * If used in coroutine context, qemu_co_enter_next is equivalent to
+ * qemu_co_queue_next.
  */
-bool qemu_co_enter_next(CoQueue *queue);
+#define qemu_co_enter_next(queue, lock) \
+    qemu_co_enter_next_impl(queue, QEMU_MAKE_LOCKABLE(lock))
+bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock);
 
 /**
  * Checks if the CoQueue is empty.
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 2a66fc1467..78fb79acf8 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -132,7 +132,7 @@ void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue)
     qemu_co_queue_do_restart(queue, false);
 }
 
-bool qemu_co_enter_next(CoQueue *queue)
+bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock)
 {
     Coroutine *next;
 
@@ -142,7 +142,13 @@ bool qemu_co_enter_next(CoQueue *queue)
     }
 
     QSIMPLEQ_REMOVE_HEAD(&queue->entries, co_queue_next);
-    qemu_coroutine_enter(next);
+    if (lock) {
+        qemu_lockable_unlock(lock);
+    }
+    aio_co_wake(next);
+    if (lock) {
+        qemu_lockable_lock(lock);
+    }
     return true;
 }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe

[Richard Henderson]

On 02/03/2018 07:39 AM, Paolo Bonzini wrote:
> qemu_co_queue_next does not need to release and re-acquire the mutex,
> because the queued coroutine does not run immediately.  However, this
> does not hold for qemu_co_enter_next.  Now that qemu_co_queue_wait
> can synchronize (via QemuLockable) with code that is not running in
> coroutine context, it's important that code using qemu_co_enter_next
> can easily use a standardized locking idiom.
> 
> First of all, qemu_co_enter_next must use aio_co_wake to restart the
> coroutine.  Second, the function gains a second argument, a QemuLockable*,
> and the comments of qemu_co_queue_next and qemu_co_queue_restart_all
> are adjusted to clarify the difference.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  fsdev/qemu-fsdev-throttle.c |  4 ++--
>  include/qemu/coroutine.h    | 19 +++++++++++++------
>  util/qemu-coroutine-lock.c  | 10 ++++++++--
>  3 files changed, 23 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 5/5] curl: convert to CoQueue

[Paolo Bonzini]

Now that CoQueues can use a QemuMutex for thread-safety, there is no
need for curl to roll its own coroutine queue.  Coroutines can be
placed directly on the queue instead of using a list of CURLAIOCBs.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/curl.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 35cf417f59..cd578d3d14 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -101,8 +101,6 @@ typedef struct CURLAIOCB {
 
     size_t start;
     size_t end;
-
-    QSIMPLEQ_ENTRY(CURLAIOCB) next;
 } CURLAIOCB;
 
 typedef struct CURLSocket {
@@ -138,7 +136,7 @@ typedef struct BDRVCURLState {
     bool accept_range;
     AioContext *aio_context;
     QemuMutex mutex;
-    QSIMPLEQ_HEAD(, CURLAIOCB) free_state_waitq;
+    CoQueue free_state_waitq;
     char *username;
     char *password;
     char *proxyusername;
@@ -538,7 +536,6 @@ static int curl_init_state(BDRVCURLState *s, CURLState *state)
 /* Called with s->mutex held.  */
 static void curl_clean_state(CURLState *s)
 {
-    CURLAIOCB *next;
     int j;
     for (j = 0; j < CURL_NUM_ACB; j++) {
         assert(!s->acb[j]);
@@ -556,13 +553,7 @@ static void curl_clean_state(CURLState *s)
 
     s->in_use = 0;
 
-    next = QSIMPLEQ_FIRST(&s->s->free_state_waitq);
-    if (next) {
-        QSIMPLEQ_REMOVE_HEAD(&s->s->free_state_waitq, next);
-        qemu_mutex_unlock(&s->s->mutex);
-        aio_co_wake(next->co);
-        qemu_mutex_lock(&s->s->mutex);
-    }
+    qemu_co_enter_next(&s->s->free_state_waitq, &s->s->mutex);
 }
 
 static void curl_parse_filename(const char *filename, QDict *options,
@@ -784,7 +775,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     DPRINTF("CURL: Opening %s\n", file);
-    QSIMPLEQ_INIT(&s->free_state_waitq);
+    qemu_co_queue_init(&s->free_state_waitq);
     s->aio_context = bdrv_get_aio_context(bs);
     s->url = g_strdup(file);
     qemu_mutex_lock(&s->mutex);
@@ -888,10 +879,7 @@ static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
         if (state) {
             break;
         }
-        QSIMPLEQ_INSERT_TAIL(&s->free_state_waitq, acb, next);
-        qemu_mutex_unlock(&s->mutex);
-        qemu_coroutine_yield();
-        qemu_mutex_lock(&s->mutex);
+        qemu_co_queue_wait(&s->free_state_waitq, &s->mutex);
     }
 
     if (curl_init_state(s, state) < 0) {
-- 
2.14.3

Re: [Qemu-devel] [PATCH 5/5] curl: convert to CoQueue

[Richard Henderson]

On 02/03/2018 07:39 AM, Paolo Bonzini wrote:
> Now that CoQueues can use a QemuMutex for thread-safety, there is no
> need for curl to roll its own coroutine queue.  Coroutines can be
> placed directly on the queue instead of using a list of CURLAIOCBs.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/curl.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v5 0/5] coroutine-lock: polymorphic CoQueue

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2118 bytes --]

On Sat, Feb 03, 2018 at 10:39:30AM -0500, Paolo Bonzini wrote:
> There are cases in which a queued coroutine must be restarted from
> non-coroutine context (with qemu_co_enter_next).  In this cases,
> qemu_co_enter_next also needs to be thread-safe, but it cannot use a
> CoMutex and so cannot qemu_co_queue_wait.  This happens in curl (which
> right now is rolling its own list of Coroutines) and will happen in
> Fam's NVMe driver as well.
> 
> This series extracts the idea of a polymorphic lockable object
> from my "scoped lock guard" proposal, and applies it to CoQueue.
> The implementation of QemuLockable is similar to C11 _Generic, but
> redone using the preprocessor and GCC builtins for compatibility.
> 
> In general, while a bit on the esoteric side, the functionality used
> to emulate _Generic is fairly old in GCC, and the builtins are already
> used by include/qemu/atomic.h; the series was tested with Fedora 27 (boot
> Damn Small Linux via http) and CentOS 6 (compiled only).
> 
> Paolo
> 
> v4->v5: fix checkpatch complaints
> 
> v3->v4: fix -O0 compilation [Fam]
>         typos and copyright dates [Eric, Fam]
>         improve CoQueue comments [Stefan]
> 
> Paolo Bonzini (5):
>   test-coroutine: add simple CoMutex test
>   lockable: add QemuLockable
>   coroutine-lock: convert CoQueue to use QemuLockable
>   coroutine-lock: make qemu_co_enter_next thread-safe
>   curl: convert to CoQueue
> 
>  block/curl.c                | 20 ++--------
>  fsdev/qemu-fsdev-throttle.c |  4 +-
>  include/qemu/compiler.h     | 40 +++++++++++++++++++
>  include/qemu/coroutine.h    | 29 +++++++++-----
>  include/qemu/lockable.h     | 96 +++++++++++++++++++++++++++++++++++++++++++++
>  include/qemu/thread.h       |  5 +--
>  include/qemu/typedefs.h     |  4 ++
>  tests/test-coroutine.c      | 75 ++++++++++++++++++++++++++++++++++-
>  util/qemu-coroutine-lock.c  | 22 +++++++----
>  9 files changed, 256 insertions(+), 39 deletions(-)
>  create mode 100644 include/qemu/lockable.h
> 
> -- 
> 2.14.3
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v5 0/5] coroutine-lock: polymorphic CoQueue

[Fam Zheng]

On Sat, 02/03 10:39, Paolo Bonzini wrote:
> There are cases in which a queued coroutine must be restarted from
> non-coroutine context (with qemu_co_enter_next).  In this cases,
> qemu_co_enter_next also needs to be thread-safe, but it cannot use a
> CoMutex and so cannot qemu_co_queue_wait.  This happens in curl (which
> right now is rolling its own list of Coroutines) and will happen in
> Fam's NVMe driver as well.
> 
> This series extracts the idea of a polymorphic lockable object
> from my "scoped lock guard" proposal, and applies it to CoQueue.
> The implementation of QemuLockable is similar to C11 _Generic, but
> redone using the preprocessor and GCC builtins for compatibility.
> 
> In general, while a bit on the esoteric side, the functionality used
> to emulate _Generic is fairly old in GCC, and the builtins are already
> used by include/qemu/atomic.h; the series was tested with Fedora 27 (boot
> Damn Small Linux via http) and CentOS 6 (compiled only).
> 
> Paolo
> 
> v4->v5: fix checkpatch complaints

Queued, thanks.

Fam

[Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue

[Paolo Bonzini]

There are cases in which a queued coroutine must be restarted from
non-coroutine context (with qemu_co_enter_next).  In this cases,
qemu_co_enter_next also needs to be thread-safe, but it cannot use a
CoMutex and so cannot qemu_co_queue_wait.  This happens in curl (which
right now is rolling its own list of Coroutines) and will happen in
Fam's NVMe driver as well.

This series extracts the idea of a polymorphic lockable object
from my "scoped lock guard" proposal, and applies it to CoQueue.
The implementation of QemuLockable is similar to C11 _Generic, but
redone using the preprocessor and GCC builtins for compatibility.

In general, while a bit on the esoteric side, the functionality used
to emulate _Generic is fairly old in GCC, and the builtins are already
used by include/qemu/atomic.h; the series was tested with Fedora 27 (boot
Damn Small Linux via http) and CentOS 6 (compiled only).

Paolo

v3->v4: fix -O0 compilation [Fam]
        typos and copyright dates [Eric, Fam]
        improve CoQueue comments [Stefan]

Paolo Bonzini (5):
  test-coroutine: add simple CoMutex test
  lockable: add QemuLockable
  coroutine-lock: convert CoQueue to use QemuLockable
  coroutine-lock: make qemu_co_enter_next thread-safe
  curl: convert to CoQueue

 block/curl.c                | 20 ++--------
 fsdev/qemu-fsdev-throttle.c |  4 +-
 include/qemu/compiler.h     | 40 +++++++++++++++++++
 include/qemu/coroutine.h    | 29 +++++++++-----
 include/qemu/lockable.h     | 96 +++++++++++++++++++++++++++++++++++++++++++++
 include/qemu/thread.h       |  5 +--
 include/qemu/typedefs.h     |  4 ++
 tests/test-coroutine.c      | 75 ++++++++++++++++++++++++++++++++++-
 util/qemu-coroutine-lock.c  | 22 +++++++----
 9 files changed, 256 insertions(+), 39 deletions(-)
 create mode 100644 include/qemu/lockable.h

-- 
2.14.3

[Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe

[Paolo Bonzini]

qemu_co_queue_next does not need to release and re-acquire the mutex,
because the queued coroutine does not run immediately.  However, this
does not hold for qemu_co_enter_next.  Now that qemu_co_queue_wait
can synchronize (via QemuLockable) with code that is not running in
coroutine context, it's important that code using qemu_co_enter_next
can easily use a standardized locking idiom.

First of all, qemu_co_enter_next must use aio_co_wake to restart the
coroutine.  Second, the function gains a second argument, a QemuLockable*,
and the comments of qemu_co_queue_next and qemu_co_queue_restart_all
are adjusted to clarify the difference.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 fsdev/qemu-fsdev-throttle.c |  4 ++--
 include/qemu/coroutine.h    | 19 +++++++++++++------
 util/qemu-coroutine-lock.c  | 10 ++++++++--
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 49eebb5412..1dc07fbc12 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -20,13 +20,13 @@
 static void fsdev_throttle_read_timer_cb(void *opaque)
 {
     FsThrottle *fst = opaque;
-    qemu_co_enter_next(&fst->throttled_reqs[false]);
+    qemu_co_enter_next(&fst->throttled_reqs[false], NULL);
 }
 
 static void fsdev_throttle_write_timer_cb(void *opaque)
 {
     FsThrottle *fst = opaque;
-    qemu_co_enter_next(&fst->throttled_reqs[true]);
+    qemu_co_enter_next(&fst->throttled_reqs[true], NULL);
 }
 
 void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 1e5f0957e6..3afee7169b 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -188,21 +188,28 @@ void qemu_co_queue_init(CoQueue *queue);
 void coroutine_fn qemu_co_queue_wait_impl(CoQueue *queue, QemuLockable *lock);
 
 /**
- * Restarts the next coroutine in the CoQueue and removes it from the queue.
- *
- * Returns true if a coroutine was restarted, false if the queue is empty.
+ * Removes the next coroutine from the CoQueue, and wake it up.
+ * Returns true if a coroutine was removed, false if the queue is empty.
  */
 bool coroutine_fn qemu_co_queue_next(CoQueue *queue);
 
 /**
- * Restarts all coroutines in the CoQueue and leaves the queue empty.
+ * Empties the CoQueue; all coroutines are woken up.
  */
 void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue);
 
 /**
- * Enter the next coroutine in the queue
+ * Removes the next coroutine from the CoQueue, and wake it up.  Unlike
+ * qemu_co_queue_next, this function releases the lock during aio_co_wake
+ * because it is meant to be used outside coroutine context; in that case, the
+ * coroutine is entered immediately, before qemu_co_enter_next returns.
+ *
+ * If used in coroutine context, qemu_co_enter_next is equivalent to
+ * qemu_co_queue_next.
  */
-bool qemu_co_enter_next(CoQueue *queue);
+#define qemu_co_enter_next(queue, lock) \
+    qemu_co_enter_next_impl(queue, QEMU_MAKE_LOCKABLE(lock))
+bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock);
 
 /**
  * Checks if the CoQueue is empty.
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 2a66fc1467..78fb79acf8 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -132,7 +132,7 @@ void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue)
     qemu_co_queue_do_restart(queue, false);
 }
 
-bool qemu_co_enter_next(CoQueue *queue)
+bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock)
 {
     Coroutine *next;
 
@@ -142,7 +142,13 @@ bool qemu_co_enter_next(CoQueue *queue)
     }
 
     QSIMPLEQ_REMOVE_HEAD(&queue->entries, co_queue_next);
-    qemu_coroutine_enter(next);
+    if (lock) {
+        qemu_lockable_unlock(lock);
+    }
+    aio_co_wake(next);
+    if (lock) {
+        qemu_lockable_lock(lock);
+    }
     return true;
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH v3 0/5] coroutine-lock: polymorphic CoQueue

[Paolo Bonzini]

There are cases in which a queued coroutine must be restarted from
non-coroutine context (with qemu_co_enter_next).  In this cases,
qemu_co_enter_next also needs to be thread-safe, but it cannot use a
CoMutex and so cannot qemu_co_queue_wait.  This happens in curl (which
right now is rolling its own list of Coroutines) and will happen in
Fam's NVMe driver as well.

This series extracts the idea of a polymorphic lockable object
from my "scoped lock guard" proposal, and applies it to CoQueue.
The implementation of QemuLockable is similar to C11 _Generic, but
redone using the preprocessor and GCC builtins for compatibility.

In general, while a bit on the esoteric side, the functionality used
to emulate _Generic is fairly old in GCC, and the builtins are already
used by include/qemu/atomic.h; the series was tested with Fedora 27 (boot
Damn Small Linux via http) and CentOS 6 (compiled only).

Paolo

v1->v2: fix typos and copyright year

v2->v3: add tests, fix brown paper bag bug, avoid -Waddress errors

Paolo Bonzini (5):
  test-coroutine: add simple CoMutex test
  lockable: add QemuLockable
  coroutine-lock: convert CoQueue to use QemuLockable
  coroutine-lock: make qemu_co_enter_next thread-safe
  curl: convert to CoQueue

 block/curl.c                | 20 +++--------
 fsdev/qemu-fsdev-throttle.c |  4 +--
 include/qemu/compiler.h     | 40 +++++++++++++++++++++
 include/qemu/coroutine.h    | 25 ++++++++-----
 include/qemu/lockable.h     | 88 +++++++++++++++++++++++++++++++++++++++++++++
 include/qemu/thread.h       |  5 ++-
 include/qemu/typedefs.h     |  4 +++
 tests/test-coroutine.c      | 75 ++++++++++++++++++++++++++++++++++++--
 util/qemu-coroutine-lock.c  | 22 ++++++++----
 9 files changed, 245 insertions(+), 38 deletions(-)
 create mode 100644 include/qemu/lockable.h

-- 
2.14.3

[Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe

[Paolo Bonzini]

qemu_co_queue_next does not need to release and re-acquire the mutex,
because the queued coroutine does not run immediately.  However, this
does not hold for qemu_co_enter_next.  Now that qemu_co_queue_wait
can synchronize (via QemuLockable) with code that is not running in
coroutine context, it's important that code using qemu_co_enter_next
can easily use a standardized locking idiom.

First of all, qemu_co_enter_next must use aio_co_wake to restart the
coroutine.  Second, the function gains a second argument, a QemuLockable*,
and the comments of qemu_co_queue_next and qemu_co_queue_restart_all
are adjusted to clarify the difference.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 fsdev/qemu-fsdev-throttle.c |  4 ++--
 include/qemu/coroutine.h    | 15 ++++++++++-----
 util/qemu-coroutine-lock.c  | 10 ++++++++--
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 49eebb5412..1dc07fbc12 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -20,13 +20,13 @@
 static void fsdev_throttle_read_timer_cb(void *opaque)
 {
     FsThrottle *fst = opaque;
-    qemu_co_enter_next(&fst->throttled_reqs[false]);
+    qemu_co_enter_next(&fst->throttled_reqs[false], NULL);
 }
 
 static void fsdev_throttle_write_timer_cb(void *opaque)
 {
     FsThrottle *fst = opaque;
-    qemu_co_enter_next(&fst->throttled_reqs[true]);
+    qemu_co_enter_next(&fst->throttled_reqs[true], NULL);
 }
 
 void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 1e5f0957e6..6fdbc837c0 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -188,21 +188,26 @@ void qemu_co_queue_init(CoQueue *queue);
 void coroutine_fn qemu_co_queue_wait_impl(CoQueue *queue, QemuLockable *lock);
 
 /**
- * Restarts the next coroutine in the CoQueue and removes it from the queue.
+ * Removes the next coroutine from the CoQueue; it will run as soon as the
+ * current one yields.
  *
- * Returns true if a coroutine was restarted, false if the queue is empty.
+ * Returns true if a coroutine was removed, false if the queue is empty.
  */
 bool coroutine_fn qemu_co_queue_next(CoQueue *queue);
 
 /**
- * Restarts all coroutines in the CoQueue and leaves the queue empty.
+ * Empties the CoQueue; all coroutines in it will run in FIFO orer as soon
+ * as the current one yields.
  */
 void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue);
 
 /**
- * Enter the next coroutine in the queue
+ * Immediately enter the next coroutine in the queue.  Release the mutex
+ * while it runs.
  */
-bool qemu_co_enter_next(CoQueue *queue);
+#define qemu_co_enter_next(queue, lock) \
+    qemu_co_enter_next_impl(queue, QEMU_MAKE_LOCKABLE(lock))
+bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock);
 
 /**
  * Checks if the CoQueue is empty.
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 2a66fc1467..78fb79acf8 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -132,7 +132,7 @@ void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue)
     qemu_co_queue_do_restart(queue, false);
 }
 
-bool qemu_co_enter_next(CoQueue *queue)
+bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock)
 {
     Coroutine *next;
 
@@ -142,7 +142,13 @@ bool qemu_co_enter_next(CoQueue *queue)
     }
 
     QSIMPLEQ_REMOVE_HEAD(&queue->entries, co_queue_next);
-    qemu_coroutine_enter(next);
+    if (lock) {
+        qemu_lockable_unlock(lock);
+    }
+    aio_co_wake(next);
+    if (lock) {
+        qemu_lockable_lock(lock);
+    }
     return true;
 }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

On 01/25/2018 11:59 AM, Paolo Bonzini wrote:
> qemu_co_queue_next does not need to release and re-acquire the mutex,
> because the queued coroutine does not run immediately.  However, this
> does not hold for qemu_co_enter_next.  Now that qemu_co_queue_wait
> can synchronize (via QemuLockable) with code that is not running in
> coroutine context, it's important that code using qemu_co_enter_next
> can easily use a standardized locking idiom.
> 
> First of all, qemu_co_enter_next must use aio_co_wake to restart the
> coroutine.  Second, the function gains a second argument, a QemuLockable*,
> and the comments of qemu_co_queue_next and qemu_co_queue_restart_all
> are adjusted to clarify the difference.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

>  /**
> - * Restarts all coroutines in the CoQueue and leaves the queue empty.
> + * Empties the CoQueue; all coroutines in it will run in FIFO orer as soon

s/orer/order/

> + * as the current one yields.
>   */
>  void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue);


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4643 bytes --]

On Thu, Jan 25, 2018 at 06:59:48PM +0100, Paolo Bonzini wrote:
> qemu_co_queue_next does not need to release and re-acquire the mutex,
> because the queued coroutine does not run immediately.  However, this
> does not hold for qemu_co_enter_next.  Now that qemu_co_queue_wait
> can synchronize (via QemuLockable) with code that is not running in
> coroutine context, it's important that code using qemu_co_enter_next
> can easily use a standardized locking idiom.
> 
> First of all, qemu_co_enter_next must use aio_co_wake to restart the
> coroutine.  Second, the function gains a second argument, a QemuLockable*,
> and the comments of qemu_co_queue_next and qemu_co_queue_restart_all
> are adjusted to clarify the difference.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  fsdev/qemu-fsdev-throttle.c |  4 ++--
>  include/qemu/coroutine.h    | 15 ++++++++++-----
>  util/qemu-coroutine-lock.c  | 10 ++++++++--
>  3 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
> index 49eebb5412..1dc07fbc12 100644
> --- a/fsdev/qemu-fsdev-throttle.c
> +++ b/fsdev/qemu-fsdev-throttle.c
> @@ -20,13 +20,13 @@
>  static void fsdev_throttle_read_timer_cb(void *opaque)
>  {
>      FsThrottle *fst = opaque;
> -    qemu_co_enter_next(&fst->throttled_reqs[false]);
> +    qemu_co_enter_next(&fst->throttled_reqs[false], NULL);
>  }
>  
>  static void fsdev_throttle_write_timer_cb(void *opaque)
>  {
>      FsThrottle *fst = opaque;
> -    qemu_co_enter_next(&fst->throttled_reqs[true]);
> +    qemu_co_enter_next(&fst->throttled_reqs[true], NULL);
>  }
>  
>  void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp)
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 1e5f0957e6..6fdbc837c0 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -188,21 +188,26 @@ void qemu_co_queue_init(CoQueue *queue);
>  void coroutine_fn qemu_co_queue_wait_impl(CoQueue *queue, QemuLockable *lock);
>  
>  /**
> - * Restarts the next coroutine in the CoQueue and removes it from the queue.
> + * Removes the next coroutine from the CoQueue; it will run as soon as the
> + * current one yields.
>   *
> - * Returns true if a coroutine was restarted, false if the queue is empty.
> + * Returns true if a coroutine was removed, false if the queue is empty.
>   */
>  bool coroutine_fn qemu_co_queue_next(CoQueue *queue);
>  
>  /**
> - * Restarts all coroutines in the CoQueue and leaves the queue empty.
> + * Empties the CoQueue; all coroutines in it will run in FIFO orer as soon
> + * as the current one yields.
>   */
>  void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue);
>  
>  /**
> - * Enter the next coroutine in the queue
> + * Immediately enter the next coroutine in the queue.  Release the mutex
> + * while it runs.

s/mutex/lock/

Is this doc comment correct?  I see multiple cases due to aio_co_wake():

1. Called from coroutine context with current AioContext matching next
   coroutine's AioContext: arrange for next coroutine to be entered
   *after* we yield (not "immediately").

2. Called from non-coroutine context with current AioContext matching
   next coroutine's AioContext: immediately enter next coroutine.

3. Called from different AioContext than next coroutine: arrange for
   next coroutine to be entered at some point.

>   */
> -bool qemu_co_enter_next(CoQueue *queue);
> +#define qemu_co_enter_next(queue, lock) \
> +    qemu_co_enter_next_impl(queue, QEMU_MAKE_LOCKABLE(lock))
> +bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock);
>  
>  /**
>   * Checks if the CoQueue is empty.
> diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
> index 2a66fc1467..78fb79acf8 100644
> --- a/util/qemu-coroutine-lock.c
> +++ b/util/qemu-coroutine-lock.c
> @@ -132,7 +132,7 @@ void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue)
>      qemu_co_queue_do_restart(queue, false);
>  }
>  
> -bool qemu_co_enter_next(CoQueue *queue)
> +bool qemu_co_enter_next_impl(CoQueue *queue, QemuLockable *lock)
>  {
>      Coroutine *next;
>  
> @@ -142,7 +142,13 @@ bool qemu_co_enter_next(CoQueue *queue)
>      }
>  
>      QSIMPLEQ_REMOVE_HEAD(&queue->entries, co_queue_next);
> -    qemu_coroutine_enter(next);
> +    if (lock) {
> +        qemu_lockable_unlock(lock);
> +    }
> +    aio_co_wake(next);
> +    if (lock) {
> +        qemu_lockable_lock(lock);
> +    }
>      return true;
>  }
>  
> -- 
> 2.14.3
> 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe

[Paolo Bonzini]

On 29/01/2018 08:26, Stefan Hajnoczi wrote:
>>  /**
>> - * Enter the next coroutine in the queue
>> + * Immediately enter the next coroutine in the queue.  Release the mutex
>> + * while it runs.
> 
> s/mutex/lock/
> 
> Is this doc comment correct?  I see multiple cases due to aio_co_wake():
> 
> 1. Called from coroutine context with current AioContext matching next
>    coroutine's AioContext: arrange for next coroutine to be entered
>    *after* we yield (not "immediately").
> 
> 2. Called from non-coroutine context with current AioContext matching
>    next coroutine's AioContext: immediately enter next coroutine.
> 
> 3. Called from different AioContext than next coroutine: arrange for
>    next coroutine to be entered at some point.

Good point, it needs to be more specific.  Here is the best I could come up
with:

+ * Removes the next coroutine from the CoQueue, and wake it up.
  * Returns true if a coroutine was removed, false if the queue is empty.
  */
 bool coroutine_fn qemu_co_queue_next(CoQueue *queue);

...

+ * Empties the CoQueue; all coroutines are woken up.
  */
 void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue);

...

+ * Removes the next coroutine from the CoQueue, and wake it up.  Unlike
+ * qemu_co_queue_next, this function releases the lock during aio_co_wake
+ * because it is meant to be used outside coroutine context; in that case, the
+ * coroutine is entered immediately, before qemu_co_enter_next returns.
+ *
+ * If used in coroutine context, qemu_co_enter_next is equivalent to
+ * qemu_co_queue_next.
  */
 #define qemu_co_enter_next(queue, lock) \
     qemu_co_enter_next_impl(queue, QEMU_MAKE_LOCKABLE(lock))