* [Qemu-devel] [PATCH 1/5] test-coroutine: add simple CoMutex test
2018-02-01 21:29 [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue Paolo Bonzini
@ 2018-02-01 21:29 ` Paolo Bonzini
2018-02-01 21:29 ` [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable Paolo Bonzini
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2018-02-01 21:29 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, stefanha, qemu-block
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 = false;
+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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable
2018-02-01 21:29 [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue Paolo Bonzini
2018-02-01 21:29 ` [Qemu-devel] [PATCH 1/5] test-coroutine: add simple CoMutex test Paolo Bonzini
@ 2018-02-01 21:29 ` Paolo Bonzini
2018-02-01 21:29 ` [Qemu-devel] [PATCH 3/5] coroutine-lock: convert CoQueue to use QemuLockable Paolo Bonzini
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2018-02-01 21:29 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, stefanha, qemu-block
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 3/5] coroutine-lock: convert CoQueue to use QemuLockable
2018-02-01 21:29 [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue Paolo Bonzini
2018-02-01 21:29 ` [Qemu-devel] [PATCH 1/5] test-coroutine: add simple CoMutex test Paolo Bonzini
2018-02-01 21:29 ` [Qemu-devel] [PATCH 2/5] lockable: add QemuLockable Paolo Bonzini
@ 2018-02-01 21:29 ` Paolo Bonzini
2018-02-01 21:29 ` [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe Paolo Bonzini
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2018-02-01 21:29 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, stefanha, qemu-block
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe
2018-02-01 21:29 [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue Paolo Bonzini
` (2 preceding siblings ...)
2018-02-01 21:29 ` [Qemu-devel] [PATCH 3/5] coroutine-lock: convert CoQueue to use QemuLockable Paolo Bonzini
@ 2018-02-01 21:29 ` Paolo Bonzini
2018-02-01 21:29 ` [Qemu-devel] [PATCH 5/5] curl: convert to CoQueue Paolo Bonzini
2018-02-01 22:01 ` [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue no-reply
5 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2018-02-01 21:29 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, stefanha, qemu-block
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 5/5] curl: convert to CoQueue
2018-02-01 21:29 [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue Paolo Bonzini
` (3 preceding siblings ...)
2018-02-01 21:29 ` [Qemu-devel] [PATCH 4/5] coroutine-lock: make qemu_co_enter_next thread-safe Paolo Bonzini
@ 2018-02-01 21:29 ` Paolo Bonzini
2018-02-01 22:01 ` [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue no-reply
5 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2018-02-01 21:29 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, stefanha, qemu-block
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue
2018-02-01 21:29 [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue Paolo Bonzini
` (4 preceding siblings ...)
2018-02-01 21:29 ` [Qemu-devel] [PATCH 5/5] curl: convert to CoQueue Paolo Bonzini
@ 2018-02-01 22:01 ` no-reply
2018-02-02 5:53 ` Fam Zheng
5 siblings, 1 reply; 12+ messages in thread
From: no-reply @ 2018-02-01 22:01 UTC (permalink / raw)
To: pbonzini; +Cc: famz, qemu-devel
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180201212917.18131-1-pbonzini@redhat.com
Subject: [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
87da24d9ea curl: convert to CoQueue
a94c2af350 coroutine-lock: make qemu_co_enter_next thread-safe
57eba09708 coroutine-lock: convert CoQueue to use QemuLockable
6efa617c67 lockable: add QemuLockable
1423cb0ab4 test-coroutine: add simple CoMutex test
=== OUTPUT BEGIN ===
Checking PATCH 1/5: test-coroutine: add simple CoMutex test...
ERROR: do not initialise statics to 0 or NULL
#30: FILE: tests/test-coroutine.c:198:
+static bool locked = false;
total: 1 errors, 0 warnings, 74 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/5: lockable: add QemuLockable...
WARNING: line over 80 characters
#58: FILE: include/qemu/compiler.h:144:
+#define QEMU_GENERIC2(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC1(x, __VA_ARGS__))
WARNING: line over 80 characters
#59: FILE: include/qemu/compiler.h:145:
+#define QEMU_GENERIC3(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC2(x, __VA_ARGS__))
WARNING: line over 80 characters
#60: FILE: include/qemu/compiler.h:146:
+#define QEMU_GENERIC4(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC3(x, __VA_ARGS__))
WARNING: line over 80 characters
#61: FILE: include/qemu/compiler.h:147:
+#define QEMU_GENERIC5(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC4(x, __VA_ARGS__))
WARNING: line over 80 characters
#62: FILE: include/qemu/compiler.h:148:
+#define QEMU_GENERIC6(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC5(x, __VA_ARGS__))
WARNING: line over 80 characters
#63: FILE: include/qemu/compiler.h:149:
+#define QEMU_GENERIC7(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC6(x, __VA_ARGS__))
WARNING: line over 80 characters
#64: FILE: include/qemu/compiler.h:150:
+#define QEMU_GENERIC8(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC7(x, __VA_ARGS__))
WARNING: line over 80 characters
#65: FILE: include/qemu/compiler.h:151:
+#define QEMU_GENERIC9(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC8(x, __VA_ARGS__))
WARNING: line over 80 characters
#66: FILE: include/qemu/compiler.h:152:
+#define QEMU_GENERIC10(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC9(x, __VA_ARGS__))
WARNING: line over 80 characters
#124: FILE: include/qemu/lockable.h:28:
+ * to QEMU_MAKE_LOCKABLE. For optimized builds, we can rely on dead-code elimination
WARNING: architecture specific defines should be avoided
#127: FILE: include/qemu/lockable.h:31:
+#ifdef __OPTIMIZE__
total: 0 errors, 11 warnings, 242 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/5: coroutine-lock: convert CoQueue to use QemuLockable...
Checking PATCH 4/5: coroutine-lock: make qemu_co_enter_next thread-safe...
Checking PATCH 5/5: curl: convert to CoQueue...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue
2018-02-01 22:01 ` [Qemu-devel] [PATCH v4 0/5] coroutine-lock: polymorphic CoQueue no-reply
@ 2018-02-02 5:53 ` Fam Zheng
0 siblings, 0 replies; 12+ messages in thread
From: Fam Zheng @ 2018-02-02 5:53 UTC (permalink / raw)
To: QEMU Developers; +Cc: Paolo Bonzini, qemu-block, Stefan Hajnoczi
On Fri, Feb 2, 2018 at 6:01 AM, <no-reply@patchew.org> wrote:
> === OUTPUT BEGIN ===
> Checking PATCH 1/5: test-coroutine: add simple CoMutex test...
> ERROR: do not initialise statics to 0 or NULL
> #30: FILE: tests/test-coroutine.c:198:
> +static bool locked = false;
>
> total: 1 errors, 0 warnings, 74 lines checked
I think we should fix this one too. Otherwise,
Reviewed-by: Fam Zheng <famz@redhat.com>
>
> Your patch has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> Checking PATCH 2/5: lockable: add QemuLockable...
> WARNING: line over 80 characters
> #58: FILE: include/qemu/compiler.h:144:
> +#define QEMU_GENERIC2(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC1(x, __VA_ARGS__))
>
> WARNING: line over 80 characters
> #59: FILE: include/qemu/compiler.h:145:
> +#define QEMU_GENERIC3(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC2(x, __VA_ARGS__))
>
> WARNING: line over 80 characters
> #60: FILE: include/qemu/compiler.h:146:
> +#define QEMU_GENERIC4(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC3(x, __VA_ARGS__))
>
> WARNING: line over 80 characters
> #61: FILE: include/qemu/compiler.h:147:
> +#define QEMU_GENERIC5(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC4(x, __VA_ARGS__))
>
> WARNING: line over 80 characters
> #62: FILE: include/qemu/compiler.h:148:
> +#define QEMU_GENERIC6(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC5(x, __VA_ARGS__))
>
> WARNING: line over 80 characters
> #63: FILE: include/qemu/compiler.h:149:
> +#define QEMU_GENERIC7(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC6(x, __VA_ARGS__))
>
> WARNING: line over 80 characters
> #64: FILE: include/qemu/compiler.h:150:
> +#define QEMU_GENERIC8(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC7(x, __VA_ARGS__))
>
> WARNING: line over 80 characters
> #65: FILE: include/qemu/compiler.h:151:
> +#define QEMU_GENERIC9(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC8(x, __VA_ARGS__))
>
> WARNING: line over 80 characters
> #66: FILE: include/qemu/compiler.h:152:
> +#define QEMU_GENERIC10(x, a0, ...) QEMU_GENERIC_IF(x, a0, QEMU_GENERIC9(x, __VA_ARGS__))
>
> WARNING: line over 80 characters
> #124: FILE: include/qemu/lockable.h:28:
> + * to QEMU_MAKE_LOCKABLE. For optimized builds, we can rely on dead-code elimination
>
> WARNING: architecture specific defines should be avoided
> #127: FILE: include/qemu/lockable.h:31:
> +#ifdef __OPTIMIZE__
>
> total: 0 errors, 11 warnings, 242 lines checked
>
> Your patch has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> Checking PATCH 3/5: coroutine-lock: convert CoQueue to use QemuLockable...
> Checking PATCH 4/5: coroutine-lock: make qemu_co_enter_next thread-safe...
> Checking PATCH 5/5: curl: convert to CoQueue...
> === OUTPUT END ===
>
> Test command exited with code: 1
>
>
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 12+ messages in thread