From e18d9a9687ff0c30b70ee0a694d9fcf9333cd844 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Tue, 27 Oct 2020 17:36:00 +0400 Subject: coroutine: let CoQueue wake up outside a coroutine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The assert() was added in commit b681a1c73e15 ("block: Repair the throttling code."), when the qemu_co_queue_do_restart() function required to be running in a coroutine. It was later made unnecessary in commit a9d9235567e7 ("coroutine-lock: reschedule coroutine on the AioContext it was running on"). Signed-off-by: Marc-André Lureau Reviewed-by: Kevin Wolf Reviewed-by: Gerd Hoffmann Message-id: 20201027133602.3038018-2-marcandre.lureau@redhat.com Signed-off-by: Gerd Hoffmann --- util/qemu-coroutine-lock.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c index 36927b5..5816bf8 100644 --- a/util/qemu-coroutine-lock.c +++ b/util/qemu-coroutine-lock.c @@ -85,15 +85,13 @@ static bool qemu_co_queue_do_restart(CoQueue *queue, bool single) return true; } -bool coroutine_fn qemu_co_queue_next(CoQueue *queue) +bool qemu_co_queue_next(CoQueue *queue) { - assert(qemu_in_coroutine()); return qemu_co_queue_do_restart(queue, true); } -void coroutine_fn qemu_co_queue_restart_all(CoQueue *queue) +void qemu_co_queue_restart_all(CoQueue *queue) { - assert(qemu_in_coroutine()); qemu_co_queue_do_restart(queue, false); } -- cgit v1.1 From d00ec2fe86e141324c26816c5847f425a63f851d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Tue, 27 Oct 2020 17:36:01 +0400 Subject: console: modify ppm_save to take a pixman image ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function is going to be called from a coroutine, and may yield. Let's ensure our image reference doesn't change over time (due to resize etc) by keeping a ref. Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster Reviewed-by: Gerd Hoffmann Message-id: 20201027133602.3038018-3-marcandre.lureau@redhat.com Signed-off-by: Gerd Hoffmann --- ui/console.c | 15 ++++++++------- ui/trace-events | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/ui/console.c b/ui/console.c index 820e408..96dd212 100644 --- a/ui/console.c +++ b/ui/console.c @@ -195,7 +195,6 @@ static void dpy_refresh(DisplayState *s); static DisplayState *get_alloc_displaystate(void); static void text_console_update_cursor_timer(void); static void text_console_update_cursor(void *opaque); -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp); static void gui_update(void *opaque) { @@ -311,16 +310,16 @@ void graphic_hw_invalidate(QemuConsole *con) } } -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp) +static bool ppm_save(int fd, pixman_image_t *image, Error **errp) { - int width = pixman_image_get_width(ds->image); - int height = pixman_image_get_height(ds->image); + int width = pixman_image_get_width(image); + int height = pixman_image_get_height(image); g_autoptr(Object) ioc = OBJECT(qio_channel_file_new_fd(fd)); g_autofree char *header = NULL; g_autoptr(pixman_image_t) linebuf = NULL; int y; - trace_ppm_save(fd, ds); + trace_ppm_save(fd, image); header = g_strdup_printf("P6\n%d %d\n%d\n", width, height, 255); if (qio_channel_write_all(QIO_CHANNEL(ioc), @@ -330,7 +329,7 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error **errp) linebuf = qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width); for (y = 0; y < height; y++) { - qemu_pixman_linebuf_fill(linebuf, ds->image, width, 0, y); + qemu_pixman_linebuf_fill(linebuf, image, width, 0, y); if (qio_channel_write_all(QIO_CHANNEL(ioc), (char *)pixman_image_get_data(linebuf), pixman_image_get_stride(linebuf), errp) < 0) { @@ -344,6 +343,7 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error **errp) void 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; DisplaySurface *surface; int fd; @@ -372,6 +372,7 @@ void qmp_screendump(const char *filename, bool has_device, const char *device, error_setg(errp, "no surface"); return; } + image = pixman_image_ref(surface->image); fd = qemu_open_old(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666); if (fd == -1) { @@ -380,7 +381,7 @@ void qmp_screendump(const char *filename, bool has_device, const char *device, return; } - if (!ppm_save(fd, surface, errp)) { + if (!ppm_save(fd, image, errp)) { qemu_unlink(filename); } } diff --git a/ui/trace-events b/ui/trace-events index b7d7270..0ffcdb4 100644 --- a/ui/trace-events +++ b/ui/trace-events @@ -15,7 +15,7 @@ displaysurface_create_pixman(void *display_surface) "surface=%p" displaysurface_free(void *display_surface) "surface=%p" displaychangelistener_register(void *dcl, const char *name) "%p [ %s ]" displaychangelistener_unregister(void *dcl, const char *name) "%p [ %s ]" -ppm_save(int fd, void *display_surface) "fd=%d surface=%p" +ppm_save(int fd, void *image) "fd=%d image=%p" # gtk-egl.c # gtk-gl-area.c -- cgit v1.1 From 0d9b90ce5c73505648909a89bcd5272081b9c348 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Tue, 27 Oct 2020 17:36:02 +0400 Subject: console: make QMP/HMP screendump run in coroutine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks to the monitors' coroutine support (merge commit b7092cda1b3), the screendump handler can 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. The IO is still blocking though, as the file is set blocking so far, this could be addressed by some future change (with other caveats). Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1230527 Signed-off-by: Marc-André Lureau Reviewed-by: Gerd Hoffmann Reviewed-by: Markus Armbruster Message-id: 20201027133602.3038018-4-marcandre.lureau@redhat.com Signed-off-by: Gerd Hoffmann --- hmp-commands.hx | 1 + monitor/hmp-cmds.c | 3 ++- qapi/ui.json | 3 ++- ui/console.c | 32 +++++++++++++++++++++++++++++--- 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/hmp-commands.hx b/hmp-commands.hx index cd06838..ff2d7aa 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 56e9bad..a6a6684 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1762,7 +1762,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 9d67210..6c7b33c 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 96dd212..e8e5970 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,18 @@ 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 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 +401,11 @@ 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. + */ if (!ppm_save(fd, image, errp)) { qemu_unlink(filename); } @@ -1297,6 +1322,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, -- cgit v1.1