[v8,1/3] qemu-coroutine-sleep: introduce qemu_co_sleep_wake
diff mbox series

Message ID 20190821165215.61406-2-vsementsov@virtuozzo.com
State New
Headers show
Series
  • NBD reconnect
Related show

Commit Message

Vladimir Sementsov-Ogievskiy Aug. 21, 2019, 4:52 p.m. UTC
Introduce a function to gracefully wake a coroutine sleeping in
qemu_co_sleep_ns().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qemu/coroutine.h    | 17 ++++++++++++--
 block/null.c                |  2 +-
 block/sheepdog.c            |  2 +-
 tests/test-bdrv-drain.c     |  6 ++---
 tests/test-block-iothread.c |  2 +-
 util/qemu-coroutine-sleep.c | 47 +++++++++++++++++++++++++++----------
 6 files changed, 55 insertions(+), 21 deletions(-)

Comments

Eric Blake Aug. 21, 2019, 5:15 p.m. UTC | #1
On 8/21/19 11:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> Introduce a function to gracefully wake a coroutine sleeping in
> qemu_co_sleep_ns().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

I'd like a second reviewer on this, but I'll at least give it a spin.

>  include/qemu/coroutine.h    | 17 ++++++++++++--
>  block/null.c                |  2 +-
>  block/sheepdog.c            |  2 +-
>  tests/test-bdrv-drain.c     |  6 ++---
>  tests/test-block-iothread.c |  2 +-
>  util/qemu-coroutine-sleep.c | 47 +++++++++++++++++++++++++++----------
>  6 files changed, 55 insertions(+), 21 deletions(-)

This merely updates existing callers to pass in NULL for the new
argument.  I'd feel a lot better if one of the two tests/ changes also
added a test passing a non-NULL sleep_state, and demonstrated its use.

> 
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 9801e7f5a4..96780a4902 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -274,9 +274,22 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock);
>  void qemu_co_rwlock_unlock(CoRwlock *lock);
>  
>  /**
> - * Yield the coroutine for a given duration
> + * Yield the coroutine for a given duration. During this yield @sleep_state (if

s/yield/yield,/

> + * not NULL) is set to opaque pointer, which may be used for

s/to/to an/

> + * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when timer
> + * shoots. Don't save obtained value to other variables and don't call

s/when timer shoots/when the timer fires/

s/save/save/the/

> + * qemu_co_sleep_wake from another aio context.
>   */
> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
> +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns,
> +                                   void **sleep_state);
> +
> +/**
> + * Wake a coroutine if it is sleeping by qemu_co_sleep_ns. Timer will be

s/by/in/
s/Timer/The timer/

> + * deleted. @sleep_state must be the variable which address was given to

s/which/whose/

> + * qemu_co_sleep_ns() and should be checked to be non-NULL before calling
> + * qemu_co_sleep_wake().
> + */
> +void qemu_co_sleep_wake(void *sleep_state);
>  

> +++ b/util/qemu-coroutine-sleep.c
> @@ -17,31 +17,52 @@
>  #include "qemu/timer.h"
>  #include "block/aio.h"
>  
> -static void co_sleep_cb(void *opaque)
> -{
> -    Coroutine *co = opaque;
> +const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";

Why is this not marked static?

> +
> +typedef struct QemuCoSleepState {
> +    Coroutine *co;
> +    QEMUTimer *ts;
> +    void **user_state_pointer;
> +} QemuCoSleepState;
>  
> +void qemu_co_sleep_wake(void *sleep_state)
> +{
> +    QemuCoSleepState *s = (QemuCoSleepState *)sleep_state;

This is C; the cast is not necessary.

>      /* Write of schedule protected by barrier write in aio_co_schedule */
> -    atomic_set(&co->scheduled, NULL);
> -    aio_co_wake(co);
> +    const char *scheduled = atomic_cmpxchg(&s->co->scheduled,
> +                                           qemu_co_sleep_ns__scheduled, NULL);
> +
> +    assert(scheduled == qemu_co_sleep_ns__scheduled);
> +    if (s->user_state_pointer) {
> +        *s->user_state_pointer = NULL;
> +    }
> +    timer_del(s->ts);
> +    aio_co_wake(s->co);
>  }
>  
> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
> +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns,
> +                                   void **sleep_state)
>  {
>      AioContext *ctx = qemu_get_current_aio_context();
> -    QEMUTimer *ts;
> -    Coroutine *co = qemu_coroutine_self();
> +    QemuCoSleepState state = {
> +        .co = qemu_coroutine_self(),
> +        .ts = aio_timer_new(ctx, type, SCALE_NS, qemu_co_sleep_wake, &state),
> +        .user_state_pointer = sleep_state,
> +    };
>  
> -    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL, __func__);
> +    const char *scheduled = atomic_cmpxchg(&state.co->scheduled, NULL,
> +                                           qemu_co_sleep_ns__scheduled);
>      if (scheduled) {
>          fprintf(stderr,
>                  "%s: Co-routine was already scheduled in '%s'\n",
>                  __func__, scheduled);
>          abort();
>      }
> -    ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, co);
> -    timer_mod(ts, qemu_clock_get_ns(type) + ns);
> +
> +    if (sleep_state) {
> +        *sleep_state = &state;
> +    }
> +    timer_mod(state.ts, qemu_clock_get_ns(type) + ns);
>      qemu_coroutine_yield();
> -    timer_del(ts);
> -    timer_free(ts);
> +    timer_free(state.ts);
>  }

Grammar changes are trivial; and while it is not the most trivial of
patches, I at least follow what it is doing.

Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf Sept. 17, 2019, 3:27 p.m. UTC | #2
Am 21.08.2019 um 18:52 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Introduce a function to gracefully wake a coroutine sleeping in
> qemu_co_sleep_ns().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Hm, this has become a bit more complex with the new sleep_state, but
it's probably unavoidable even we need to timer_del() from the callback.

>  include/qemu/coroutine.h    | 17 ++++++++++++--
>  block/null.c                |  2 +-
>  block/sheepdog.c            |  2 +-
>  tests/test-bdrv-drain.c     |  6 ++---
>  tests/test-block-iothread.c |  2 +-
>  util/qemu-coroutine-sleep.c | 47 +++++++++++++++++++++++++++----------
>  6 files changed, 55 insertions(+), 21 deletions(-)
> 
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 9801e7f5a4..96780a4902 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -274,9 +274,22 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock);
>  void qemu_co_rwlock_unlock(CoRwlock *lock);
>  
>  /**
> - * Yield the coroutine for a given duration
> + * Yield the coroutine for a given duration. During this yield @sleep_state (if
> + * not NULL) is set to opaque pointer, which may be used for
> + * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when timer
> + * shoots. Don't save obtained value to other variables and don't call
> + * qemu_co_sleep_wake from another aio context.
>   */
> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
> +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns,
> +                                   void **sleep_state);

Let's make the typedef for QemuCoSleepState public so that we don't have
to use a void pointer here.

I wonder if it would make sense to rename this function and keep
qemu_co_sleep_ns() as a wrapper (maybe even just macro) so that most
callers don't have to add a NULL.

> +/**
> + * Wake a coroutine if it is sleeping by qemu_co_sleep_ns. Timer will be
> + * deleted. @sleep_state must be the variable which address was given to
> + * qemu_co_sleep_ns() and should be checked to be non-NULL before calling
> + * qemu_co_sleep_wake().
> + */
> +void qemu_co_sleep_wake(void *sleep_state);
>  
>  /**
>   * Yield until a file descriptor becomes readable

The actual implementation looks right to me, so with a public
QemuCoSleepState and optionally the wrapper:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Vladimir Sementsov-Ogievskiy Sept. 17, 2019, 3:50 p.m. UTC | #3
17.09.2019 18:27, Kevin Wolf wrote:
> Am 21.08.2019 um 18:52 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Introduce a function to gracefully wake a coroutine sleeping in
>> qemu_co_sleep_ns().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Hm, this has become a bit more complex with the new sleep_state, but
> it's probably unavoidable even we need to timer_del() from the callback.
> 
>>   include/qemu/coroutine.h    | 17 ++++++++++++--
>>   block/null.c                |  2 +-
>>   block/sheepdog.c            |  2 +-
>>   tests/test-bdrv-drain.c     |  6 ++---
>>   tests/test-block-iothread.c |  2 +-
>>   util/qemu-coroutine-sleep.c | 47 +++++++++++++++++++++++++++----------
>>   6 files changed, 55 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
>> index 9801e7f5a4..96780a4902 100644
>> --- a/include/qemu/coroutine.h
>> +++ b/include/qemu/coroutine.h
>> @@ -274,9 +274,22 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock);
>>   void qemu_co_rwlock_unlock(CoRwlock *lock);
>>   
>>   /**
>> - * Yield the coroutine for a given duration
>> + * Yield the coroutine for a given duration. During this yield @sleep_state (if
>> + * not NULL) is set to opaque pointer, which may be used for
>> + * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when timer
>> + * shoots. Don't save obtained value to other variables and don't call
>> + * qemu_co_sleep_wake from another aio context.
>>    */
>> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
>> +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns,
>> +                                   void **sleep_state);
> 
> Let's make the typedef for QemuCoSleepState public so that we don't have
> to use a void pointer here.
> 
> I wonder if it would make sense to rename this function and keep
> qemu_co_sleep_ns() as a wrapper (maybe even just macro) so that most
> callers don't have to add a NULL.

Sure. Strange that I didn't go that way and touched a lot of extra files.

> 
>> +/**
>> + * Wake a coroutine if it is sleeping by qemu_co_sleep_ns. Timer will be
>> + * deleted. @sleep_state must be the variable which address was given to
>> + * qemu_co_sleep_ns() and should be checked to be non-NULL before calling
>> + * qemu_co_sleep_wake().
>> + */
>> +void qemu_co_sleep_wake(void *sleep_state);
>>   
>>   /**
>>    * Yield until a file descriptor becomes readable
> 
> The actual implementation looks right to me, so with a public
> QemuCoSleepState and optionally the wrapper:
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 

Will resend with both things, thank you!

Patch
diff mbox series

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 9801e7f5a4..96780a4902 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -274,9 +274,22 @@  void qemu_co_rwlock_wrlock(CoRwlock *lock);
 void qemu_co_rwlock_unlock(CoRwlock *lock);
 
 /**
- * Yield the coroutine for a given duration
+ * Yield the coroutine for a given duration. During this yield @sleep_state (if
+ * not NULL) is set to opaque pointer, which may be used for
+ * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when timer
+ * shoots. Don't save obtained value to other variables and don't call
+ * qemu_co_sleep_wake from another aio context.
  */
-void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
+void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns,
+                                   void **sleep_state);
+
+/**
+ * Wake a coroutine if it is sleeping by qemu_co_sleep_ns. Timer will be
+ * deleted. @sleep_state must be the variable which address was given to
+ * qemu_co_sleep_ns() and should be checked to be non-NULL before calling
+ * qemu_co_sleep_wake().
+ */
+void qemu_co_sleep_wake(void *sleep_state);
 
 /**
  * Yield until a file descriptor becomes readable
diff --git a/block/null.c b/block/null.c
index 699aa295cb..1e3f26b07e 100644
--- a/block/null.c
+++ b/block/null.c
@@ -109,7 +109,7 @@  static coroutine_fn int null_co_common(BlockDriverState *bs)
     BDRVNullState *s = bs->opaque;
 
     if (s->latency_ns) {
-        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, s->latency_ns);
+        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, s->latency_ns, NULL);
     }
     return 0;
 }
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 773dfc6ab1..3a7ef2f209 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -743,7 +743,7 @@  static coroutine_fn void reconnect_to_sdog(void *opaque)
         if (s->fd < 0) {
             trace_sheepdog_reconnect_to_sdog();
             error_report_err(local_err);
-            qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000000ULL);
+            qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000000ULL, NULL);
         }
     };
 
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 374bef6bb2..2f53a7add5 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -43,7 +43,7 @@  static void coroutine_fn bdrv_test_co_drain_begin(BlockDriverState *bs)
     BDRVTestState *s = bs->opaque;
     s->drain_count++;
     if (s->sleep_in_drain_begin) {
-        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
+        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000, NULL);
     }
 }
 
@@ -74,7 +74,7 @@  static int coroutine_fn bdrv_test_co_preadv(BlockDriverState *bs,
      * it to complete. We need to sleep a while as bdrv_drain_invoke() comes
      * first and polls its result, too, but it shouldn't accidentally complete
      * this request yet. */
-    qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
+    qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000, NULL);
 
     if (s->bh_indirection_ctx) {
         aio_bh_schedule_oneshot(s->bh_indirection_ctx, co_reenter_bh,
@@ -829,7 +829,7 @@  static int coroutine_fn test_job_run(Job *job, Error **errp)
         /* Avoid job_sleep_ns() because it marks the job as !busy. We want to
          * emulate some actual activity (probably some I/O) here so that drain
          * has to wait for this activity to stop. */
-        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
+        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000, NULL);
 
         job_pause_point(&s->common.job);
     }
diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c
index 926577b1f9..a1ac5efcaa 100644
--- a/tests/test-block-iothread.c
+++ b/tests/test-block-iothread.c
@@ -381,7 +381,7 @@  static int coroutine_fn test_job_run(Job *job, Error **errp)
          * emulate some actual activity (probably some I/O) here so that the
          * drain involved in AioContext switches has to wait for this activity
          * to stop. */
-        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
+        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000, NULL);
 
         job_pause_point(&s->common.job);
     }
diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c
index 4bfdd30cbf..48a64bb8d8 100644
--- a/util/qemu-coroutine-sleep.c
+++ b/util/qemu-coroutine-sleep.c
@@ -17,31 +17,52 @@ 
 #include "qemu/timer.h"
 #include "block/aio.h"
 
-static void co_sleep_cb(void *opaque)
-{
-    Coroutine *co = opaque;
+const char *qemu_co_sleep_ns__scheduled = "qemu_co_sleep_ns";
+
+typedef struct QemuCoSleepState {
+    Coroutine *co;
+    QEMUTimer *ts;
+    void **user_state_pointer;
+} QemuCoSleepState;
 
+void qemu_co_sleep_wake(void *sleep_state)
+{
+    QemuCoSleepState *s = (QemuCoSleepState *)sleep_state;
     /* Write of schedule protected by barrier write in aio_co_schedule */
-    atomic_set(&co->scheduled, NULL);
-    aio_co_wake(co);
+    const char *scheduled = atomic_cmpxchg(&s->co->scheduled,
+                                           qemu_co_sleep_ns__scheduled, NULL);
+
+    assert(scheduled == qemu_co_sleep_ns__scheduled);
+    if (s->user_state_pointer) {
+        *s->user_state_pointer = NULL;
+    }
+    timer_del(s->ts);
+    aio_co_wake(s->co);
 }
 
-void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
+void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns,
+                                   void **sleep_state)
 {
     AioContext *ctx = qemu_get_current_aio_context();
-    QEMUTimer *ts;
-    Coroutine *co = qemu_coroutine_self();
+    QemuCoSleepState state = {
+        .co = qemu_coroutine_self(),
+        .ts = aio_timer_new(ctx, type, SCALE_NS, qemu_co_sleep_wake, &state),
+        .user_state_pointer = sleep_state,
+    };
 
-    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL, __func__);
+    const char *scheduled = atomic_cmpxchg(&state.co->scheduled, NULL,
+                                           qemu_co_sleep_ns__scheduled);
     if (scheduled) {
         fprintf(stderr,
                 "%s: Co-routine was already scheduled in '%s'\n",
                 __func__, scheduled);
         abort();
     }
-    ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, co);
-    timer_mod(ts, qemu_clock_get_ns(type) + ns);
+
+    if (sleep_state) {
+        *sleep_state = &state;
+    }
+    timer_mod(state.ts, qemu_clock_get_ns(type) + ns);
     qemu_coroutine_yield();
-    timer_del(ts);
-    timer_free(ts);
+    timer_free(state.ts);
 }