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