All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] coroutine: new sleep/wake API
@ 2021-05-03 11:25 Paolo Bonzini
  2021-05-03 11:25 ` [PATCH 1/6] coroutine-sleep: use a stack-allocated timer Paolo Bonzini
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Paolo Bonzini @ 2021-05-03 11:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: eesposit, stefanha

This is a revamp of the qemu_co_sleep* API that makes it easier to
extend the API: the state that is needed to wake up a coroutine is now
part of the public API instead of hidden behind a pointer-to-pointer;
the API is made more extensible by pushing the rest of QemuCoSleepState
into local variables.

In the future, this will be extended to introduce a prepare/sleep/cancel
API similar to Linux's prepare_to_wait/schedule/finish_wait functions.
For now, this is just a nice refactoring.

Paolo Bonzini (6):
  coroutine-sleep: use a stack-allocated timer
  coroutine-sleep: disallow NULL QemuCoSleepState** argument
  coroutine-sleep: allow qemu_co_sleep_wake(NULL)
  coroutine-sleep: move timer out of QemuCoSleepState
  coroutine-sleep: replace QemuCoSleepState pointer with struct in the
    API
  coroutine-sleep: introduce qemu_co_sleep

 block/block-copy.c          | 10 +++---
 block/nbd.c                 | 14 +++-----
 include/qemu/coroutine.h    | 26 ++++++++------
 util/qemu-coroutine-sleep.c | 69 +++++++++++++++++--------------------
 4 files changed, 57 insertions(+), 62 deletions(-)

-- 
2.31.1



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/6] coroutine-sleep: use a stack-allocated timer
  2021-05-03 11:25 [PATCH 0/6] coroutine: new sleep/wake API Paolo Bonzini
@ 2021-05-03 11:25 ` Paolo Bonzini
  2021-05-10 10:00   ` Vladimir Sementsov-Ogievskiy
  2021-05-03 11:25 ` [PATCH 2/6] coroutine-sleep: disallow NULL QemuCoSleepState** argument Paolo Bonzini
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2021-05-03 11:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: eesposit, stefanha

The lifetime of the timer is well-known (it cannot outlive
qemu_co_sleep_ns_wakeable, because it's deleted by the time the
coroutine resumes), so it is not necessary to place it on the heap.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-coroutine-sleep.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index 8c4dac4fd7..eec6e81f3f 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -21,7 +21,7 @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";
 
 struct QemuCoSleepState {
     Coroutine *co;
-    QEMUTimer *ts;
+    QEMUTimer ts;
     QemuCoSleepState **user_state_pointer;
 };
 
@@ -35,7 +35,7 @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)
     if (sleep_state->user_state_pointer) {
         *sleep_state->user_state_pointer = NULL;
     }
-    timer_del(sleep_state->ts);
+    timer_del(&sleep_state->ts);
     aio_co_wake(sleep_state->co);
 }
 
@@ -50,7 +50,6 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
     AioContext *ctx = qemu_get_current_aio_context();
     QemuCoSleepState state = {
         .co = qemu_coroutine_self(),
-        .ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &state),
         .user_state_pointer = sleep_state,
     };
 
@@ -63,10 +62,11 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
         abort();
     }
 
+    aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state);
     if (sleep_state) {
         *sleep_state = &state;
     }
-    timer_mod(state.ts, qemu_clock_get_ns(type) + ns);
+    timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();
     if (sleep_state) {
         /*
@@ -75,5 +75,4 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
          */
         assert(*sleep_state == NULL);
     }
-    timer_free(state.ts);
 }
-- 
2.31.1




^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/6] coroutine-sleep: disallow NULL QemuCoSleepState** argument
  2021-05-03 11:25 [PATCH 0/6] coroutine: new sleep/wake API Paolo Bonzini
  2021-05-03 11:25 ` [PATCH 1/6] coroutine-sleep: use a stack-allocated timer Paolo Bonzini
@ 2021-05-03 11:25 ` Paolo Bonzini
  2021-05-10 10:03   ` Vladimir Sementsov-Ogievskiy
  2021-05-03 11:25 ` [PATCH 3/6] coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing Paolo Bonzini
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2021-05-03 11:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: eesposit, stefanha

Simplify the code by removing conditionals.  qemu_co_sleep_ns
can simply use point the argument to an on-stack temporary.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/coroutine.h    |  5 +++--
 util/qemu-coroutine-sleep.c | 18 +++++-------------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index ce5b9c6851..c5d7742989 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -295,7 +295,7 @@ typedef struct QemuCoSleepState QemuCoSleepState;
 
 /**
  * Yield the coroutine for a given duration. During this yield, @sleep_state
- * (if not NULL) is set to an opaque pointer, which may be used for
+ * is set to an opaque pointer, which may be used for
  * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the
  * timer fires. Don't save the obtained value to other variables and don't call
  * qemu_co_sleep_wake from another aio context.
@@ -304,7 +304,8 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
                                             QemuCoSleepState **sleep_state);
 static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
 {
-    qemu_co_sleep_ns_wakeable(type, ns, NULL);
+    QemuCoSleepState *unused = NULL;
+    qemu_co_sleep_ns_wakeable(type, ns, &unused);
 }
 
 /**
diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index eec6e81f3f..3f6f637e81 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -32,9 +32,7 @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)
                                            qemu_co_sleep_ns__scheduled, NULL);
 
     assert(scheduled == qemu_co_sleep_ns__scheduled);
-    if (sleep_state->user_state_pointer) {
-        *sleep_state->user_state_pointer = NULL;
-    }
+    *sleep_state->user_state_pointer = NULL;
     timer_del(&sleep_state->ts);
     aio_co_wake(sleep_state->co);
 }
@@ -63,16 +61,10 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
     }
 
     aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state);
-    if (sleep_state) {
-        *sleep_state = &state;
-    }
+    *sleep_state = &state;
     timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();
-    if (sleep_state) {
-        /*
-         * Note that *sleep_state is cleared during qemu_co_sleep_wake
-         * before resuming this coroutine.
-         */
-        assert(*sleep_state == NULL);
-    }
+
+    /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine.  */
+    assert(*sleep_state == NULL);
 }
-- 
2.31.1




^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/6] coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing
  2021-05-03 11:25 [PATCH 0/6] coroutine: new sleep/wake API Paolo Bonzini
  2021-05-03 11:25 ` [PATCH 1/6] coroutine-sleep: use a stack-allocated timer Paolo Bonzini
  2021-05-03 11:25 ` [PATCH 2/6] coroutine-sleep: disallow NULL QemuCoSleepState** argument Paolo Bonzini
@ 2021-05-03 11:25 ` Paolo Bonzini
  2021-05-10 10:15   ` Vladimir Sementsov-Ogievskiy
  2021-05-03 11:25 ` [PATCH 4/6] coroutine-sleep: move timer out of QemuCoSleepState Paolo Bonzini
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2021-05-03 11:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: eesposit, stefanha

All callers of qemu_co_sleep_wake are checking whether they are passing
a NULL argument inside the pointer-to-pointer: do the check in
qemu_co_sleep_wake itself.

As a side effect, qemu_co_sleep_wake can be called more than once and it
will only wake the coroutine once; after the first time, the argument
will be set to NULL via *sleep_state->user_state_pointer.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/block-copy.c          |  4 +---
 block/nbd.c                 |  8 ++------
 util/qemu-coroutine-sleep.c | 21 ++++++++++++---------
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 9b4af00614..f896dc56f2 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -674,9 +674,7 @@ out:
 
 void block_copy_kick(BlockCopyCallState *call_state)
 {
-    if (call_state->sleep_state) {
-        qemu_co_sleep_wake(call_state->sleep_state);
-    }
+    qemu_co_sleep_wake(call_state->sleep_state);
 }
 
 /*
diff --git a/block/nbd.c b/block/nbd.c
index 1d4668d42d..1c6315b168 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -289,9 +289,7 @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs)
     BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
 
     s->drained = true;
-    if (s->connection_co_sleep_ns_state) {
-        qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
-    }
+    qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
 
     nbd_co_establish_connection_cancel(bs, false);
 
@@ -330,9 +328,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 
     s->state = NBD_CLIENT_QUIT;
     if (s->connection_co) {
-        if (s->connection_co_sleep_ns_state) {
-            qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
-        }
+        qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
         nbd_co_establish_connection_cancel(bs, true);
     }
     if (qemu_in_coroutine()) {
diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index 3f6f637e81..096eea3444 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -27,19 +27,22 @@ struct QemuCoSleepState {
 
 void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)
 {
-    /* Write of schedule protected by barrier write in aio_co_schedule */
-    const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled,
-                                           qemu_co_sleep_ns__scheduled, NULL);
+    if (sleep_state) {
+        /* Write of schedule protected by barrier write in aio_co_schedule */
+        const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled,
+                                               qemu_co_sleep_ns__scheduled, NULL);
 
-    assert(scheduled == qemu_co_sleep_ns__scheduled);
-    *sleep_state->user_state_pointer = NULL;
-    timer_del(&sleep_state->ts);
-    aio_co_wake(sleep_state->co);
+        assert(scheduled == qemu_co_sleep_ns__scheduled);
+        *sleep_state->user_state_pointer = NULL;
+        timer_del(&sleep_state->ts);
+        aio_co_wake(sleep_state->co);
+    }
 }
 
 static void co_sleep_cb(void *opaque)
 {
-    qemu_co_sleep_wake(opaque);
+    QemuCoSleepState **sleep_state = opaque;
+    qemu_co_sleep_wake(*sleep_state);
 }
 
 void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
@@ -60,7 +63,7 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
         abort();
     }
 
-    aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state);
+    aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, sleep_state);
     *sleep_state = &state;
     timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();
-- 
2.31.1




^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/6] coroutine-sleep: move timer out of QemuCoSleepState
  2021-05-03 11:25 [PATCH 0/6] coroutine: new sleep/wake API Paolo Bonzini
                   ` (2 preceding siblings ...)
  2021-05-03 11:25 ` [PATCH 3/6] coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing Paolo Bonzini
@ 2021-05-03 11:25 ` Paolo Bonzini
  2021-05-10 10:19   ` Vladimir Sementsov-Ogievskiy
  2021-05-03 11:25 ` [PATCH 5/6] coroutine-sleep: replace QemuCoSleepState pointer with struct in the API Paolo Bonzini
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2021-05-03 11:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: eesposit, stefanha

This simplification is enabled by the previous patch.  Now aio_co_wake
will only be called once, therefore we do not care about a spurious
firing of the timer after a qemu_co_sleep_wake.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-coroutine-sleep.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index 096eea3444..68e9505b2e 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -21,7 +21,6 @@ static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";
 
 struct QemuCoSleepState {
     Coroutine *co;
-    QEMUTimer ts;
     QemuCoSleepState **user_state_pointer;
 };
 
@@ -34,7 +33,6 @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)
 
         assert(scheduled == qemu_co_sleep_ns__scheduled);
         *sleep_state->user_state_pointer = NULL;
-        timer_del(&sleep_state->ts);
         aio_co_wake(sleep_state->co);
     }
 }
@@ -49,6 +47,7 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
                                             QemuCoSleepState **sleep_state)
 {
     AioContext *ctx = qemu_get_current_aio_context();
+    QEMUTimer ts;
     QemuCoSleepState state = {
         .co = qemu_coroutine_self(),
         .user_state_pointer = sleep_state,
@@ -63,10 +62,11 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
         abort();
     }
 
-    aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, sleep_state);
+    aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, sleep_state);
     *sleep_state = &state;
-    timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);
+    timer_mod(&ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();
+    timer_del(&ts);
 
     /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine.  */
     assert(*sleep_state == NULL);
-- 
2.31.1




^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/6] coroutine-sleep: replace QemuCoSleepState pointer with struct in the API
  2021-05-03 11:25 [PATCH 0/6] coroutine: new sleep/wake API Paolo Bonzini
                   ` (3 preceding siblings ...)
  2021-05-03 11:25 ` [PATCH 4/6] coroutine-sleep: move timer out of QemuCoSleepState Paolo Bonzini
@ 2021-05-03 11:25 ` Paolo Bonzini
  2021-05-10 10:38   ` Vladimir Sementsov-Ogievskiy
  2021-05-03 11:25 ` [PATCH 6/6] coroutine-sleep: introduce qemu_co_sleep Paolo Bonzini
  2021-05-11 14:07 ` [PATCH 0/6] coroutine: new sleep/wake API Stefan Hajnoczi
  6 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2021-05-03 11:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: eesposit, stefanha

Right now, users of qemu_co_sleep_ns_wakeable are passing
a pointer to QemuCoSleepState by reference to the function, but
QemuCoSleepState really is just a Coroutine*.  Making the
content of the struct public is just as efficient and lets us
skip the user_state_pointer indirection: the Coroutine* is
cleared directly, rather than the pointer to it.

Since the usage is changed, take the occasion to rename the
struct to QemuCoSleep.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/block-copy.c          |  8 +++----
 block/nbd.c                 | 10 ++++-----
 include/qemu/coroutine.h    | 22 +++++++++----------
 util/qemu-coroutine-sleep.c | 43 ++++++++++++++++---------------------
 4 files changed, 39 insertions(+), 44 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index f896dc56f2..c2e5090412 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -50,7 +50,7 @@ typedef struct BlockCopyCallState {
     /* State */
     int ret;
     bool finished;
-    QemuCoSleepState *sleep_state;
+    QemuCoSleep sleep;
     bool cancelled;
 
     /* OUT parameters */
@@ -625,8 +625,8 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
                 if (ns > 0) {
                     block_copy_task_end(task, -EAGAIN);
                     g_free(task);
-                    qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, ns,
-                                              &call_state->sleep_state);
+                    qemu_co_sleep_ns_wakeable(&call_state->sleep,
+                                              QEMU_CLOCK_REALTIME, ns);
                     continue;
                 }
             }
@@ -674,7 +674,7 @@ out:
 
 void block_copy_kick(BlockCopyCallState *call_state)
 {
-    qemu_co_sleep_wake(call_state->sleep_state);
+    qemu_co_sleep_wake(&call_state->sleep);
 }
 
 /*
diff --git a/block/nbd.c b/block/nbd.c
index 1c6315b168..616f9ae6c4 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -116,7 +116,7 @@ typedef struct BDRVNBDState {
     CoQueue free_sema;
     Coroutine *connection_co;
     Coroutine *teardown_co;
-    QemuCoSleepState *connection_co_sleep_ns_state;
+    QemuCoSleep reconnect_sleep;
     bool drained;
     bool wait_drained_end;
     int in_flight;
@@ -289,7 +289,7 @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs)
     BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
 
     s->drained = true;
-    qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
+    qemu_co_sleep_wake(&s->reconnect_sleep);
 
     nbd_co_establish_connection_cancel(bs, false);
 
@@ -328,7 +328,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 
     s->state = NBD_CLIENT_QUIT;
     if (s->connection_co) {
-        qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
+        qemu_co_sleep_wake(&s->reconnect_sleep);
         nbd_co_establish_connection_cancel(bs, true);
     }
     if (qemu_in_coroutine()) {
@@ -685,8 +685,8 @@ static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState *s)
             }
             bdrv_inc_in_flight(s->bs);
         } else {
-            qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, timeout,
-                                      &s->connection_co_sleep_ns_state);
+            qemu_co_sleep_ns_wakeable(&s->reconnect_sleep,
+                                      QEMU_CLOCK_REALTIME, timeout);
             if (s->drained) {
                 continue;
             }
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index c5d7742989..77cb8ce459 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -291,21 +291,21 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock);
  */
 void qemu_co_rwlock_unlock(CoRwlock *lock);
 
-typedef struct QemuCoSleepState QemuCoSleepState;
+typedef struct QemuCoSleep {
+    Coroutine *to_wake;
+} QemuCoSleep;
 
 /**
- * Yield the coroutine for a given duration. During this yield, @sleep_state
- * is set to an opaque pointer, which may be used for
- * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the
- * timer fires. Don't save the obtained value to other variables and don't call
- * qemu_co_sleep_wake from another aio context.
+ * Yield the coroutine for a given duration. During this yield, @w
+ * can be used with qemu_co_sleep_wake() to terminate the sleep.
  */
-void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
-                                            QemuCoSleepState **sleep_state);
+void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,
+                                            QEMUClockType type, int64_t ns);
+
 static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
 {
-    QemuCoSleepState *unused = NULL;
-    qemu_co_sleep_ns_wakeable(type, ns, &unused);
+    QemuCoSleep w = { 0 };
+    qemu_co_sleep_ns_wakeable(&w, type, ns);
 }
 
 /**
@@ -314,7 +314,7 @@ static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
  * qemu_co_sleep_ns() and should be checked to be non-NULL before calling
  * qemu_co_sleep_wake().
  */
-void qemu_co_sleep_wake(QemuCoSleepState *sleep_state);
+void qemu_co_sleep_wake(QemuCoSleep *w);
 
 /**
  * Yield until a file descriptor becomes readable
diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index 68e9505b2e..89c3b758c5 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -19,42 +19,37 @@
 
 static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";
 
-struct QemuCoSleepState {
+void qemu_co_sleep_wake(QemuCoSleep *w)
+{
     Coroutine *co;
-    QemuCoSleepState **user_state_pointer;
-};
 
-void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)
-{
-    if (sleep_state) {
+    co = w->to_wake;
+    w->to_wake = NULL;
+    if (co) {
         /* Write of schedule protected by barrier write in aio_co_schedule */
-        const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled,
-                                               qemu_co_sleep_ns__scheduled, NULL);
+        const char *scheduled = qatomic_cmpxchg(&co->scheduled,
+                                                qemu_co_sleep_ns__scheduled, NULL);
 
         assert(scheduled == qemu_co_sleep_ns__scheduled);
-        *sleep_state->user_state_pointer = NULL;
-        aio_co_wake(sleep_state->co);
+        aio_co_wake(co);
     }
 }
 
 static void co_sleep_cb(void *opaque)
 {
-    QemuCoSleepState **sleep_state = opaque;
-    qemu_co_sleep_wake(*sleep_state);
+    QemuCoSleep *w = opaque;
+    qemu_co_sleep_wake(w);
 }
 
-void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
-                                            QemuCoSleepState **sleep_state)
+void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,
+                                            QEMUClockType type, int64_t ns)
 {
+    Coroutine *co = qemu_coroutine_self();
     AioContext *ctx = qemu_get_current_aio_context();
     QEMUTimer ts;
-    QemuCoSleepState state = {
-        .co = qemu_coroutine_self(),
-        .user_state_pointer = sleep_state,
-    };
 
-    const char *scheduled = qatomic_cmpxchg(&state.co->scheduled, NULL,
-                                           qemu_co_sleep_ns__scheduled);
+    const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL,
+                                            qemu_co_sleep_ns__scheduled);
     if (scheduled) {
         fprintf(stderr,
                 "%s: Co-routine was already scheduled in '%s'\n",
@@ -62,12 +57,12 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
         abort();
     }
 
-    aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, sleep_state);
-    *sleep_state = &state;
+    w->to_wake = co;
+    aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w),
     timer_mod(&ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();
     timer_del(&ts);
 
-    /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine.  */
-    assert(*sleep_state == NULL);
+    /* w->to_wake is cleared before resuming this coroutine.  */
+    assert(w->to_wake == NULL);
 }
-- 
2.31.1




^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 6/6] coroutine-sleep: introduce qemu_co_sleep
  2021-05-03 11:25 [PATCH 0/6] coroutine: new sleep/wake API Paolo Bonzini
                   ` (4 preceding siblings ...)
  2021-05-03 11:25 ` [PATCH 5/6] coroutine-sleep: replace QemuCoSleepState pointer with struct in the API Paolo Bonzini
@ 2021-05-03 11:25 ` Paolo Bonzini
  2021-05-10 10:46   ` Vladimir Sementsov-Ogievskiy
  2021-05-11 14:07 ` [PATCH 0/6] coroutine: new sleep/wake API Stefan Hajnoczi
  6 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2021-05-03 11:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: eesposit, stefanha

Allow using QemuCoSleep to sleep forever until woken by qemu_co_sleep_wake.
This makes the logic of qemu_co_sleep_ns_wakeable easy to understand.

In the future we could introduce an API that can work even if the
sleep and wake happen from different threads.  For now, initializing
w->to_wake after timer_mod is fine because the timer can only fire in
the same AioContext.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/coroutine.h    |  5 +++++
 util/qemu-coroutine-sleep.c | 20 +++++++++++++-------
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 77cb8ce459..b53e9632b9 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -302,6 +302,11 @@ typedef struct QemuCoSleep {
 void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,
                                             QEMUClockType type, int64_t ns);
 
+/**
+ * Yield the coroutine until the next call to qemu_co_sleep_wake.
+ */
+void coroutine_fn qemu_co_sleep(QemuCoSleep *w);
+
 static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
 {
     QemuCoSleep w = { 0 };
diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index 89c3b758c5..df6edba2e4 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -41,12 +41,9 @@ static void co_sleep_cb(void *opaque)
     qemu_co_sleep_wake(w);
 }
 
-void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,
-                                            QEMUClockType type, int64_t ns)
+void coroutine_fn qemu_co_sleep(QemuCoSleep *w)
 {
     Coroutine *co = qemu_coroutine_self();
-    AioContext *ctx = qemu_get_current_aio_context();
-    QEMUTimer ts;
 
     const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL,
                                             qemu_co_sleep_ns__scheduled);
@@ -58,11 +55,20 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,
     }
 
     w->to_wake = co;
-    aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w),
-    timer_mod(&ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();
-    timer_del(&ts);
 
     /* w->to_wake is cleared before resuming this coroutine.  */
     assert(w->to_wake == NULL);
 }
+
+void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,
+                                            QEMUClockType type, int64_t ns)
+{
+    AioContext *ctx = qemu_get_current_aio_context();
+    QEMUTimer ts;
+
+    aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w);
+    timer_mod(&ts, qemu_clock_get_ns(type) + ns);
+    qemu_co_sleep(w);
+    timer_del(&ts);
+}
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/6] coroutine-sleep: use a stack-allocated timer
  2021-05-03 11:25 ` [PATCH 1/6] coroutine-sleep: use a stack-allocated timer Paolo Bonzini
@ 2021-05-10 10:00   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-10 10:00 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: eesposit, stefanha

03.05.2021 14:25, Paolo Bonzini wrote:
> The lifetime of the timer is well-known (it cannot outlive
> qemu_co_sleep_ns_wakeable, because it's deleted by the time the
> coroutine resumes), so it is not necessary to place it on the heap.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/6] coroutine-sleep: disallow NULL QemuCoSleepState** argument
  2021-05-03 11:25 ` [PATCH 2/6] coroutine-sleep: disallow NULL QemuCoSleepState** argument Paolo Bonzini
@ 2021-05-10 10:03   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-10 10:03 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: eesposit, stefanha

03.05.2021 14:25, Paolo Bonzini wrote:
> Simplify the code by removing conditionals.  qemu_co_sleep_ns
> can simply use point the argument to an on-stack temporary.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/qemu/coroutine.h    |  5 +++--
>   util/qemu-coroutine-sleep.c | 18 +++++-------------
>   2 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index ce5b9c6851..c5d7742989 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -295,7 +295,7 @@ typedef struct QemuCoSleepState QemuCoSleepState;
>   
>   /**
>    * Yield the coroutine for a given duration. During this yield, @sleep_state
> - * (if not NULL) is set to an opaque pointer, which may be used for
> + * is set to an opaque pointer, which may be used for
>    * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the
>    * timer fires. Don't save the obtained value to other variables and don't call
>    * qemu_co_sleep_wake from another aio context.
> @@ -304,7 +304,8 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
>                                               QemuCoSleepState **sleep_state);
>   static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
>   {
> -    qemu_co_sleep_ns_wakeable(type, ns, NULL);
> +    QemuCoSleepState *unused = NULL;
> +    qemu_co_sleep_ns_wakeable(type, ns, &unused);
>   }
>   
>   /**
> diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
> index eec6e81f3f..3f6f637e81 100644
> --- a/util/qemu-coroutine-sleep.c
> +++ b/util/qemu-coroutine-sleep.c
> @@ -32,9 +32,7 @@ void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)
>                                              qemu_co_sleep_ns__scheduled, NULL);
>   
>       assert(scheduled == qemu_co_sleep_ns__scheduled);
> -    if (sleep_state->user_state_pointer) {
> -        *sleep_state->user_state_pointer = NULL;
> -    }
> +    *sleep_state->user_state_pointer = NULL;
>       timer_del(&sleep_state->ts);
>       aio_co_wake(sleep_state->co);
>   }
> @@ -63,16 +61,10 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
>       }
>   
>       aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state);
> -    if (sleep_state) {
> -        *sleep_state = &state;
> -    }
> +    *sleep_state = &state;
>       timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);
>       qemu_coroutine_yield();
> -    if (sleep_state) {
> -        /*
> -         * Note that *sleep_state is cleared during qemu_co_sleep_wake
> -         * before resuming this coroutine.
> -         */
> -        assert(*sleep_state == NULL);
> -    }
> +
> +    /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine.  */

over-80 line

> +    assert(*sleep_state == NULL);
>   }
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/6] coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing
  2021-05-03 11:25 ` [PATCH 3/6] coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing Paolo Bonzini
@ 2021-05-10 10:15   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-10 10:15 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: eesposit, stefanha

03.05.2021 14:25, Paolo Bonzini wrote:
> All callers of qemu_co_sleep_wake are checking whether they are passing
> a NULL argument inside the pointer-to-pointer: do the check in
> qemu_co_sleep_wake itself.
> 
> As a side effect, qemu_co_sleep_wake can be called more than once and it
> will only wake the coroutine once; after the first time, the argument
> will be set to NULL via *sleep_state->user_state_pointer.
> 

commit message say nothing about change in co_sleep_cb: it now passes the user pointer too.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/block-copy.c          |  4 +---
>   block/nbd.c                 |  8 ++------
>   util/qemu-coroutine-sleep.c | 21 ++++++++++++---------
>   3 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 9b4af00614..f896dc56f2 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -674,9 +674,7 @@ out:
>   
>   void block_copy_kick(BlockCopyCallState *call_state)
>   {
> -    if (call_state->sleep_state) {
> -        qemu_co_sleep_wake(call_state->sleep_state);
> -    }
> +    qemu_co_sleep_wake(call_state->sleep_state);
>   }
>   
>   /*
> diff --git a/block/nbd.c b/block/nbd.c
> index 1d4668d42d..1c6315b168 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -289,9 +289,7 @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs)
>       BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
>   
>       s->drained = true;
> -    if (s->connection_co_sleep_ns_state) {
> -        qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
> -    }
> +    qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
>   
>       nbd_co_establish_connection_cancel(bs, false);
>   
> @@ -330,9 +328,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>   
>       s->state = NBD_CLIENT_QUIT;
>       if (s->connection_co) {
> -        if (s->connection_co_sleep_ns_state) {
> -            qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
> -        }
> +        qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
>           nbd_co_establish_connection_cancel(bs, true);
>       }
>       if (qemu_in_coroutine()) {
> diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
> index 3f6f637e81..096eea3444 100644
> --- a/util/qemu-coroutine-sleep.c
> +++ b/util/qemu-coroutine-sleep.c
> @@ -27,19 +27,22 @@ struct QemuCoSleepState {
>   
>   void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)
>   {
> -    /* Write of schedule protected by barrier write in aio_co_schedule */
> -    const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled,
> -                                           qemu_co_sleep_ns__scheduled, NULL);
> +    if (sleep_state) {
> +        /* Write of schedule protected by barrier write in aio_co_schedule */
> +        const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled,
> +                                               qemu_co_sleep_ns__scheduled, NULL);

over-80 line (and indentation is broken)

>   
> -    assert(scheduled == qemu_co_sleep_ns__scheduled);
> -    *sleep_state->user_state_pointer = NULL;
> -    timer_del(&sleep_state->ts);
> -    aio_co_wake(sleep_state->co);
> +        assert(scheduled == qemu_co_sleep_ns__scheduled);
> +        *sleep_state->user_state_pointer = NULL;
> +        timer_del(&sleep_state->ts);
> +        aio_co_wake(sleep_state->co);
> +    }
>   }
>   
>   static void co_sleep_cb(void *opaque)
>   {
> -    qemu_co_sleep_wake(opaque);
> +    QemuCoSleepState **sleep_state = opaque;
> +    qemu_co_sleep_wake(*sleep_state);
>   }
>   
>   void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
> @@ -60,7 +63,7 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
>           abort();
>       }
>   
> -    aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, &state);
> +    aio_timer_init(ctx, &state.ts, type, SCALE_NS, co_sleep_cb, sleep_state);
>       *sleep_state = &state;
>       timer_mod(&state.ts, qemu_clock_get_ns(type) + ns);
>       qemu_coroutine_yield();
> 

Patch looks OK itself:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

(still, please improve commit message).

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/6] coroutine-sleep: move timer out of QemuCoSleepState
  2021-05-03 11:25 ` [PATCH 4/6] coroutine-sleep: move timer out of QemuCoSleepState Paolo Bonzini
@ 2021-05-10 10:19   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-10 10:19 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: eesposit, stefanha

03.05.2021 14:25, Paolo Bonzini wrote:
> This simplification is enabled by the previous patch.  Now aio_co_wake
> will only be called once, therefore we do not care about a spurious
> firing of the timer after a qemu_co_sleep_wake.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/6] coroutine-sleep: replace QemuCoSleepState pointer with struct in the API
  2021-05-03 11:25 ` [PATCH 5/6] coroutine-sleep: replace QemuCoSleepState pointer with struct in the API Paolo Bonzini
@ 2021-05-10 10:38   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-10 10:38 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: eesposit, stefanha

03.05.2021 14:25, Paolo Bonzini wrote:
> Right now, users of qemu_co_sleep_ns_wakeable are passing
> a pointer to QemuCoSleepState by reference to the function, but
> QemuCoSleepState really is just a Coroutine*.  Making the
> content of the struct public is just as efficient and lets us
> skip the user_state_pointer indirection: the Coroutine* is
> cleared directly, rather than the pointer to it.
> 
> Since the usage is changed, take the occasion to rename the
> struct to QemuCoSleep.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/block-copy.c          |  8 +++----
>   block/nbd.c                 | 10 ++++-----
>   include/qemu/coroutine.h    | 22 +++++++++----------
>   util/qemu-coroutine-sleep.c | 43 ++++++++++++++++---------------------
>   4 files changed, 39 insertions(+), 44 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index f896dc56f2..c2e5090412 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -50,7 +50,7 @@ typedef struct BlockCopyCallState {
>       /* State */
>       int ret;
>       bool finished;
> -    QemuCoSleepState *sleep_state;
> +    QemuCoSleep sleep;
>       bool cancelled;
>   
>       /* OUT parameters */
> @@ -625,8 +625,8 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
>                   if (ns > 0) {
>                       block_copy_task_end(task, -EAGAIN);
>                       g_free(task);
> -                    qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, ns,
> -                                              &call_state->sleep_state);
> +                    qemu_co_sleep_ns_wakeable(&call_state->sleep,
> +                                              QEMU_CLOCK_REALTIME, ns);
>                       continue;
>                   }
>               }
> @@ -674,7 +674,7 @@ out:
>   
>   void block_copy_kick(BlockCopyCallState *call_state)
>   {
> -    qemu_co_sleep_wake(call_state->sleep_state);
> +    qemu_co_sleep_wake(&call_state->sleep);
>   }
>   
>   /*
> diff --git a/block/nbd.c b/block/nbd.c
> index 1c6315b168..616f9ae6c4 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -116,7 +116,7 @@ typedef struct BDRVNBDState {
>       CoQueue free_sema;
>       Coroutine *connection_co;
>       Coroutine *teardown_co;
> -    QemuCoSleepState *connection_co_sleep_ns_state;
> +    QemuCoSleep reconnect_sleep;
>       bool drained;
>       bool wait_drained_end;
>       int in_flight;
> @@ -289,7 +289,7 @@ static void coroutine_fn nbd_client_co_drain_begin(BlockDriverState *bs)
>       BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
>   
>       s->drained = true;
> -    qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
> +    qemu_co_sleep_wake(&s->reconnect_sleep);
>   
>       nbd_co_establish_connection_cancel(bs, false);
>   
> @@ -328,7 +328,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
>   
>       s->state = NBD_CLIENT_QUIT;
>       if (s->connection_co) {
> -        qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
> +        qemu_co_sleep_wake(&s->reconnect_sleep);
>           nbd_co_establish_connection_cancel(bs, true);
>       }
>       if (qemu_in_coroutine()) {
> @@ -685,8 +685,8 @@ static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState *s)
>               }
>               bdrv_inc_in_flight(s->bs);
>           } else {
> -            qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, timeout,
> -                                      &s->connection_co_sleep_ns_state);
> +            qemu_co_sleep_ns_wakeable(&s->reconnect_sleep,
> +                                      QEMU_CLOCK_REALTIME, timeout);
>               if (s->drained) {
>                   continue;
>               }
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index c5d7742989..77cb8ce459 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -291,21 +291,21 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock);
>    */
>   void qemu_co_rwlock_unlock(CoRwlock *lock);
>   
> -typedef struct QemuCoSleepState QemuCoSleepState;
> +typedef struct QemuCoSleep {
> +    Coroutine *to_wake;
> +} QemuCoSleep;
>   
>   /**
> - * Yield the coroutine for a given duration. During this yield, @sleep_state
> - * is set to an opaque pointer, which may be used for
> - * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the
> - * timer fires. Don't save the obtained value to other variables and don't call
> - * qemu_co_sleep_wake from another aio context.
> + * Yield the coroutine for a given duration. During this yield, @w
> + * can be used with qemu_co_sleep_wake() to terminate the sleep.

I'd add that function initializes @w.

>    */
> -void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
> -                                            QemuCoSleepState **sleep_state);
> +void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,
> +                                            QEMUClockType type, int64_t ns);
> +
>   static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
>   {
> -    QemuCoSleepState *unused = NULL;
> -    qemu_co_sleep_ns_wakeable(type, ns, &unused);
> +    QemuCoSleep w = { 0 };
> +    qemu_co_sleep_ns_wakeable(&w, type, ns);
>   }
>   
>   /**
> @@ -314,7 +314,7 @@ static inline void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
>    * qemu_co_sleep_ns() and should be checked to be non-NULL before calling
>    * qemu_co_sleep_wake().
>    */
> -void qemu_co_sleep_wake(QemuCoSleepState *sleep_state);
> +void qemu_co_sleep_wake(QemuCoSleep *w);
>   
>   /**
>    * Yield until a file descriptor becomes readable
> diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
> index 68e9505b2e..89c3b758c5 100644
> --- a/util/qemu-coroutine-sleep.c
> +++ b/util/qemu-coroutine-sleep.c
> @@ -19,42 +19,37 @@
>   
>   static const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";
>   
> -struct QemuCoSleepState {
> +void qemu_co_sleep_wake(QemuCoSleep *w)
> +{
>       Coroutine *co;
> -    QemuCoSleepState **user_state_pointer;
> -};
>   
> -void qemu_co_sleep_wake(QemuCoSleepState *sleep_state)
> -{
> -    if (sleep_state) {
> +    co = w->to_wake;
> +    w->to_wake = NULL;

Don't you like g_steal_pointer()?

> +    if (co) {
>           /* Write of schedule protected by barrier write in aio_co_schedule */
> -        const char *scheduled = qatomic_cmpxchg(&sleep_state->co->scheduled,
> -                                               qemu_co_sleep_ns__scheduled, NULL);
> +        const char *scheduled = qatomic_cmpxchg(&co->scheduled,
> +                                                qemu_co_sleep_ns__scheduled, NULL);

indentation fixed, but not over-80 line )

>   
>           assert(scheduled == qemu_co_sleep_ns__scheduled);
> -        *sleep_state->user_state_pointer = NULL;
> -        aio_co_wake(sleep_state->co);
> +        aio_co_wake(co);
>       }
>   }
>   
>   static void co_sleep_cb(void *opaque)
>   {
> -    QemuCoSleepState **sleep_state = opaque;
> -    qemu_co_sleep_wake(*sleep_state);
> +    QemuCoSleep *w = opaque;
> +    qemu_co_sleep_wake(w);
>   }
>   
> -void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
> -                                            QemuCoSleepState **sleep_state)
> +void coroutine_fn qemu_co_sleep_ns_wakeable(QemuCoSleep *w,
> +                                            QEMUClockType type, int64_t ns)
>   {
> +    Coroutine *co = qemu_coroutine_self();
>       AioContext *ctx = qemu_get_current_aio_context();
>       QEMUTimer ts;
> -    QemuCoSleepState state = {
> -        .co = qemu_coroutine_self(),
> -        .user_state_pointer = sleep_state,
> -    };
>   
> -    const char *scheduled = qatomic_cmpxchg(&state.co->scheduled, NULL,
> -                                           qemu_co_sleep_ns__scheduled);
> +    const char *scheduled = qatomic_cmpxchg(&co->scheduled, NULL,
> +                                            qemu_co_sleep_ns__scheduled);

indentation fixed..

>       if (scheduled) {
>           fprintf(stderr,
>                   "%s: Co-routine was already scheduled in '%s'\n",
> @@ -62,12 +57,12 @@ void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
>           abort();
>       }
>   
> -    aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, sleep_state);
> -    *sleep_state = &state;
> +    w->to_wake = co;
> +    aio_timer_init(ctx, &ts, type, SCALE_NS, co_sleep_cb, w),
>       timer_mod(&ts, qemu_clock_get_ns(type) + ns);
>       qemu_coroutine_yield();
>       timer_del(&ts);
>   
> -    /* qemu_co_sleep_wake clears *sleep_state before resuming this coroutine.  */
> -    assert(*sleep_state == NULL);
> +    /* w->to_wake is cleared before resuming this coroutine.  */
> +    assert(w->to_wake == NULL);
>   }
> 

with indentations and over-80 line fixed in appropriate previous patch:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 6/6] coroutine-sleep: introduce qemu_co_sleep
  2021-05-03 11:25 ` [PATCH 6/6] coroutine-sleep: introduce qemu_co_sleep Paolo Bonzini
@ 2021-05-10 10:46   ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-10 10:46 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: eesposit, stefanha

03.05.2021 14:25, Paolo Bonzini wrote:
> Allow using QemuCoSleep to sleep forever until woken by qemu_co_sleep_wake.
> This makes the logic of qemu_co_sleep_ns_wakeable easy to understand.
> 
> In the future we could introduce an API that can work even if the
> sleep and wake happen from different threads.  For now, initializing
> w->to_wake after timer_mod is fine because the timer can only fire in
> the same AioContext.

worth a comment inside qemu_co_sleep_ns_wakeable() ?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/6] coroutine: new sleep/wake API
  2021-05-03 11:25 [PATCH 0/6] coroutine: new sleep/wake API Paolo Bonzini
                   ` (5 preceding siblings ...)
  2021-05-03 11:25 ` [PATCH 6/6] coroutine-sleep: introduce qemu_co_sleep Paolo Bonzini
@ 2021-05-11 14:07 ` Stefan Hajnoczi
  6 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2021-05-11 14:07 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: eesposit, qemu-devel

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

On Mon, May 03, 2021 at 01:25:44PM +0200, Paolo Bonzini wrote:
> This is a revamp of the qemu_co_sleep* API that makes it easier to
> extend the API: the state that is needed to wake up a coroutine is now
> part of the public API instead of hidden behind a pointer-to-pointer;
> the API is made more extensible by pushing the rest of QemuCoSleepState
> into local variables.
> 
> In the future, this will be extended to introduce a prepare/sleep/cancel
> API similar to Linux's prepare_to_wait/schedule/finish_wait functions.
> For now, this is just a nice refactoring.
> 
> Paolo Bonzini (6):
>   coroutine-sleep: use a stack-allocated timer
>   coroutine-sleep: disallow NULL QemuCoSleepState** argument
>   coroutine-sleep: allow qemu_co_sleep_wake(NULL)
>   coroutine-sleep: move timer out of QemuCoSleepState
>   coroutine-sleep: replace QemuCoSleepState pointer with struct in the
>     API
>   coroutine-sleep: introduce qemu_co_sleep
> 
>  block/block-copy.c          | 10 +++---
>  block/nbd.c                 | 14 +++-----
>  include/qemu/coroutine.h    | 26 ++++++++------
>  util/qemu-coroutine-sleep.c | 69 +++++++++++++++++--------------------
>  4 files changed, 57 insertions(+), 62 deletions(-)
> 
> -- 
> 2.31.1
> 

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-05-11 14:09 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-03 11:25 [PATCH 0/6] coroutine: new sleep/wake API Paolo Bonzini
2021-05-03 11:25 ` [PATCH 1/6] coroutine-sleep: use a stack-allocated timer Paolo Bonzini
2021-05-10 10:00   ` Vladimir Sementsov-Ogievskiy
2021-05-03 11:25 ` [PATCH 2/6] coroutine-sleep: disallow NULL QemuCoSleepState** argument Paolo Bonzini
2021-05-10 10:03   ` Vladimir Sementsov-Ogievskiy
2021-05-03 11:25 ` [PATCH 3/6] coroutine-sleep: allow qemu_co_sleep_wake that wakes nothing Paolo Bonzini
2021-05-10 10:15   ` Vladimir Sementsov-Ogievskiy
2021-05-03 11:25 ` [PATCH 4/6] coroutine-sleep: move timer out of QemuCoSleepState Paolo Bonzini
2021-05-10 10:19   ` Vladimir Sementsov-Ogievskiy
2021-05-03 11:25 ` [PATCH 5/6] coroutine-sleep: replace QemuCoSleepState pointer with struct in the API Paolo Bonzini
2021-05-10 10:38   ` Vladimir Sementsov-Ogievskiy
2021-05-03 11:25 ` [PATCH 6/6] coroutine-sleep: introduce qemu_co_sleep Paolo Bonzini
2021-05-10 10:46   ` Vladimir Sementsov-Ogievskiy
2021-05-11 14:07 ` [PATCH 0/6] coroutine: new sleep/wake API Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.