Hi On Tue, Oct 27, 2020 at 12:41 PM Markus Armbruster <[email protected]> wrote: > > [email protected] writes: > > > From: Marc-André Lureau <[email protected]> > > > > Thanks to the monitors coroutine support, the screendump handler can > > monitors' > > Suggest to add (merge commit b7092cda1b3) right before the comma. > > > trigger a graphic_hw_update(), yield and let the main loop run until > > update is done. Then the handler is resumed, and ppm_save() will write > > the screen image to disk in the coroutine context (thus non-blocking). > > > > Potentially, during non-blocking write, some new graphic update could > > happen, and thus the image may have some glitches. Whether that > > behaviour is acceptable is discutable. Allocating new memory may not be > > s/discutable/debatable/ > > > a good idea, as framebuffers can be quite large. Even then, QEMU may > > become less responsive as it requires paging in etc. > > Tradeoff. I'm okay with "simple & efficient, but might glitch". It > should be documented, though. Followup patch is fine. > > > Related to: > > https://bugzilla.redhat.com/show_bug.cgi?id=1230527 > > > > Signed-off-by: Marc-André Lureau <[email protected]> > > --- > > hmp-commands.hx | 1 + > > monitor/hmp-cmds.c | 3 ++- > > qapi/ui.json | 3 ++- > > ui/console.c | 27 ++++++++++++++++++++++++--- > > 4 files changed, 29 insertions(+), 5 deletions(-) > > > > diff --git a/hmp-commands.hx b/hmp-commands.hx > > index cd068389de..ff2d7aa8f3 100644 > > --- a/hmp-commands.hx > > +++ b/hmp-commands.hx > > @@ -254,6 +254,7 @@ ERST > > .help = "save screen from head 'head' of display device > > 'device' " > > "into PPM image 'filename'", > > .cmd = hmp_screendump, > > + .coroutine = true, > > }, > > > > SRST > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > > index 9789f4277f..91608bac6d 100644 > > --- a/monitor/hmp-cmds.c > > +++ b/monitor/hmp-cmds.c > > @@ -1756,7 +1756,8 @@ err_out: > > goto out; > > } > > > > -void hmp_screendump(Monitor *mon, const QDict *qdict) > > +void coroutine_fn > > +hmp_screendump(Monitor *mon, const QDict *qdict) > > { > > const char *filename = qdict_get_str(qdict, "filename"); > > const char *id = qdict_get_try_str(qdict, "device"); > > diff --git a/qapi/ui.json b/qapi/ui.json > > index 9d6721037f..6c7b33cb72 100644 > > --- a/qapi/ui.json > > +++ b/qapi/ui.json > > @@ -98,7 +98,8 @@ > > # > > ## > > { 'command': 'screendump', > > - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'} } > > + 'data': {'filename': 'str', '*device': 'str', '*head': 'int'}, > > + 'coroutine': true } > > > > ## > > # == Spice > > diff --git a/ui/console.c b/ui/console.c > > index a56fe0dd26..0118f70d9a 100644 > > --- a/ui/console.c > > +++ b/ui/console.c > > @@ -168,6 +168,7 @@ struct QemuConsole { > > QEMUFIFO out_fifo; > > uint8_t out_fifo_buf[16]; > > QEMUTimer *kbd_timer; > > + CoQueue dump_queue; > > > > QTAILQ_ENTRY(QemuConsole) next; > > }; > > @@ -263,6 +264,7 @@ static void gui_setup_refresh(DisplayState *ds) > > > > void graphic_hw_update_done(QemuConsole *con) > > { > > + qemu_co_queue_restart_all(&con->dump_queue); > > } > > > > void graphic_hw_update(QemuConsole *con) > > @@ -340,8 +342,15 @@ static bool ppm_save(int fd, pixman_image_t *image, > > Error **errp) > > return true; > > } > > > > -void qmp_screendump(const char *filename, bool has_device, const char > > *device, > > - bool has_head, int64_t head, Error **errp) > > +static void graphic_hw_update_bh(void *con) > > +{ > > + graphic_hw_update(con); > > +} > > + > > +/* Safety: coroutine-only, concurrent-coroutine safe, main thread only */ > > +void coroutine_fn > > +qmp_screendump(const char *filename, bool has_device, const char *device, > > + bool has_head, int64_t head, Error **errp) > > { > > g_autoptr(pixman_image_t) image = NULL; > > QemuConsole *con; > > @@ -366,7 +375,15 @@ void qmp_screendump(const char *filename, bool > > has_device, const char *device, > > } > > } > > > > - graphic_hw_update(con); > > + if (qemu_co_queue_empty(&con->dump_queue)) { > > + /* Defer the update, it will restart the pending coroutines */ > > + aio_bh_schedule_oneshot(qemu_get_aio_context(), > > + graphic_hw_update_bh, con); > > + } > > + qemu_co_queue_wait(&con->dump_queue, NULL); > > + > > + /* All pending coroutines are woken up, while BQL taken, no further > > graphic > > + * update are possible until it is released, take an image ref before > > that. */ > > "while BQL taken": I guess you mean "while the BQL is held". > > Style nit: CODING_STYLE.rst asks for wings. > > Recommend to limit comment line length for readability. > > Recommend to turn a few commas into periods. > > Together: > > /* > * All pending coroutines are woken up, while the BQL is held. No > * further graphic update are possible until it is released. Take > * an image ref before that. > */ > > > surface = qemu_console_surface(con); > > if (!surface) { > > error_setg(errp, "no surface"); > > @@ -381,6 +398,9 @@ void qmp_screendump(const char *filename, bool > > has_device, const char *device, > > return; > > } > > > > + /* The image content could potentially be updated as the coroutine > > yields > > + * and releases the BQL. It could produce corrupted dump, but it > > should be > > + * otherwise safe. */ > > Similar nit. > > /* > * The image content could potentially be updated as the coroutine > * yields and releases the BQL. It could produce corrupted dump, but > * it should be otherwise safe. > */ > > > if (!ppm_save(fd, image, errp)) { > > qemu_unlink(filename); > > } > > @@ -1297,6 +1317,7 @@ static QemuConsole *new_console(DisplayState *ds, > > console_type_t console_type, > > > > obj = object_new(TYPE_QEMU_CONSOLE); > > s = QEMU_CONSOLE(obj); > > + qemu_co_queue_init(&s->dump_queue); > > s->head = head; > > object_property_add_link(obj, "device", TYPE_DEVICE, > > (Object **)&s->device, > > Simpler than v1 thanks to coroutine support for HMP, and the use of > CoQueue. > > > Let's revisit the experiment I did for v1: "observe the main loop keeps > running while the screendump does its work". > > Message-ID: <[email protected]> > https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01235.html > > I repeated the first experiment "does the main loop continue to run when > writing out the screendump blocks / would block?" Same result: main > loop remains blocked. > > Back then, you replied > > Right, the goal was rather originally to fix rhbz#1230527. We got > coroutine IO by accident, and I was too optimistic about default > behaviour change ;) I will update the patch. > > I'm unsure what exactly the update is. Is it the change from > > Fixes: > https://bugzilla.redhat.com/show_bug.cgi?id=1230527 > > to > > Related to: > https://bugzilla.redhat.com/show_bug.cgi?id=1230527 > > ?
Right, qmp_screendump() calls qemu_open_old(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666), so opened in blocking mode. So simply opening a FS file with O_NONBLOCK or calling qemu_try_set_nonblock(fd) with the resulting fd doesn't really help to check it's async (unless I am missing a trick to slow down disk IO somehow...?) It's annoying that it also does O_TRUNC: even if you pass a socket/pipe to add-fd, it will fail to ftruncate() (via the "/dev/fdset" path). Furthermore, access mode check seems kinda incomplete. Afaict, in monitor_fdset_dup_fd_add(), F_GETFL may return O_RDWR which is different than O_RDONLY or O_WRONLY, yet should be considered compatible for the caller I think.. With some little hacks though, I could check passing a pipe does indeed make PPM save async, and the main loop is reentered. I don't know if it's enough to convince you it's not really the problem of this code change though. We get coroutine IO by accident here, I think we already said that. > The commit message says "ppm_save() will write the screen image to disk > in the coroutine context (thus non-blocking)." Sure reads like a claim > the main loop is no longer blocked. It is blocked, at least for me. > Please clarify. Let's clarify it by saying it's still blocking then, and tackle that in a future change. > Back then, I proposed a second experiment: "does the main loop continue > to run while we wait for graphic_hw_update_done()?" I don't know the > result. Do you? > > The commit message claims "the screendump handler can trigger a > graphic_hw_update(), yield and let the main loop run until update is > done." Isn't it clearly the case with the BH being triggered after main loop?
