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

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Fri, Mar 6, 2020 at 9:44 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
>>
>> > Hi
>> >
>> > On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster <armbru@redhat.com> 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.

The correctness argument is non-local and relies on current limitations
of both QMP and HMP:

* QMP never interleaves commands execution, not even with multiple QMP
  monitors.  Complete HMP commands can still be interleaved with a QMP
  command.

* QMP executes commands marked 'coroutine' in a coroutine.  HMP does not
  execute commands in coroutines.

* qmp_screendump() carefully avoids the graphic_hw_update() machinery
  for HMP.  It uses "running in coroutine" as a proxy for "HMP".

* No other user of the graphic_hw_update() machinery wants
  graphic_hw_update_done() to wake up a coroutine.

* Therefore, at any time no more than one such update is for a user that
  wants a coroutine woken up.

* Therefore, storing the coroutine to be woken up in QemuConsole is
  safe.

If you insist that's just fine, please add a comment with the
correctness argument, and get Gerd's blessing for it.

I'd rather remove the need for such a longwinded and brittle argument,
but I'm not the maintainer of ui/ and hw/display/, Gerd is.

>
>>
>> >> >>
>> >> >> 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.

This is for Gerd to decide.