Re: [PATCH] console: make QMP screendump use coroutine

[Markus Armbruster <armbru@redhat.com>]

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

> Thanks to the QMP 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 the ppm_save() will
> write the screen image to disk in the coroutine context (thus
> non-blocking).
>
> For now, HMP doesn't have coroutine support, so it remains potentially
> outdated or glitched.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1230527
>
> Based-on: <20200109183545.27452-2-kwolf@redhat.com>
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi/ui.json    |  3 ++-
>  ui/console.c    | 35 +++++++++++++++++++++++++++--------
>  ui/trace-events |  2 +-
>  3 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index e04525d8b4..d941202f34 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -96,7 +96,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 ac79d679f5..db184b473f 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -167,6 +167,7 @@ struct QemuConsole {
>      QEMUFIFO out_fifo;
>      uint8_t out_fifo_buf[16];
>      QEMUTimer *kbd_timer;
> +    Coroutine *screendump_co;
>  
>      QTAILQ_ENTRY(QemuConsole) next;
>  };
> @@ -194,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)
>  {
> @@ -263,6 +263,9 @@ static void gui_setup_refresh(DisplayState *ds)
>  
>  void graphic_hw_update_done(QemuConsole *con)
>  {
> +    if (con && con->screendump_co) {

How can !con happen?

> +        aio_co_wake(con->screendump_co);
> +    }
>  }
>  
>  void graphic_hw_update(QemuConsole *con)
> @@ -310,16 +313,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),
> @@ -329,7 +332,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) {

Looks like an unrelated optimization / simplification.  If I was
maintainer, I'd ask for a separate patch.

> @@ -340,11 +343,18 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
>      return true;
>  }
>  
> +static void graphic_hw_update_bh(void *con)
> +{
> +    graphic_hw_update(con);
> +}
> +
> +/* may be called in coroutine context or not */

Hmm.

Even though the QMP core always calls in coroutine context, the comment
is correct: hmp_screendump() calls it outside coroutine context.
Because of that...

>  void qmp_screendump(const char *filename, bool has_device, const char *device,
>                      bool has_head, int64_t head, Error **errp)
>  {
>      QemuConsole *con;
>      DisplaySurface *surface;
> +    g_autoptr(pixman_image_t) image = NULL;
>      int fd;
>  
>      if (has_device) {
> @@ -365,7 +375,15 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
>          }
>      }
>  
> -    graphic_hw_update(con);
> +    if (qemu_in_coroutine()) {
> +        assert(!con->screendump_co);

What if multiple QMP monitors simultaneously screendump?  Hmm, it works
because all execute one after another in the same coroutine
qmp_dispatcher_co.  Implicit mutual exclusion.

Executing them one after another is bad, because it lets an ill-behaved
QMP command starve *all* QMP monitors.  We do it only out of
(reasonable!) fear of implicit mutual exclusion requirements like the
one you add.

Let's not add more if we can help it.

Your screendump_co is per QemuConsole instead of per QMP monitor only
because you need to find the coroutine in graphic_hw_update_done().  Can
we somehow pass it via function arguments?

In case avoiding the mutual exclusion is impractical: please explain it
in a comment to make it somewhat less implicit.

> +        con->screendump_co = qemu_coroutine_self();
> +        aio_bh_schedule_oneshot(qemu_get_aio_context(),
> +                                graphic_hw_update_bh, con);
> +        qemu_coroutine_yield();
> +        con->screendump_co = NULL;
> +    }
> +

... the command handler needs extra code to cope with either.  Is this
really what we want for coroutine QMP command handlers?  We'll acquire
more of them, and I'd hate to make each one run both in and outside
coroutine context.  Shouldn't we let the HMP core take care of this?  Or
at least have some common infrastructure these handlers can use?

Why is it okay not to call graphic_hw_update() anymore when
!qemu_in_coroutine()?

If qemu_in_coroutine(), we now run graphic_hw_update() in a bottom half,
then yield until the update completes (see graphic_hw_update_done()
above).  Can you explain the need for the bottom half?

>      surface = qemu_console_surface(con);
>      if (!surface) {
>          error_setg(errp, "no surface");
> @@ -379,7 +397,8 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
>          return;
>      }
>  
> -    if (!ppm_save(fd, surface, errp)) {
> +    image = pixman_image_ref(surface->image);
> +    if (!ppm_save(fd, image, errp)) {
>          qemu_unlink(filename);
>      }
>  }
> diff --git a/ui/trace-events b/ui/trace-events
> index 0dcda393c1..e8726fc969 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.c
>  # gtk-gl-area.c