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