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

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

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

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?

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?