[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/3] console: make QMP/HMP screendump run in coroutine
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 3/3] console: make QMP/HMP screendump run in coroutine |
Date: |
Tue, 27 Oct 2020 09:41:15 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 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 <marcandre.lureau@redhat.com>
> ---
> 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: <87a74ueudt.fsf@dusky.pond.sub.org>
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
?
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.
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."