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(-)
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);
}
--
2.18.0
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
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>
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! -- Best regards, Vladimir
© 2016 - 2026 Red Hat, Inc.