Kevin Wolf <kw...@redhat.com> writes: > Am 06.03.2020 um 08:25 hat Markus Armbruster geschrieben: >> Kevin Wolf <kw...@redhat.com> writes: >> >> > Am 05.03.2020 um 16:30 hat Markus Armbruster geschrieben: >> >> Kevin Wolf <kw...@redhat.com> writes: >> >> >> >> > Am 22.01.2020 um 07:32 hat Markus Armbruster geschrieben: >> >> >> Kevin Wolf <kw...@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.
Yes. >> > 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. Yes. >> 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 Argh! Any idea where we define GDB command "monitor"? > So we do want stop it from processing requests while a QMP command is > running. Then a slow QMP command can interfere with debugging. Perhaps we can synchronize just the "monitor" command.