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

[Vladimir Sementsov-Ogievskiy]

11.06.2019 11:53, Kevin Wolf wrote:
> Am 07.06.2019 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 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?
> 
> This is probably slower than timer_del() and directly entering the
> coroutine. Is there any advantage in using timer_mod()? I don't think
> messing with .scheduled is too bad as it's set in the function just
> below, so it pairs nicely enough.
> 

Ok, will try this variant too.


-- 
Best regards,
Vladimir