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

[Kevin Wolf]

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.

Kevin