[PATCH 0/3] console: make QMP screendump use coroutine

[PATCH 0/3] console: make QMP screendump use coroutine

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Thanks to recent work by Kevin, it becomes possible to run HMP/QMP commands i=
n a
coroutine. The screendump command is a good target, as it requires to re-enter
the main-loop in ordre to flush the display, and write to file in a non-block=
ing
way.

Despite the flush, the dump may still have glitches. The graphic device may
perform some operations during the write on the same framebuffer. Doing a mem=
ory
copy could help, but it would also create a number of other issues. Keeping t=
he
BQL would defeat a number of advantages of using a coroutine. Afaik, there is=
 no
mechanism to "freeze" the device either (and this could also have bad
consequences anyway). Good enough?

Marc-Andr=C3=A9 Lureau (3):
  coroutine: let CoQueue wake up outside a coroutine
  console: modify ppm_save to take a pixman image ref
  console: make QMP/HMP screendump run in coroutine

 hmp-commands.hx            |  1 +
 monitor/hmp-cmds.c         |  3 ++-
 qapi/ui.json               |  3 ++-
 ui/console.c               | 42 +++++++++++++++++++++++++++++---------
 ui/trace-events            |  2 +-
 util/qemu-coroutine-lock.c |  6 ++----
 6 files changed, 40 insertions(+), 17 deletions(-)

--=20
2.28.0

[PATCH 1/3] coroutine: let CoQueue wake up outside a coroutine

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

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 <marcandre.lureau@redhat.com>
---
 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 36927b5f88..5816bf8900 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);
 }
 
-- 
2.28.0

Re: [PATCH 1/3] coroutine: let CoQueue wake up outside a coroutine

[Markus Armbruster]

Kevin or Stefan, please review.

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 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 <marcandre.lureau@redhat.com>
> ---
>  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 36927b5f88..5816bf8900 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);
>  }

Re: [PATCH 1/3] coroutine: let CoQueue wake up outside a coroutine

[Kevin Wolf]

Am 10.10.2020 um 22:41 hat marcandre.lureau@redhat.com geschrieben:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 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 <marcandre.lureau@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH 2/3] console: modify ppm_save to take a pixman image ref

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The function is going to be called from a coroutine, and may yields.
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 <marcandre.lureau@redhat.com>
---
 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 54a74c0b16..a56fe0dd26 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 b7d7270c02..0ffcdb4408 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
-- 
2.28.0

Re: [PATCH 2/3] console: modify ppm_save to take a pixman image ref

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The function is going to be called from a coroutine, and may yields.

s/yields/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 <marcandre.lureau@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH 3/3] console: make QMP/HMP screendump run in coroutine

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks to the monitors coroutine support, 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 (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
a good idea, as framebuffers can be quite large. Even then, QEMU may
become less responsive as it requires paging in etc.

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. */
     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. */
     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,
-- 
2.28.0

Re: [PATCH 3/3] console: make QMP/HMP screendump run in coroutine

[Markus Armbruster]

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

Re: [PATCH 3/3] console: make QMP/HMP screendump run in coroutine

[Marc-André Lureau]

Hi

On Tue, Oct 27, 2020 at 12:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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
>
> ?

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?

Re: [PATCH 3/3] console: make QMP/HMP screendump run in coroutine

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Tue, Oct 27, 2020 at 12:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> 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>
[...]
>> 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
>>
>> ?
>
> 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.

I'm okay with this patch "only" getting us closer to the goal of having
screendump not block the main loop.  I just want the commit message to
be clear on how close.

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

Works for me!

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

Yes, but have you *tested* the main loop keeps running?

Re: [PATCH 0/3] console: make QMP screendump use coroutine

[Gerd Hoffmann]

  Hi,

> Despite the flush, the dump may still have glitches. The graphic
> device may perform some operations during the write on the same
> framebuffer.

That problem exists anyway, even without coroutines.  The guest can
write to vga memory in parallel to iothread writing out the screen
dump.

> Good enough?

I'd say yes.  console bits are:

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

I can take that through ui patch queue, but want an ack from someone who
knows coroutines better than me for that.  Merging through some other
queue (monitor?) is fine with me too.

take care,
  Gerd

Re: [PATCH 0/3] console: make QMP screendump use coroutine

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> Thanks to recent work by Kevin, it becomes possible to run HMP/QMP commands i=
> n a
> coroutine. The screendump command is a good target, as it requires to re-enter
> the main-loop in ordre to flush the display, and write to file in a non-block=
> ing
> way.
>
> Despite the flush, the dump may still have glitches. The graphic device may
> perform some operations during the write on the same framebuffer. Doing a mem=
> ory
> copy could help, but it would also create a number of other issues. Keeping t=
> he
> BQL would defeat a number of advantages of using a coroutine. Afaik, there is=
>  no
> mechanism to "freeze" the device either (and this could also have bad
> consequences anyway). Good enough?

This is v2 of
Message-Id: <20200113144848.2168018-1-marcandre.lureau@redhat.com>
https://lists.nongnu.org/archive/html/qemu-devel/2020-01/msg02313.html

The title has become slightly misleading: v2 covers HMP, too, as the
description says.

A changelog would've helped me review.  Next time :)

[...]