From: Markus Armbruster <armbru@redhat.com>
To: "Marc-André Lureau" <marcandre.lureau@gmail.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>,
QEMU <qemu-devel@nongnu.org>,
David Alan Gilbert <dgilbert@redhat.com>
Subject: Re: [PATCH] console: make QMP screendump use coroutine
Date: Wed, 11 Mar 2020 13:16:46 +0100 [thread overview]
Message-ID: <87a74ndr2p.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <CAJ+F1CKxbReSyR+fXzSuHWOXXs_DP1gdnhCOzqKJ2eqLERrzNQ@mail.gmail.com> (=?utf-8?Q?=22Marc-Andr=C3=A9?= Lureau"'s message of "Fri, 6 Mar 2020 11:03:37 +0100")
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.
next prev parent reply other threads:[~2020-03-11 12:17 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-13 14:48 [PATCH] console: make QMP screendump use coroutine Marc-André Lureau
2020-01-13 16:32 ` no-reply
2020-01-13 16:36 ` no-reply
2020-02-12 12:29 ` Gerd Hoffmann
2020-02-13 13:10 ` Markus Armbruster
2020-02-20 7:48 ` Markus Armbruster
2020-02-20 9:43 ` Marc-André Lureau
2020-02-20 16:01 ` Markus Armbruster
2020-02-20 20:11 ` Dr. David Alan Gilbert
2020-02-21 6:31 ` Markus Armbruster
2020-02-21 10:25 ` Dr. David Alan Gilbert
2020-02-21 10:07 ` Kevin Wolf
2020-02-21 16:50 ` Markus Armbruster
2020-02-24 16:20 ` Marc-André Lureau
2020-03-02 14:22 ` Markus Armbruster
2020-03-02 15:36 ` Kevin Wolf
2020-03-03 7:41 ` Markus Armbruster
2020-03-03 10:56 ` Marc-André Lureau
2020-03-03 14:47 ` Markus Armbruster
2020-03-03 16:03 ` Marc-André Lureau
2020-03-06 8:44 ` Markus Armbruster
2020-03-06 10:03 ` Marc-André Lureau
2020-03-11 12:16 ` Markus Armbruster [this message]
2020-03-05 14:41 ` Markus Armbruster
2020-03-05 15:08 ` Marc-André Lureau
2020-03-06 5:56 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a74ndr2p.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=dgilbert@redhat.com \
--cc=kraxel@redhat.com \
--cc=kwolf@redhat.com \
--cc=marcandre.lureau@gmail.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).