include/qemu/coroutine.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
co_sleep_ns() was removed in commit 0b9caf9b, leaving behind a
stale comment. Update the documentation to match the current
usage of this function.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/qemu/coroutine.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 9aff9a735e..01ae415767 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -262,8 +262,11 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
/**
* Yield the coroutine for a given duration
*
- * Behaves similarly to co_sleep_ns(), but the sleeping coroutine will be
- * resumed when using aio_poll().
+ * This function uses timers and hence needs to know the event loop
+ * (#AioContext) to place the timer on. In any case, co_aio_sleep_ns()
+ * does not affect the #AioContext where the current coroutine is running,
+ * as the coroutine will restart on the same #AioContext that it is
+ * running on.
*/
void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type,
int64_t ns);
--
2.13.6
Hi On Tue, Nov 7, 2017 at 11:37 PM, Eric Blake <eblake@redhat.com> wrote: > co_sleep_ns() was removed in commit 0b9caf9b, leaving behind a > stale comment. Update the documentation to match the current > usage of this function. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/qemu/coroutine.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h > index 9aff9a735e..01ae415767 100644 > --- a/include/qemu/coroutine.h > +++ b/include/qemu/coroutine.h > @@ -262,8 +262,11 @@ void qemu_co_rwlock_unlock(CoRwlock *lock); > /** > * Yield the coroutine for a given duration > * > - * Behaves similarly to co_sleep_ns(), but the sleeping coroutine will be > - * resumed when using aio_poll(). > + * This function uses timers and hence needs to know the event loop > + * (#AioContext) to place the timer on. In any case, co_aio_sleep_ns() > + * does not affect the #AioContext where the current coroutine is running, > + * as the coroutine will restart on the same #AioContext that it is > + * running on. > */ comment seems correct to me, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type, > int64_t ns); > -- > 2.13.6 > > -- Marc-André Lureau
On Tue, Nov 07, 2017 at 04:37:08PM -0600, Eric Blake wrote: > co_sleep_ns() was removed in commit 0b9caf9b, leaving behind a > stale comment. Update the documentation to match the current > usage of this function. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/qemu/coroutine.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h > index 9aff9a735e..01ae415767 100644 > --- a/include/qemu/coroutine.h > +++ b/include/qemu/coroutine.h > @@ -262,8 +262,11 @@ void qemu_co_rwlock_unlock(CoRwlock *lock); > /** > * Yield the coroutine for a given duration > * > - * Behaves similarly to co_sleep_ns(), but the sleeping coroutine will be > - * resumed when using aio_poll(). > + * This function uses timers and hence needs to know the event loop > + * (#AioContext) to place the timer on. In any case, co_aio_sleep_ns() > + * does not affect the #AioContext where the current coroutine is running, > + * as the coroutine will restart on the same #AioContext that it is > + * running on. I cannot parse the second sentence. What does "affecting" an AioContext mean? Does "where the current coroutine is running" simply mean "the caller"? What is it trying to say? My guess is: the caller will be resumed in the current AioContext, not the timer's AioContext.
On 08/11/2017 16:42, Stefan Hajnoczi wrote: >> In any case, co_aio_sleep_ns() >> + * does not affect the #AioContext where the current coroutine is running, >> + * as the coroutine will restart on the same #AioContext that it is >> + * running on. > I cannot parse the second sentence. What does "affecting" an AioContext > mean? Does "where the current coroutine is running" simply mean "the > caller"? > > What is it trying to say? My guess is: the caller will be resumed in > the current AioContext, not the timer's AioContext. Yes, that is the intended meaning. Perhaps just s/current//. Paolo
On 11/08/2017 09:47 AM, Paolo Bonzini wrote: > On 08/11/2017 16:42, Stefan Hajnoczi wrote: >>> In any case, co_aio_sleep_ns() >>> + * does not affect the #AioContext where the current coroutine is running, >>> + * as the coroutine will restart on the same #AioContext that it is >>> + * running on. >> I cannot parse the second sentence. What does "affecting" an AioContext >> mean? Does "where the current coroutine is running" simply mean "the >> caller"? >> >> What is it trying to say? My guess is: the caller will be resumed in >> the current AioContext, not the timer's AioContext. > > Yes, that is the intended meaning. Perhaps just s/current//. How about: This function uses timers and hence needs to know the event loop (#AioContext) to place the timer on. After the time elapses, the current coroutine will restart with the same #AioContext it is currently running in, even if that is different than the timer context passed to co_aio_sleep_ns(). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Wed, Nov 08, 2017 at 09:57:47AM -0600, Eric Blake wrote: > On 11/08/2017 09:47 AM, Paolo Bonzini wrote: > > On 08/11/2017 16:42, Stefan Hajnoczi wrote: > >>> In any case, co_aio_sleep_ns() > >>> + * does not affect the #AioContext where the current coroutine is running, > >>> + * as the coroutine will restart on the same #AioContext that it is > >>> + * running on. > >> I cannot parse the second sentence. What does "affecting" an AioContext > >> mean? Does "where the current coroutine is running" simply mean "the > >> caller"? > >> > >> What is it trying to say? My guess is: the caller will be resumed in > >> the current AioContext, not the timer's AioContext. > > > > Yes, that is the intended meaning. Perhaps just s/current//. > > How about: > > This function uses timers and hence needs to know the event loop > (#AioContext) to place the timer on. After the time elapses, the > current coroutine will restart with the same #AioContext it is currently > running in, even if that is different than the timer context passed to > co_aio_sleep_ns(). These complicated semantics are a clue that the API should be simplified. QEMU has changed since this function was first introduced. Now we can do the following: void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) { AioContext *ctx = qemu_get_current_aio_context(); CoSleepCB sleep_cb = { .co = qemu_coroutine_self(), }; sleep_cb.ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &sleep_cb); timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns); qemu_coroutine_yield(); timer_del(sleep_cb.ts); timer_free(sleep_cb.ts); } I don't see a reason for the caller to pass in an AioContext. Any objections? Will send a patch if this is okay. Stefan
On 11/08/2017 11:36 AM, Stefan Hajnoczi wrote: >> This function uses timers and hence needs to know the event loop >> (#AioContext) to place the timer on. After the time elapses, the >> current coroutine will restart with the same #AioContext it is currently >> running in, even if that is different than the timer context passed to >> co_aio_sleep_ns(). > > These complicated semantics are a clue that the API should be > simplified. QEMU has changed since this function was first introduced. > Now we can do the following: > > void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) > { > AioContext *ctx = qemu_get_current_aio_context(); > CoSleepCB sleep_cb = { > .co = qemu_coroutine_self(), > }; > sleep_cb.ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &sleep_cb); > timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns); > qemu_coroutine_yield(); > timer_del(sleep_cb.ts); > timer_free(sleep_cb.ts); > } > > I don't see a reason for the caller to pass in an AioContext. > > Any objections? Will send a patch if this is okay. Makes sense to me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 08/11/2017 18:36, Stefan Hajnoczi wrote: > On Wed, Nov 08, 2017 at 09:57:47AM -0600, Eric Blake wrote: >> On 11/08/2017 09:47 AM, Paolo Bonzini wrote: >>> On 08/11/2017 16:42, Stefan Hajnoczi wrote: >>>>> In any case, co_aio_sleep_ns() >>>>> + * does not affect the #AioContext where the current coroutine is running, >>>>> + * as the coroutine will restart on the same #AioContext that it is >>>>> + * running on. >>>> I cannot parse the second sentence. What does "affecting" an AioContext >>>> mean? Does "where the current coroutine is running" simply mean "the >>>> caller"? >>>> >>>> What is it trying to say? My guess is: the caller will be resumed in >>>> the current AioContext, not the timer's AioContext. >>> >>> Yes, that is the intended meaning. Perhaps just s/current//. >> >> How about: >> >> This function uses timers and hence needs to know the event loop >> (#AioContext) to place the timer on. After the time elapses, the >> current coroutine will restart with the same #AioContext it is currently >> running in, even if that is different than the timer context passed to >> co_aio_sleep_ns(). > > These complicated semantics are a clue that the API should be > simplified. QEMU has changed since this function was first introduced. > Now we can do the following: > > void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) > { > AioContext *ctx = qemu_get_current_aio_context(); > CoSleepCB sleep_cb = { > .co = qemu_coroutine_self(), > }; > sleep_cb.ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &sleep_cb); > timer_mod(sleep_cb.ts, qemu_clock_get_ns(type) + ns); > qemu_coroutine_yield(); > timer_del(sleep_cb.ts); > timer_free(sleep_cb.ts); > } > > I don't see a reason for the caller to pass in an AioContext. That should work, yes. Paolo > > Any objections? Will send a patch if this is okay. > > Stefan >
© 2016 - 2024 Red Hat, Inc.