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? fwiw, my earlier async series did allow that, and was using a queue for concurrent screendumps (but without coroutine & CoQueue, since it was all traditional callback/events-based) -- Marc-André Lureau