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

[Marc-André Lureau]

Hi

On Fri, Mar 6, 2020 at 9:44 AM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> writes:
>
> > Hi
> >
> > On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster <address@hidden> wrote:
> [...]
> >> >> 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.
>
> As discussed in the "[PATCH v4 1/4] qapi: Add a 'coroutine' flag for
> commands" sub-thread, HMP commands may execute interleaved.  Your code
> still works, because it only ever abuses QemuConsole with QMP.  But it's
> brittle.
>
> Looks like we'll change HMP not to run interleaved.  That adds a belt to
> the suspenders.  You might argue that's robust enough.
>
> But it's not just the brittleness I dislike.  Storing per-monitor-
> command data in QemuConsole is ugly as sin.  Arguing that it works
> because commands are strictly serialized, and that burying one more
> dependence on such serialization deep in command code won't make the
> situation appreciably worse, doesn't change the fact that QemuConsole
> has no business holding per-monitor-command data.

It is data (the monitor coroutine) associated with an event handler
(graphic-update).

Someone has to hold the handler/data, and the console seems appropriate.

We could abstract this a bit, for ex, having a GHookList, but as long
as there is only one handler, it's unnecessary.

>
> >> >>
> >> >> 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;
> >  };
>
> No, because it still stores per-command data in QemuConsole.  You need
> to, because...
>
> > @@ -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);
>
> ... you may call aio_co_wake() only while @qmp_dispatcher_co is waiting
> for it after yielding ...
>
> >      }
> >  }
> >
> > @@ -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();
>
> ... here.  I missed that need when I suggested to use
> @qmp_dispatcher_co.  Sorry.
>
> > -        con->screendump_co = NULL;
> > +        con->wake_qmp_dispatcher_on_update = false;
> >      }
>
> Have a look at qxl, the only provider of asynchronous .gfx_update().
> The actual work is done in qxl-render.c.  qxl_render_update(),
> qxl_render_update_area_bh(), qxl_render_update_area_unlocked(),
> qxl_render_update_area_done() cooperate carefully to support multiple
> updates in flight.
>
> I guess that's necessary because we also call graphic_hw_update() from
> display code such as ui/vnc.c and ui/spice-display.c.
>
> Before your patch, none of these users waits for an asynchronous update
> to complete, as far as I can tell.  Afterwards, QMP screendump does.
> Whether more users should I can't tell.  Gerd, can you?
>
> Your patch communicates completion to screendump by making
> graphic_hw_update() wake a coroutine.  It stores the coroutine in
> QemuConsole, exploiting that only one call site actually waits for an
> asynchronous update to complete, and that caller is never reentered.
>
> This new mechanism is not usable for any other caller, unless it somehow
> synchronizes with screendump to avoid reentrance.
>
> Shouldn't we offer a more generally useful way to wait for asynchronous
> update to complete?  Kevin's idea to use a queue of waiters sounds more
> appropriate than ever to me.
>

A CoQueue is a queue of coroutine. Similarly to the GHook suggestion,
I don't see much point as long as there is a single known handler.
Covering it through those abstractions will just lead to wrong
assumptions or code harder to read imho.

-- 
Marc-André Lureau