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 <vsement...@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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
