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 
> ---
>  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 


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 
---
 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(>throttled_reqs[false]);
+qemu_co_enter_next(>throttled_reqs[false], NULL);
 }
 
 static void fsdev_throttle_write_timer_cb(void *opaque)
 {
 FsThrottle *fst = opaque;
-qemu_co_enter_next(>throttled_reqs[true]);
+qemu_co_enter_next(>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(>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 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 
---
 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(>throttled_reqs[false]);
+qemu_co_enter_next(>throttled_reqs[false], NULL);
 }
 
 static void fsdev_throttle_write_timer_cb(void *opaque)
 {
 FsThrottle *fst = opaque;
-qemu_co_enter_next(>throttled_reqs[true]);
+qemu_co_enter_next(>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(>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

[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))

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

[Stefan Hajnoczi]

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 
> ---
>  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(>throttled_reqs[false]);
> +qemu_co_enter_next(>throttled_reqs[false], NULL);
>  }
>  
>  static void fsdev_throttle_write_timer_cb(void *opaque)
>  {
>  FsThrottle *fst = opaque;
> -qemu_co_enter_next(>throttled_reqs[true]);
> +qemu_co_enter_next(>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(>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
> 
> 
> 


signature.asc
Description: PGP signature

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

[Eric Blake]

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 
> ---

>  /**
> - * 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



signature.asc
Description: OpenPGP digital signature

[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 
---
 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(>throttled_reqs[false]);
+qemu_co_enter_next(>throttled_reqs[false], NULL);
 }
 
 static void fsdev_throttle_write_timer_cb(void *opaque)
 {
 FsThrottle *fst = opaque;
-qemu_co_enter_next(>throttled_reqs[true]);
+qemu_co_enter_next(>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(>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