Re: [Qemu-devel] [PATCH v8 1/3] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5067 bytes --]

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>

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


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