| Message ID | 20190821165215.61406-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | NBD reconnect | expand |
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>
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!
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); }
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(-)