Marc-André Lureau <marcandre.lur...@gmail.com> writes: > Hi > > On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster <arm...@redhat.com> wrote: >> >> Kevin Wolf <kw...@redhat.com> writes: >> >> > Am 20.02.2020 um 17:01 hat Markus Armbruster geschrieben: >> >> >> > void qmp_screendump(const char *filename, bool has_device, const >> >> >> > char *device, >> >> >> > bool has_head, int64_t head, Error **errp) >> >> >> > { >> >> >> > QemuConsole *con; >> >> >> > DisplaySurface *surface; >> >> >> > + g_autoptr(pixman_image_t) image = NULL; >> >> >> > int fd; >> >> >> > >> >> >> > if (has_device) { >> >> >> > @@ -365,7 +375,15 @@ void qmp_screendump(const char *filename, bool >> >> >> > has_device, const char *device, >> >> >> > } >> >> >> > } >> >> >> > >> >> >> > - graphic_hw_update(con); >> >> >> > + if (qemu_in_coroutine()) { >> >> >> > + assert(!con->screendump_co); >> >> >> > + con->screendump_co = qemu_coroutine_self(); >> >> >> > + aio_bh_schedule_oneshot(qemu_get_aio_context(), >> >> >> > + graphic_hw_update_bh, con); >> >> >> > + qemu_coroutine_yield(); >> >> >> > + con->screendump_co = NULL; >> >> >> > + } >> >> >> >> >> >> What if multiple QMP monitors simultaneously screendump? Hmm, it works >> >> >> because all execute one after another in the same coroutine >> >> >> qmp_dispatcher_co. Implicit mutual exclusion. >> >> >> >> >> >> Executing them one after another is bad, because it lets an ill-behaved >> >> >> QMP command starve *all* QMP monitors. We do it only out of >> >> >> (reasonable!) fear of implicit mutual exclusion requirements like the >> >> >> one you add. >> >> >> >> >> >> Let's not add more if we can help it. >> >> > >> >> > The situation is not worse than the current blocking handling. >> >> >> >> Really? >> >> >> >> What makes executing multiple qmp_screendump() concurrently (in separate >> >> threads) or interleaved (in separate coroutines in the same thread) >> >> unsafe before this patch? >> > >> > QMP command handlers are guaranteed to run in the main thread with the >> > BQL held, so there is no concurrency. If you want to change this, you >> > would have much more complicated problems to solve than in this handler. >> > I'm not sure it's fair to require thread-safety from one handler when >> > no other handler is thread safe (except accidentally) and nobody seems >> > to plan actually calling them from multiple threads. >> >> "Let's not [...] if we can help it." is hardly a "change this or else no >> merge" demand. It is a challenge to find a more elegant solution. >> >> >> >> Your screendump_co is per QemuConsole instead of per QMP monitor only >> >> >> because you need to find the coroutine in graphic_hw_update_done(). >> >> >> Can >> >> >> we somehow pass it via function arguments? >> >> > >> >> > I think it could be done later, so I suggest a TODO. >> >> >> >> We should avoid making our dependence on implicit mutual exclusion >> >> worse. When we do it anyway, a big, fat, ugly comment is definitely >> >> called for. >> > >> > Anyway, what I really wanted to add: >> > >> > This should be easy to solve by having a CoQueue instead of a single >> >> Ah, challenge accepted! Exactly the outcome I was hoping for :) >> >> > Coroutine pointer. The coroutine would just call qemu_co_queue_wait(), >> > which adds itself to the queue before it yields and the update >> > completion would wake up all coroutines that are currently queued with >> > qemu_co_queue_restart_all(). >> > >> > qemu_co_queue_wait() takes a lock as its second parameter. You don't >> > need it in this context and can just pass NULL. (This is a lock that >> > would be dropped while the coroutine is sleeping and automatically >> > reacquired afterwards.) >> > >> >> >> In case avoiding the mutual exclusion is impractical: please explain it >> >> >> in a comment to make it somewhat less implicit. >> >> >> >> It is anything but: see appended patch. >> > >> > This works, too, but it requires an additional struct. I think the queue >> > is easier. (Note there is a difference in the mechanism: Your patch >> > waits for the specific update it triggered, while the CoQueue would wait >> > for _any_ update to complete. I assume effectively the result is the >> > same.) >> >> Your idea sounds much nicer to me. Thanks! > > Similar to the NULL check you asked to remove, > having a CoQueue there would lead to think that several concurrently > running screendump are possible. > > Is this a direction we are willing to take?
Let's take a step back. The actual problem is to find the coroutine in graphic_hw_update_done(), so you can wake it. Your solution stores the coroutine in the QemuConsole, because that's readily available in graphic_hw_update_done(). However, it really, really doesn't belong there, it belongs to the monitor. Works anyway only because QMP commands execute one after the other. Kevin suggested using a CoQueue to avoid this unspoken dependency. You object, because it could make readers assume multiple screendump commands could run concurrently, which is not the case. Alright, let's KISS: since there's just one main loop, there's just one coroutine: @qmp_dispatcher_co. Let's use that, so the dependency on "one command after the other" is explicit and obvious. [...]