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

Eric Blake posted 1 patch 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171107223708.17935-1-eblake@redhat.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
There is a newer version of this series
include/qemu/coroutine.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
[Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()
Posted by Eric Blake 6 years, 4 months ago
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()
Posted by Marc-André Lureau 6 years, 4 months ago
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()
Posted by Stefan Hajnoczi 6 years, 4 months ago
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.
Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()
Posted by Paolo Bonzini 6 years, 4 months ago
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

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()
Posted by Eric Blake 6 years, 4 months ago
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

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()
Posted by Stefan Hajnoczi 6 years, 4 months ago
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
Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()
Posted by Eric Blake 6 years, 4 months ago
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

Re: [Qemu-devel] [PATCH] coroutine: Fix documentation of co_aio_sleep_ns()
Posted by Paolo Bonzini 6 years, 4 months ago
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
>