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

[Marc-André Lureau]

Hi

On Thu, Feb 20, 2020 at 8:49 AM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> 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: <address@hidden>
> >
> > Cc: Kevin Wolf <address@hidden>
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  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?

I don't think it can happen anymore (the patch evolved over several
years, this is probably a left-over). In any case, it doesn't hurt.


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

I can be split, but it's related. We should pass a reference to
pixman_image_t, rather than a pointer to DisplaySurface, as the
underlying image may change over time, and would result in corrupted
coroutine save or worse.

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

The situation is not worse than the current blocking handling.

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

I think it could be done later, so I suggest a TODO.

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

We have several functions that have this dual support, for ex QIO.

Changing both QMP & HMP commands to run in coroutine is likely
additional work that we may not care at this point.

I propose to leave a TODO, once we have several similar QMP & HMP mix
cases we can try to find a common HMP solution to make the code
simpler in QMP handler.

I don't know if this is going to be a common pattern, we may end up
with conversions that can run both without explicit handling (like the
ppm_save() function, thanks to QIO).

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

You could call it, but then you should wait for completion by
reentering the main loop (that was the point of my earlier qapi-async
series)

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

At least spice rendering is done in a separate thread, completion is async.

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



-- 
Marc-André Lureau