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

[Kevin Wolf]

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 <address@hidden>

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 <address@hidden>