diff mbox

[v2,23/25] console: make screendump async

Message ID 20170118160332.13390-24-marcandre.lureau@redhat.com
State New
Headers show

Commit Message

Marc-André Lureau Jan. 18, 2017, 4:03 p.m. UTC
Make screendump async to provide correct screendumps.

HMP doesn't have async support, so it has to remain sync to avoid
potential races.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1230527

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qapi-schema.json     |  5 +++-
 include/ui/console.h |  1 +
 hmp.c                |  2 +-
 ui/console.c         | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 73 insertions(+), 3 deletions(-)

Comments

Gerd Hoffmann Jan. 19, 2017, 8:20 a.m. UTC | #1
On Mi, 2017-01-18 at 20:03 +0400, Marc-André Lureau wrote:
> +    surface = qemu_console_surface(con);
> +
> +    /* FIXME: async save with coroutine? it would have to copy or
> lock
> +     * the surface. */
> +    ppm_save(filename, surface, &err);
> +

No need to lock or copy.

ppm_save() uses the pixman image (surface->image) only anyway, so
changing it to accept a pixman image instead of the surface is easy.

pixman images are reference counted, so you can just grab a reference
using pixman_image_ref() and run with it, without risking it'll be
released underneath your feet, then unref when done.

cheers,
  Gerd
Marc-André Lureau Jan. 20, 2017, 1:07 p.m. UTC | #2
Hi

On Thu, Jan 19, 2017 at 12:21 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mi, 2017-01-18 at 20:03 +0400, Marc-André Lureau wrote:
> > +    surface = qemu_console_surface(con);
> > +
> > +    /* FIXME: async save with coroutine? it would have to copy or
> > lock
> > +     * the surface. */
> > +    ppm_save(filename, surface, &err);
> > +
>
> No need to lock or copy.
>
> ppm_save() uses the pixman image (surface->image) only anyway, so
> changing it to accept a pixman image instead of the surface is easy.
>
> pixman images are reference counted, so you can just grab a reference
> using pixman_image_ref() and run with it, without risking it'll be
> released underneath your feet, then unref when done.
>
>
If you only ref, you could have the image being modified while it is being
saved asynchronously, this could result in tearing artefacts, no?
Gerd Hoffmann Jan. 23, 2017, 12:04 p.m. UTC | #3
Hi,
> 
> If you only ref, you could have the image being modified while it is
> being saved asynchronously, this could result in tearing artefacts,
> no?

Yes, but you have that problem _anyway_.  screendump qmp command can run
in parallel to guest vcpu, so it can race with guest display updates
today.  Assuming they happen as simple framebuffer writes and therefore
don't involve vmexit + grabbing big qemu lock.

cheers,
  Gerd
Marc-André Lureau Jan. 23, 2017, 12:35 p.m. UTC | #4
Hi

On Mon, Jan 23, 2017 at 4:04 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> >
> > If you only ref, you could have the image being modified while it is
> > being saved asynchronously, this could result in tearing artefacts,
> > no?
>
> Yes, but you have that problem _anyway_.  screendump qmp command can run
> in parallel to guest vcpu, so it can race with guest display updates
> today.  Assuming they happen as simple framebuffer writes and therefore
> don't involve vmexit + grabbing big qemu lock.
>

Agreed, fwiw, I implemented a coroutine-based ppm_save in the qapi-async
branch:
https://github.com/elmarco/qemu/commit/6f45a28ce737eab1d0ce4639f7b015a51a43674c

btw, is it supported to reenter the main_loop in hmp_screendump_sync()
(Paolo)?

(the alternative is probably to block hmp until the async command is
finished. This would eventually be interesting if more qmp commands used
from hmp are converted to be async)
diff mbox

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index 2e7c96c122..2af8661357 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4686,6 +4686,9 @@ 
 #
 # Returns: Nothing on success
 #
+# Note: the operation can be cancelled and the file not written if the
+# client disconnects before completion.
+#
 # Since: 0.14.0
 #
 # Example:
@@ -4695,7 +4698,7 @@ 
 # <- { "return": {} }
 #
 ##
-{ 'command': 'screendump', 'data': {'filename': 'str'} }
+{ 'command': 'screendump', 'data': {'filename': 'str'}, 'async': true }
 
 
 ##
diff --git a/include/ui/console.h b/include/ui/console.h
index e2d4f0df0d..4199e7008a 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -75,6 +75,7 @@  struct MouseTransformInfo {
 };
 
 void hmp_mouse_set(Monitor *mon, const QDict *qdict);
+void hmp_screendump_sync(const char *filename, Error **errp);
 
 /* keysym is a unicode code except for special keys (see QEMU_KEY_xxx
    constants) */
diff --git a/hmp.c b/hmp.c
index 8522efea26..6586247a31 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1917,7 +1917,7 @@  void hmp_screendump(Monitor *mon, const QDict *qdict)
     const char *filename = qdict_get_str(qdict, "filename");
     Error *err = NULL;
 
-    qmp_screendump(filename, &err);
+    hmp_screendump_sync(filename, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/ui/console.c b/ui/console.c
index 46ad6abd94..199296525c 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -114,6 +114,12 @@  typedef enum {
     TEXT_CONSOLE_FIXED_SIZE
 } console_type_t;
 
+struct qmp_screendump {
+    gchar *filename;
+    QmpReturn *ret;
+    QLIST_ENTRY(qmp_screendump) link;
+};
+
 struct QemuConsole {
     Object parent;
 
@@ -163,6 +169,8 @@  struct QemuConsole {
     QEMUFIFO out_fifo;
     uint8_t out_fifo_buf[16];
     QEMUTimer *kbd_timer;
+
+    QLIST_HEAD(, qmp_screendump) qmp_screendumps;
 };
 
 struct DisplayState {
@@ -188,6 +196,8 @@  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 void ppm_save(const char *filename, DisplaySurface *ds,
+                     Error **errp);
 
 static void gui_update(void *opaque)
 {
@@ -254,8 +264,39 @@  static void gui_setup_refresh(DisplayState *ds)
     ds->have_text = have_text;
 }
 
+static void qmp_screendump_finish(QemuConsole *con, const char *filename,
+                                  QmpReturn *ret)
+{
+    Error *err = NULL;
+    DisplaySurface *surface;
+
+    if (qmp_return_is_cancelled(ret)) {
+        return;
+    }
+
+    surface = qemu_console_surface(con);
+
+    /* FIXME: async save with coroutine? it would have to copy or lock
+     * the surface. */
+    ppm_save(filename, surface, &err);
+
+    if (err) {
+        qmp_return_error(ret, err);
+    } else {
+        qmp_screendump_return(ret);
+    }
+}
+
 void graphic_hw_update_done(QemuConsole *con)
 {
+    struct qmp_screendump *dump, *next;
+
+    QLIST_FOREACH_SAFE(dump, &con->qmp_screendumps, link, next) {
+        qmp_screendump_finish(con, dump->filename, dump->ret);
+        g_free(dump->filename);
+        QLIST_REMOVE(dump, link);
+        g_free(dump);
+    }
 }
 
 bool graphic_hw_update(QemuConsole *con)
@@ -356,7 +397,8 @@  write_err:
     goto out;
 }
 
-void qmp_screendump(const char *filename, Error **errp)
+/* this sync screendump may produce outdated dumps: use qmp instead */
+void hmp_screendump_sync(const char *filename, Error **errp)
 {
     QemuConsole *con = qemu_console_lookup_by_index(0);
     DisplaySurface *surface;
@@ -371,6 +413,29 @@  void qmp_screendump(const char *filename, Error **errp)
     ppm_save(filename, surface, errp);
 }
 
+void qmp_screendump(const char *filename, QmpReturn *qret)
+{
+    QemuConsole *con = qemu_console_lookup_by_index(0);
+    bool async;
+    Error *err = NULL;
+
+    if (con == NULL) {
+        error_setg(&err, "There is no QemuConsole I can screendump from.");
+        qmp_return_error(qret, err);
+        return;
+    }
+
+    async = graphic_hw_update(con);
+    if (async) {
+        struct qmp_screendump *dump = g_new(struct qmp_screendump, 1);
+        dump->filename = g_strdup(filename);
+        dump->ret = qret;
+        QLIST_INSERT_HEAD(&con->qmp_screendumps, dump, link);
+    } else {
+        qmp_screendump_finish(con, filename, qret);
+    }
+}
+
 void graphic_hw_text_update(QemuConsole *con, console_ch_t *chardata)
 {
     if (!con) {
@@ -1248,6 +1313,7 @@  static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
     obj = object_new(TYPE_QEMU_CONSOLE);
     s = QEMU_CONSOLE(obj);
     s->head = head;
+    QLIST_INIT(&s->qmp_screendumps);
     object_property_add_link(obj, "device", TYPE_DEVICE,
                              (Object **)&s->device,
                              object_property_allow_set_link,