[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v8 1/3] qemu-coroutine-sleep: introduce qemu_co_
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH v8 1/3] qemu-coroutine-sleep: introduce qemu_co_sleep_wake |
Date: |
Tue, 17 Sep 2019 15:50:04 +0000 |
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 <address@hidden>
>
> 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 <address@hidden>
>
Will resend with both things, thank you!
--
Best regards,
Vladimir