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

[Kevin Wolf <kwolf@redhat.com>]

Am 06.03.2020 um 08:25 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 05.03.2020 um 16:30 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > Am 22.01.2020 um 07:32 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <kwolf@redhat.com> 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.

There is one exception, actually: Obviously, human-monitor-command must
allow HMP commands to run in parallel.

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

monitor_init_hmp() is called from (based on my block branch):

* monitor_init(): This is interactive.
* qemu_chr_new_noreplay(): Interactive, too.
* gdbserver_start(): Non-interactive.

There is also qmp_human_monitor_command(), which manually creates a
MonitorHMP without going through monitor_init_hmp(). It does call
monitor_data_init(), though. There are no additional callers of
monitor_data_init() and I think I would have added it to every creation
of a Monitor object when I did the QMP/HMP split of the struct.

So GDB and human-monitor-command should be the two non-interactive
cases.

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

I think you can send HMP commands from within gdb with it:

(gdb) tar rem:1234
Remote debugging using :1234
[...]
(gdb) monitor info block
ide1-cd0: [not inserted]
    Attached to:      /machine/unattached/device[23]
    Removable device: not locked, tray closed

floppy0: [not inserted]
    Attached to:      /machine/unattached/device[16]
    Removable device: not locked, tray closed

sd0: [not inserted]
    Removable device: not locked, tray closed

So we do want stop it from processing requests while a QMP command is
running.

Kevin