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!
