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