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

[Markus Armbruster <armbru@redhat.com>]

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.

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