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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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.

As long as running QMP commands from *all* monitors one after the other
is good enough, I can't see why running HMP commands interleaved is
worth the risk.

> 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.

I believe the non-"interactive" HMP code is used only by gdbstub.c.

I keep forgetting our GDB server stub creates an "HMP" monitor.
Possibly because I don't understand why.  Alex, Philippe, can you
enlighten me?

Aside: I figure the distribution of work between monitor_init(),
monitor_init_hmp() and monitor_init_qmp() could be improved.

>> 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