Re: [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands

[Kevin Wolf]

Am 05.03.2020 um 16:30 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 22.01.2020 um 07:32 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <address@hidden> writes:
> >> 
> >> > This patch adds a new 'coroutine' flag to QMP command definitions that
> >> > tells the QMP dispatcher that the command handler is safe to be run in a
> >> > coroutine.
> >> 
> >> I'm afraid I missed this question in my review of v3: when is a handler
> >> *not* safe to be run in a coroutine?
> >
> > That's a hard one to answer fully.
> >
> > Basically, I think the biggest problem is with calling functions that
> > change their behaviour if run in a coroutine compared to running them
> > outside of coroutine context. In most cases the differences like having
> > a nested event loop instead of yielding are just fine, but they are
> > still subtly different.
> >
> > I know this is vague, but I can assure you that problematic cases exist.
> > I hit one of them with my initial hack that just moved everything into a
> > coroutine. It was related to graph modifications and bdrv_drain and
> > resulted in a hang. For the specifics, I would have to try and reproduce
> > the problem again.
> 
> I'm afraid it's even more complicated.
> 
> Monitors (HMP and QMP) run in the main loop.  Before this patch, HMP and
> QMP commands run start to finish, one after the other.
> 
> After this patch, QMP commands may elect to yield.  QMP commands still
> run one after the other (the shared dispatcher ensures this even when we
> have multiple QMP monitors).
> 
> However, *HMP* commands can now run interleaved with a coroutine-enabled
> QMP command, i.e. between a yield and its re-enter.

I guess that's right. :-(

> Consider an HMP screendump running in the middle of a coroutine-enabled
> QMP screendump, using Marc-André's patch.  As far as I can tell, it
> works, because:
> 
> 1. HMP does not run in a coroutine.  If it did, and both dumps dumped
> the same @con, then it would overwrite con->screndump_co.  If we ever
> decide to make HMP coroutine-capable so it doesn't block the main loop,
> this will become unsafe in a nasty way.

At the same time, switching HMP to coroutines would give us an easy way
to fix the problem: Just use a CoMutex so that HMP and QMP commands
never run in parallel. Unless we actually _want_ to run both at the same
time for some commands, but I don't see why.

While we don't have this, maybe it's worth considering if there is
another simple way to achieve the same thing. Could QMP just suspend all
HMP monitors while it's executing a command? At first sight it seems
that this would work only for "interactive" monitors.

> 2. graphic_hw_update() is safe to call even while another
> graphic_hw_update() runs.  qxl_render_update() appears to ensure that
> with the help of qxl->ssd.lock.
> 
> 3. There is no 3[*].
> 
> My point is: this is a non-trivial argument.  Whether a QMP command
> handler is safe to run in a coroutine depends on how it interacts with
> all the stuff that can run interleaved with it.  Typically includes
> itself via its HMP buddy.

If the handler doesn't yield, it's still trivial. So I think my original
statement that with the existing QMP handlers, the problem is with code
that behaves different between coroutine and non-coroutine calls, is
still true because that is the only code that could possibly yield with
existing QMP command handlers.

Of course, you're right that handlers that actually can yield need to be
careful that they can deal with whatever happens until they are
reentered. And that seems to include HMP handlers.

Kevin