Re: [Qemu-devel] [PATCH v6 5/7] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Vladimir Sementsov-Ogievskiy]

07.06.2019 18:52, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2019 16:02, Kevin Wolf wrote:
>> Am 07.06.2019 um 13:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> 07.06.2019 10:57, Kevin Wolf wrote:
>>>> Am 11.04.2019 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> Introduce a function to gracefully wake-up a coroutine, sleeping in
>>>>> qemu_co_sleep_ns() sleep.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>>
>>>> You can simply reenter the coroutine while it has yielded in
>>>> qemu_co_sleep_ns(). This is supported.
>>>
>>> No it doesn't. qemu_aio_coroutine_enter checks for scheduled field,
>>> and aborts if it is set.
>>
>> Ah, yes, it has been broken since commit
>>
>> I actually tried to fix it once, but it turned out more complicated and
>> I think we found a different solution for the problem at hand:
>>
>>      Subject: [PATCH for-2.11 0/4] Fix qemu-iotests failures
>>      Message-Id: <address@hidden>
>>
>> In this case, I guess your approach with a new function to interrupt
>> qemu_co_sleep_ns() is okay.
>>
>> Do we need to timer_del() when taking the shortcut? We don't necessarily
>> reenter the coroutine immediately, but might only be scheduling it. In
>> this case, the timer could fire before qemu_co_sleep_ns() has run and
>> schedule the coroutine a second time
> 
> No it will not, as we do cmpxchg, scheduled to NULL, so second call will do
> nothing..
> 
> But it seems unsafe, as even coroutine pointer may be stale when we call
> qemu_co_sleep_wake second time. So, we possibly should remove timer, but ..
> 
>   (ignoring co->scheduled again -
>> maybe we should actually not do that in the timer callback path, but
>> instead let it run into the assertion because it would be a bug for the
>> timer callback to end up in this situation).
>>
>> Kevin
>>
> 
> Interesting, could there be a race condition, when we call qemu_co_sleep_wake,
> but co_sleep_cb already scheduled in some queue and will run soon? Then 
> removing
> the timer will not help.
> 
> 

Hmm, it's commented that timer_del is thread-safe..

Hmm, so, if anyway want to return Timer pointer from qemu_co_sleep_ns, may be 
it's better
to just call timer_mod(ts, 0) to shorten waiting instead of cheating with 
.scheduled?


-- 
Best regards,
Vladimir