[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] console: make QMP screendump use coroutine
From: |
Markus Armbruster |
Subject: |
Re: [PATCH] console: make QMP screendump use coroutine |
Date: |
Mon, 02 Mar 2020 15:22:13 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> Hi
>
> On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster <address@hidden> wrote:
>>
>> Kevin Wolf <address@hidden> 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.
[...]
- Re: [PATCH] console: make QMP screendump use coroutine,
Markus Armbruster <=
- Re: [PATCH] console: make QMP screendump use coroutine, Kevin Wolf, 2020/03/02
- Re: [PATCH] console: make QMP screendump use coroutine, Markus Armbruster, 2020/03/03
- Re: [PATCH] console: make QMP screendump use coroutine, Marc-André Lureau, 2020/03/03
- Re: [PATCH] console: make QMP screendump use coroutine, Markus Armbruster, 2020/03/06
- Re: [PATCH] console: make QMP screendump use coroutine, Marc-André Lureau, 2020/03/06
- Re: [PATCH] console: make QMP screendump use coroutine, Markus Armbruster, 2020/03/11