Hi
On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> Kevin Wolf <kw...@redhat.com> writes:
>
> > Am 02.03.2020 um 15:22 hat Markus Armbruster geschrieben:
> >> 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.
> >
> > Ugh... If you choose that this is the way to go, please add an assertion
> > at least that we are indeed in qmp_dispatcher_co before yielding.
>
> No objection.
>
> To apply the QMP coroutine infrastructure for 5.0, I need a user. We
> have two: block_resize from Kevin, and screendump from Marc-André.
> Neither is quite ready, yet. I'll wait for a respin of either one.
>
Is this the change you expect?
diff --git a/ui/console.c b/ui/console.c
index 57df3a5439..d6a8bf0cee 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -167,7 +167,7 @@ struct QemuConsole {
QEMUFIFO out_fifo;
uint8_t out_fifo_buf[16];
QEMUTimer *kbd_timer;
- Coroutine *screendump_co;
+ bool wake_qmp_dispatcher_on_update;
QTAILQ_ENTRY(QemuConsole) next;
};
@@ -263,8 +263,8 @@ static void gui_setup_refresh(DisplayState *ds)
void graphic_hw_update_done(QemuConsole *con)
{
- if (con && con->screendump_co) {
- aio_co_wake(con->screendump_co);
+ if (con->wake_qmp_dispatcher_on_update) {
+ aio_co_wake(qmp_dispatcher_co);
}
}
@@ -376,12 +376,15 @@ void qmp_screendump(const char *filename, bool
has_device, const char *device,
}
if (qemu_in_coroutine()) {
- assert(!con->screendump_co);
- con->screendump_co = qemu_coroutine_self();
+ /*
+ * The coroutine code is generic, but we are supposed to be on
+ * the QMP dispatcher coroutine, and we will resume only that now.
+ */
+ assert(qemu_coroutine_self() == qmp_dispatcher_co);
+ con->wake_qmp_dispatcher_on_update = true;
aio_bh_schedule_oneshot(qemu_get_aio_context(),
graphic_hw_update_bh, con);
qemu_coroutine_yield();
- con->screendump_co = NULL;
+ con->wake_qmp_dispatcher_on_update = false;
}
--
Marc-André Lureau
