Re: [Qemu-devel] [PATCH v8 1/3] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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