Re: [PATCH] console: make QMP screendump use coroutine

[Marc-André Lureau]

Hi

On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster <address@hidden> wrote:
>
> Kevin Wolf <address@hidden> writes:
>
> > Am 02.03.2020 um 15:22 hat Markus Armbruster geschrieben:
> >> 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.
> >
> > 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