[Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()

[Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()

[Eric Blake]

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

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()

[Marc-André Lureau]

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

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]