[PATCH v6 00/12] monitor: Optionally run handlers in coroutines

[PATCH v6 00/12] monitor: Optionally run handlers in coroutines

[Kevin Wolf]

Some QMP command handlers can block the main loop for a relatively long
time, for example because they perform some I/O. This is quite nasty.
Allowing such handlers to run in a coroutine where they can yield (and
therefore release the BQL) while waiting for an event such as I/O
completion solves the problem.

This series adds the infrastructure to allow this and switches
block_resize to run in a coroutine as a first example.

This is an alternative solution to Marc-André's "monitor: add
asynchronous command type" series.

v6:
- Fixed cur_mon behaviour: It should always point to the Monitor while
  we're in the handler coroutine, but be NULL while the handler
  coroutines has yielded. [Markus]
- Give HMP handlers the coroutine option, too, because they will call
  QMP handlers, and life is easier when we know whether we are in
  coroutine context or not.
- Fixed block_resize for block devices in iothreads and for HMP
- Resolved some merge conflict with QAPI generator and monitor
  refactorings that were merged in the meantime

v5:
- Improved comments and documentation [Markus]

v4:
- Forbid 'oob': true, 'coroutine': true [Markus]
- Removed Python type hints [Markus]
- Introduced separate bool qmp_dispatcher_co_shutdown to make it clearer
  how a shutdown request is signalled to the dispatcher [Markus]
- Allow using aio_poll() with iohandler_ctx and use that instead of
  aio_bh_poll() [Markus]
- Removed coroutine_fn from qmp_block_resize() again because at least
  one caller (HMP) calls it outside of coroutine context [Markus]
- Tried to make the synchronisation between dispatcher and the monitor
  thread clearer, and fixed a race condition
- Improved documentation and comments

v3:
- Fix race between monitor thread and dispatcher that could schedule the
  dispatcher coroutine twice if a second requests comes in before the
  dispatcher can wake up [Patchew]

v2:
- Fix typo in a commit message [Eric]
- Use hyphen instead of underscore for the test command [Eric]
- Mark qmp_block_resize() as coroutine_fn [Stefan]


Kevin Wolf (12):
  monitor: Add Monitor parameter to monitor_set_cpu()
  monitor: Use getter/setter functions for cur_mon
  hmp: Set cur_mon only in handle_hmp_command()
  qmp: Assert that no other monitor is active
  qmp: Call monitor_set_cur() only in qmp_dispatch()
  monitor: Make current monitor a per-coroutine property
  qapi: Add a 'coroutine' flag for commands
  qmp: Move dispatcher to a coroutine
  hmp: Add support for coroutine command handlers
  util/async: Add aio_co_reschedule_self()
  block: Add bdrv_co_move_to_aio_context()
  block: Convert 'block_resize' to coroutine

 qapi/block-core.json                    |   3 +-
 docs/devel/qapi-code-gen.txt            |  12 +++
 include/block/aio.h                     |  10 ++
 include/block/block.h                   |   6 ++
 include/monitor/monitor.h               |   5 +-
 include/qapi/qmp/dispatch.h             |   5 +-
 monitor/monitor-internal.h              |   7 +-
 audio/wavcapture.c                      |   8 +-
 block.c                                 |  10 ++
 blockdev.c                              |  13 ++-
 dump/dump.c                             |   2 +-
 hw/scsi/vhost-scsi.c                    |   2 +-
 hw/virtio/vhost-vsock.c                 |   2 +-
 migration/fd.c                          |   4 +-
 monitor/hmp-cmds.c                      |   2 +-
 monitor/hmp.c                           |  51 +++++++---
 monitor/misc.c                          |  21 ++--
 monitor/monitor.c                       |  87 ++++++++++++++--
 monitor/qmp-cmds-control.c              |   2 +
 monitor/qmp-cmds.c                      |   2 +-
 monitor/qmp.c                           | 130 +++++++++++++++++-------
 net/socket.c                            |   2 +-
 net/tap.c                               |   6 +-
 qapi/qmp-dispatch.c                     |  52 +++++++++-
 qapi/qmp-registry.c                     |   3 +
 qga/main.c                              |   2 +-
 stubs/monitor-core.c                    |   9 +-
 tests/test-qmp-cmds.c                   |  10 +-
 tests/test-util-sockets.c               |  22 ++--
 trace/control.c                         |   2 +-
 util/aio-posix.c                        |   8 +-
 util/async.c                            |  30 ++++++
 util/qemu-error.c                       |   4 +-
 util/qemu-print.c                       |   3 +-
 util/qemu-sockets.c                     |   1 +
 scripts/qapi/commands.py                |  10 +-
 scripts/qapi/doc.py                     |   2 +-
 scripts/qapi/expr.py                    |  10 +-
 scripts/qapi/introspect.py              |   2 +-
 scripts/qapi/schema.py                  |  12 ++-
 tests/qapi-schema/test-qapi.py          |   7 +-
 hmp-commands.hx                         |   1 +
 tests/Makefile.include                  |   1 +
 tests/qapi-schema/oob-coroutine.err     |   2 +
 tests/qapi-schema/oob-coroutine.json    |   2 +
 tests/qapi-schema/oob-coroutine.out     |   0
 tests/qapi-schema/qapi-schema-test.json |   1 +
 tests/qapi-schema/qapi-schema-test.out  |   2 +
 48 files changed, 454 insertions(+), 136 deletions(-)
 create mode 100644 tests/qapi-schema/oob-coroutine.err
 create mode 100644 tests/qapi-schema/oob-coroutine.json
 create mode 100644 tests/qapi-schema/oob-coroutine.out

-- 
2.25.4

[PATCH v6 01/12] monitor: Add Monitor parameter to monitor_set_cpu()

[Kevin Wolf]

Most callers actually don't have to rely on cur_mon, but already know
for which monitor they call monitor_set_cpu().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/monitor/monitor.h |  2 +-
 monitor/hmp-cmds.c        |  2 +-
 monitor/misc.c            | 10 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 1018d754a6..0dcaefd4f9 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -33,7 +33,7 @@ int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
     GCC_FMT_ATTR(2, 0);
 int monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
 void monitor_flush(Monitor *mon);
-int monitor_set_cpu(int cpu_index);
+int monitor_set_cpu(Monitor *mon, int cpu_index);
 int monitor_get_cpu_index(void);
 
 void monitor_read_command(MonitorHMP *mon, int show_prompt);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 9c61e769ca..5e22ee2556 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -969,7 +969,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict)
     /* XXX: drop the monitor_set_cpu() usage when all HMP commands that
             use it are converted to the QAPI */
     cpu_index = qdict_get_int(qdict, "index");
-    if (monitor_set_cpu(cpu_index) < 0) {
+    if (monitor_set_cpu(mon, cpu_index) < 0) {
         monitor_printf(mon, "invalid CPU index\n");
     }
 }
diff --git a/monitor/misc.c b/monitor/misc.c
index f5207cd242..bdf49e49e5 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -130,7 +130,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     cur_mon = &hmp.common;
 
     if (has_cpu_index) {
-        int ret = monitor_set_cpu(cpu_index);
+        int ret = monitor_set_cpu(&hmp.common, cpu_index);
         if (ret < 0) {
             cur_mon = old_mon;
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
@@ -256,7 +256,7 @@ static void monitor_init_qmp_commands(void)
 }
 
 /* Set the current CPU defined by the user. Callers must hold BQL. */
-int monitor_set_cpu(int cpu_index)
+int monitor_set_cpu(Monitor *mon, int cpu_index)
 {
     CPUState *cpu;
 
@@ -264,8 +264,8 @@ int monitor_set_cpu(int cpu_index)
     if (cpu == NULL) {
         return -1;
     }
-    g_free(cur_mon->mon_cpu_path);
-    cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
+    g_free(mon->mon_cpu_path);
+    mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
     return 0;
 }
 
@@ -286,7 +286,7 @@ static CPUState *mon_get_cpu_sync(bool synchronize)
         if (!first_cpu) {
             return NULL;
         }
-        monitor_set_cpu(first_cpu->cpu_index);
+        monitor_set_cpu(cur_mon, first_cpu->cpu_index);
         cpu = first_cpu;
     }
     assert(cpu != NULL);
-- 
2.25.4

Re: [PATCH v6 01/12] monitor: Add Monitor parameter to monitor_set_cpu()

[Eric Blake]

On 5/28/20 10:37 AM, Kevin Wolf wrote:
> Most callers actually don't have to rely on cur_mon, but already know
> for which monitor they call monitor_set_cpu().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/monitor/monitor.h |  2 +-
>   monitor/hmp-cmds.c        |  2 +-
>   monitor/misc.c            | 10 +++++-----
>   3 files changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v6 01/12] monitor: Add Monitor parameter to monitor_set_cpu()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Most callers actually don't have to rely on cur_mon, but already know
> for which monitor they call monitor_set_cpu().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/monitor/monitor.h |  2 +-
>  monitor/hmp-cmds.c        |  2 +-
>  monitor/misc.c            | 10 +++++-----
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 1018d754a6..0dcaefd4f9 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -33,7 +33,7 @@ int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>      GCC_FMT_ATTR(2, 0);
>  int monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>  void monitor_flush(Monitor *mon);
> -int monitor_set_cpu(int cpu_index);
> +int monitor_set_cpu(Monitor *mon, int cpu_index);
>  int monitor_get_cpu_index(void);

monitor_set_cpu() now takes a Monitor * argument, while
monitor_get_cpu_index() continues to assume cur_mon.  Not wrong, but the
asymmetry bothers me.

Both callers of the latter look like they could easily pass a Monitor *
argument.  What do you think?

>  
>  void monitor_read_command(MonitorHMP *mon, int show_prompt);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 9c61e769ca..5e22ee2556 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -969,7 +969,7 @@ void hmp_cpu(Monitor *mon, const QDict *qdict)
>      /* XXX: drop the monitor_set_cpu() usage when all HMP commands that
>              use it are converted to the QAPI */
>      cpu_index = qdict_get_int(qdict, "index");
> -    if (monitor_set_cpu(cpu_index) < 0) {
> +    if (monitor_set_cpu(mon, cpu_index) < 0) {
>          monitor_printf(mon, "invalid CPU index\n");
>      }
>  }
> diff --git a/monitor/misc.c b/monitor/misc.c
> index f5207cd242..bdf49e49e5 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -130,7 +130,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      cur_mon = &hmp.common;
>  
>      if (has_cpu_index) {
> -        int ret = monitor_set_cpu(cpu_index);
> +        int ret = monitor_set_cpu(&hmp.common, cpu_index);
>          if (ret < 0) {
>              cur_mon = old_mon;
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
> @@ -256,7 +256,7 @@ static void monitor_init_qmp_commands(void)
>  }
>  
>  /* Set the current CPU defined by the user. Callers must hold BQL. */
> -int monitor_set_cpu(int cpu_index)
> +int monitor_set_cpu(Monitor *mon, int cpu_index)
>  {
>      CPUState *cpu;
>  
> @@ -264,8 +264,8 @@ int monitor_set_cpu(int cpu_index)
>      if (cpu == NULL) {
>          return -1;
>      }
> -    g_free(cur_mon->mon_cpu_path);
> -    cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> +    g_free(mon->mon_cpu_path);
> +    mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
>      return 0;
>  }
>  
> @@ -286,7 +286,7 @@ static CPUState *mon_get_cpu_sync(bool synchronize)
>          if (!first_cpu) {
>              return NULL;
>          }
> -        monitor_set_cpu(first_cpu->cpu_index);
> +        monitor_set_cpu(cur_mon, first_cpu->cpu_index);
>          cpu = first_cpu;
>      }
>      assert(cpu != NULL);

Patch looks good otherwise.

[PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Kevin Wolf]

cur_mon really needs to be coroutine-local as soon as we move monitor
command handlers to coroutines and let them yield. As a first step, just
remove all direct accesses to cur_mon so that we can implement this in
the getter function later.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/monitor/monitor.h  |  3 ++-
 audio/wavcapture.c         |  8 ++++----
 dump/dump.c                |  2 +-
 hw/scsi/vhost-scsi.c       |  2 +-
 hw/virtio/vhost-vsock.c    |  2 +-
 migration/fd.c             |  4 ++--
 monitor/hmp.c              | 10 +++++-----
 monitor/misc.c             | 14 +++++++++-----
 monitor/monitor.c          | 15 ++++++++++++++-
 monitor/qmp-cmds-control.c |  2 ++
 monitor/qmp-cmds.c         |  2 +-
 monitor/qmp.c              |  6 +++---
 net/socket.c               |  2 +-
 net/tap.c                  |  6 +++---
 stubs/monitor-core.c       |  5 ++++-
 tests/test-util-sockets.c  | 22 +++++++++++-----------
 trace/control.c            |  2 +-
 util/qemu-error.c          |  4 ++--
 util/qemu-print.c          |  3 ++-
 util/qemu-sockets.c        |  1 +
 20 files changed, 70 insertions(+), 45 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 0dcaefd4f9..43cc746078 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -5,7 +5,6 @@
 #include "qapi/qapi-types-misc.h"
 #include "qemu/readline.h"
 
-extern __thread Monitor *cur_mon;
 typedef struct MonitorHMP MonitorHMP;
 typedef struct MonitorOptions MonitorOptions;
 
@@ -13,6 +12,8 @@ typedef struct MonitorOptions MonitorOptions;
 
 extern QemuOptsList qemu_mon_opts;
 
+Monitor *monitor_cur(void);
+void monitor_set_cur(Monitor *mon);
 bool monitor_cur_is_qmp(void);
 
 void monitor_init_globals(void);
diff --git a/audio/wavcapture.c b/audio/wavcapture.c
index 8d7ce2eda1..e7dc97d16e 100644
--- a/audio/wavcapture.c
+++ b/audio/wavcapture.c
@@ -1,5 +1,5 @@
 #include "qemu/osdep.h"
-#include "monitor/monitor.h"
+#include "qemu/qemu-print.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "audio.h"
@@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
     WAVState *wav = opaque;
     char *path = wav->path;
 
-    monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
-                    wav->freq, wav->bits, wav->nchannels,
-                    path ? path : "<not available>", wav->bytes);
+    qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
+                wav->freq, wav->bits, wav->nchannels,
+                path ? path : "<not available>", wav->bytes);
 }
 
 static struct capture_ops wav_capture_ops = {
diff --git a/dump/dump.c b/dump/dump.c
index 248ea06370..36d26159a0 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1989,7 +1989,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
 
 #if !defined(WIN32)
     if (strstart(file, "fd:", &p)) {
-        fd = monitor_get_fd(cur_mon, p, errp);
+        fd = monitor_get_fd(monitor_cur(), p, errp);
         if (fd == -1) {
             return;
         }
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index c1b012aea4..3920825bd6 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -177,7 +177,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
     }
 
     if (vs->conf.vhostfd) {
-        vhostfd = monitor_fd_param(cur_mon, vs->conf.vhostfd, errp);
+        vhostfd = monitor_fd_param(monitor_cur(), vs->conf.vhostfd, errp);
         if (vhostfd == -1) {
             error_prepend(errp, "vhost-scsi: unable to parse vhostfd: ");
             return;
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 4a228f5168..e72c9005b4 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -317,7 +317,7 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
     }
 
     if (vsock->conf.vhostfd) {
-        vhostfd = monitor_fd_param(cur_mon, vsock->conf.vhostfd, errp);
+        vhostfd = monitor_fd_param(monitor_cur(), vsock->conf.vhostfd, errp);
         if (vhostfd == -1) {
             error_prepend(errp, "vhost-vsock: unable to parse vhostfd: ");
             return;
diff --git a/migration/fd.c b/migration/fd.c
index 0a29ecdebf..6f2f50475f 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -26,7 +26,7 @@
 void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
 {
     QIOChannel *ioc;
-    int fd = monitor_get_fd(cur_mon, fdname, errp);
+    int fd = monitor_get_fd(monitor_cur(), fdname, errp);
     if (fd == -1) {
         return;
     }
@@ -55,7 +55,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
 void fd_start_incoming_migration(const char *fdname, Error **errp)
 {
     QIOChannel *ioc;
-    int fd = monitor_fd_param(cur_mon, fdname, errp);
+    int fd = monitor_fd_param(monitor_cur(), fdname, errp);
     if (fd == -1) {
         return;
     }
diff --git a/monitor/hmp.c b/monitor/hmp.c
index d598dd02bb..f609fcf75b 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1301,11 +1301,11 @@ cleanup:
 static void monitor_read(void *opaque, const uint8_t *buf, int size)
 {
     MonitorHMP *mon;
-    Monitor *old_mon = cur_mon;
+    Monitor *old_mon = monitor_cur();
     int i;
 
-    cur_mon = opaque;
-    mon = container_of(cur_mon, MonitorHMP, common);
+    monitor_set_cur(opaque);
+    mon = container_of(monitor_cur(), MonitorHMP, common);
 
     if (mon->rs) {
         for (i = 0; i < size; i++) {
@@ -1313,13 +1313,13 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
         }
     } else {
         if (size == 0 || buf[size - 1] != 0) {
-            monitor_printf(cur_mon, "corrupted command\n");
+            monitor_printf(&mon->common, "corrupted command\n");
         } else {
             handle_hmp_command(mon, (char *)buf);
         }
     }
 
-    cur_mon = old_mon;
+    monitor_set_cur(old_mon);
 }
 
 static void monitor_event(void *opaque, QEMUChrEvent event)
diff --git a/monitor/misc.c b/monitor/misc.c
index bdf49e49e5..6cf7f60872 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -126,13 +126,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
 
     monitor_data_init(&hmp.common, false, true, false);
 
-    old_mon = cur_mon;
-    cur_mon = &hmp.common;
+    old_mon = monitor_cur();
+    monitor_set_cur(&hmp.common);
 
     if (has_cpu_index) {
         int ret = monitor_set_cpu(&hmp.common, cpu_index);
         if (ret < 0) {
-            cur_mon = old_mon;
+            monitor_set_cur(old_mon);
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
                        "a CPU number");
             goto out;
@@ -140,7 +140,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     }
 
     handle_hmp_command(&hmp, command_line);
-    cur_mon = old_mon;
+    monitor_set_cur(old_mon);
 
     qemu_mutex_lock(&hmp.common.mon_lock);
     if (qstring_get_length(hmp.common.outbuf) > 0) {
@@ -258,6 +258,7 @@ static void monitor_init_qmp_commands(void)
 /* Set the current CPU defined by the user. Callers must hold BQL. */
 int monitor_set_cpu(Monitor *mon, int cpu_index)
 {
+    Monitor *cur_mon = monitor_cur();
     CPUState *cpu;
 
     cpu = qemu_get_cpu(cpu_index);
@@ -272,6 +273,7 @@ int monitor_set_cpu(Monitor *mon, int cpu_index)
 /* Callers must hold BQL. */
 static CPUState *mon_get_cpu_sync(bool synchronize)
 {
+    Monitor *cur_mon = monitor_cur();
     CPUState *cpu = NULL;
 
     if (cur_mon->mon_cpu_path) {
@@ -1232,6 +1234,7 @@ static void hmp_acl_remove(Monitor *mon, const QDict *qdict)
 
 void qmp_getfd(const char *fdname, Error **errp)
 {
+    Monitor *cur_mon = monitor_cur();
     mon_fd_t *monfd;
     int fd, tmp_fd;
 
@@ -1272,6 +1275,7 @@ void qmp_getfd(const char *fdname, Error **errp)
 
 void qmp_closefd(const char *fdname, Error **errp)
 {
+    Monitor *cur_mon = monitor_cur();
     mon_fd_t *monfd;
     int tmp_fd;
 
@@ -1361,7 +1365,7 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
                       const char *opaque, Error **errp)
 {
     int fd;
-    Monitor *mon = cur_mon;
+    Monitor *mon = monitor_cur();
     AddfdInfo *fdinfo;
 
     fd = qemu_chr_fe_get_msgfd(&mon->chr);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 125494410a..182ba136b4 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -66,13 +66,24 @@ MonitorList mon_list;
 int mon_refcount;
 static bool monitor_destroyed;
 
-__thread Monitor *cur_mon;
+static __thread Monitor *cur_monitor;
+
+Monitor *monitor_cur(void)
+{
+    return cur_monitor;
+}
+
+void monitor_set_cur(Monitor *mon)
+{
+    cur_monitor = mon;
+}
 
 /**
  * Is the current monitor, if any, a QMP monitor?
  */
 bool monitor_cur_is_qmp(void)
 {
+    Monitor *cur_mon = monitor_cur();
     return cur_mon && monitor_is_qmp(cur_mon);
 }
 
@@ -209,6 +220,7 @@ int monitor_printf(Monitor *mon, const char *fmt, ...)
  */
 int error_vprintf(const char *fmt, va_list ap)
 {
+    Monitor *cur_mon = monitor_cur();
     if (cur_mon && !monitor_cur_is_qmp()) {
         return monitor_vprintf(cur_mon, fmt, ap);
     }
@@ -217,6 +229,7 @@ int error_vprintf(const char *fmt, va_list ap)
 
 int error_vprintf_unless_qmp(const char *fmt, va_list ap)
 {
+    Monitor *cur_mon = monitor_cur();
     if (!cur_mon) {
         return vfprintf(stderr, fmt, ap);
     }
diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c
index 8f04cfa6e6..a456762f6a 100644
--- a/monitor/qmp-cmds-control.c
+++ b/monitor/qmp-cmds-control.c
@@ -69,6 +69,7 @@ static bool qmp_caps_accept(MonitorQMP *mon, QMPCapabilityList *list,
 void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
                           Error **errp)
 {
+    Monitor *cur_mon = monitor_cur();
     MonitorQMP *mon;
 
     assert(monitor_is_qmp(cur_mon));
@@ -119,6 +120,7 @@ static void query_commands_cb(const QmpCommand *cmd, void *opaque)
 CommandInfoList *qmp_query_commands(Error **errp)
 {
     CommandInfoList *list = NULL;
+    Monitor *cur_mon = monitor_cur();
     MonitorQMP *mon;
 
     assert(monitor_is_qmp(cur_mon));
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 864cbfa32e..c7bf6bb4dc 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -327,7 +327,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
     Chardev *s;
     int fd;
 
-    fd = monitor_get_fd(cur_mon, fdname, errp);
+    fd = monitor_get_fd(monitor_cur(), fdname, errp);
     if (fd < 0) {
         return;
     }
diff --git a/monitor/qmp.c b/monitor/qmp.c
index d433ceae5b..5e9abd4711 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -139,12 +139,12 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
     QDict *rsp;
     QDict *error;
 
-    old_mon = cur_mon;
-    cur_mon = &mon->common;
+    old_mon = monitor_cur();
+    monitor_set_cur(&mon->common);
 
     rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
 
-    cur_mon = old_mon;
+    monitor_set_cur(old_mon);
 
     if (mon->commands == &qmp_cap_negotiation_commands) {
         error = qdict_get_qdict(rsp, "error");
diff --git a/net/socket.c b/net/socket.c
index c92354049b..93bee968a8 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -727,7 +727,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
     if (sock->has_fd) {
         int fd;
 
-        fd = monitor_fd_param(cur_mon, sock->fd, errp);
+        fd = monitor_fd_param(monitor_cur(), sock->fd, errp);
         if (fd == -1) {
             return -1;
         }
diff --git a/net/tap.c b/net/tap.c
index 6207f61f84..fd7d15936d 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -689,7 +689,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         }
 
         if (vhostfdname) {
-            vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
+            vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, &err);
             if (vhostfd == -1) {
                 if (tap->has_vhostforce && tap->vhostforce) {
                     error_propagate(errp, err);
@@ -789,7 +789,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
             return -1;
         }
 
-        fd = monitor_fd_param(cur_mon, tap->fd, &err);
+        fd = monitor_fd_param(monitor_cur(), tap->fd, &err);
         if (fd == -1) {
             error_propagate(errp, err);
             return -1;
@@ -836,7 +836,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
         }
 
         for (i = 0; i < nfds; i++) {
-            fd = monitor_fd_param(cur_mon, fds[i], &err);
+            fd = monitor_fd_param(monitor_cur(), fds[i], &err);
             if (fd == -1) {
                 error_propagate(errp, err);
                 ret = -1;
diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index 6cff1c4e1d..0cd2d864b2 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -3,7 +3,10 @@
 #include "qemu-common.h"
 #include "qapi/qapi-emit-events.h"
 
-__thread Monitor *cur_mon;
+Monitor *monitor_cur(void)
+{
+    return NULL;
+}
 
 void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
 {
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 2ca1e99f17..36fabb5e46 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -53,27 +53,27 @@ static void test_fd_is_socket_good(void)
 static int mon_fd = -1;
 static const char *mon_fdname;
 
-int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
-{
-    g_assert(cur_mon);
-    g_assert(mon == cur_mon);
-    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
-        error_setg(errp, "No fd named %s", fdname);
-        return -1;
-    }
-    return dup(mon_fd);
-}
-
 /* Syms in libqemustub.a are discarded at .o file granularity.
  * To replace monitor_get_fd() we must ensure everything in
  * stubs/monitor.c is defined, to make sure monitor.o is discarded
  * otherwise we get duplicate syms at link time.
  */
 __thread Monitor *cur_mon;
+Monitor *monitor_cur(void) { return cur_mon; }
 int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
 void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp) {}
 void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp) {}
 
+int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
+{
+    g_assert(cur_mon);
+    g_assert(mon == cur_mon);
+    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
+        error_setg(errp, "No fd named %s", fdname);
+        return -1;
+    }
+    return dup(mon_fd);
+}
 
 static void test_socket_fd_pass_name_good(void)
 {
diff --git a/trace/control.c b/trace/control.c
index 2ffe000818..62993daf64 100644
--- a/trace/control.c
+++ b/trace/control.c
@@ -176,7 +176,7 @@ void trace_enable_events(const char *line_buf)
 {
     if (is_help_option(line_buf)) {
         trace_list_events();
-        if (cur_mon == NULL) {
+        if (monitor_cur() == NULL) {
             exit(0);
         }
     } else {
diff --git a/util/qemu-error.c b/util/qemu-error.c
index dac7c7dc50..8d4ed723f5 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -169,7 +169,7 @@ static void print_loc(void)
     int i;
     const char *const *argp;
 
-    if (!cur_mon && progname) {
+    if (!monitor_cur() && progname) {
         fprintf(stderr, "%s:", progname);
         sep = " ";
     }
@@ -206,7 +206,7 @@ static void vreport(report_type type, const char *fmt, va_list ap)
     GTimeVal tv;
     gchar *timestr;
 
-    if (error_with_timestamp && !cur_mon) {
+    if (error_with_timestamp && !monitor_cur()) {
         g_get_current_time(&tv);
         timestr = g_time_val_to_iso8601(&tv);
         error_printf("%s ", timestr);
diff --git a/util/qemu-print.c b/util/qemu-print.c
index e79d6b8396..69ba612f56 100644
--- a/util/qemu-print.c
+++ b/util/qemu-print.c
@@ -20,6 +20,7 @@
  */
 int qemu_vprintf(const char *fmt, va_list ap)
 {
+    Monitor *cur_mon = monitor_cur();
     if (cur_mon) {
         return monitor_vprintf(cur_mon, fmt, ap);
     }
@@ -48,7 +49,7 @@ int qemu_printf(const char *fmt, ...)
 int qemu_vfprintf(FILE *stream, const char *fmt, va_list ap)
 {
     if (!stream) {
-        return monitor_vprintf(cur_mon, fmt, ap);
+        return monitor_vprintf(monitor_cur(), fmt, ap);
     }
     return vfprintf(stream, fmt, ap);
 }
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b37d288866..40c18ba142 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1092,6 +1092,7 @@ fail:
 
 static int socket_get_fd(const char *fdstr, int num, Error **errp)
 {
+    Monitor *cur_mon = monitor_cur();
     int fd;
     if (num != 1) {
         error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");
-- 
2.25.4

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Eric Blake]

On 5/28/20 10:37 AM, Kevin Wolf wrote:
> cur_mon really needs to be coroutine-local as soon as we move monitor
> command handlers to coroutines and let them yield. As a first step, just
> remove all direct accesses to cur_mon so that we can implement this in
> the getter function later.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---


> +++ b/audio/wavcapture.c
> @@ -1,5 +1,5 @@
>   #include "qemu/osdep.h"
> -#include "monitor/monitor.h"
> +#include "qemu/qemu-print.h"
>   #include "qapi/error.h"
>   #include "qemu/error-report.h"
>   #include "audio.h"
> @@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
>       WAVState *wav = opaque;
>       char *path = wav->path;
>   
> -    monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> -                    wav->freq, wav->bits, wav->nchannels,
> -                    path ? path : "<not available>", wav->bytes);
> +    qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> +                wav->freq, wav->bits, wav->nchannels,
> +                path ? path : "<not available>", wav->bytes);

Why the change to qemu_printf() here instead of a call to monitor_cur() 
as is done in the rest of the patch?

> +++ b/stubs/monitor-core.c
> @@ -3,7 +3,10 @@
>   #include "qemu-common.h"
>   #include "qapi/qapi-emit-events.h"
>   
> -__thread Monitor *cur_mon;
> +Monitor *monitor_cur(void)
> +{
> +    return NULL;
> +}
>   

monitor_set_cur() didn't need a stub?  Odd, but I guess it's okay.

Otherwise the patch looks fine to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Kevin Wolf]

Am 28.05.2020 um 20:31 hat Eric Blake geschrieben:
> On 5/28/20 10:37 AM, Kevin Wolf wrote:
> > cur_mon really needs to be coroutine-local as soon as we move monitor
> > command handlers to coroutines and let them yield. As a first step, just
> > remove all direct accesses to cur_mon so that we can implement this in
> > the getter function later.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> 
> > +++ b/audio/wavcapture.c
> > @@ -1,5 +1,5 @@
> >   #include "qemu/osdep.h"
> > -#include "monitor/monitor.h"
> > +#include "qemu/qemu-print.h"
> >   #include "qapi/error.h"
> >   #include "qemu/error-report.h"
> >   #include "audio.h"
> > @@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
> >       WAVState *wav = opaque;
> >       char *path = wav->path;
> > -    monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> > -                    wav->freq, wav->bits, wav->nchannels,
> > -                    path ? path : "<not available>", wav->bytes);
> > +    qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> > +                wav->freq, wav->bits, wav->nchannels,
> > +                path ? path : "<not available>", wav->bytes);
> 
> Why the change to qemu_printf() here instead of a call to
> monitor_cur() as is done in the rest of the patch?

There was actually only one other monitor_printf() call for cur_mon, and
that was in the monitor core, so I think there's no reason to switch
that one.

If you prefer, I can call monitor_cur() directly here, but I thought not
calling it from everywhere when there is already a wrapper like
qemu_printf() would be a good thing.

Maybe wav_capture_info() should actually be passed a Monitor object so
we don't need either.

> > +++ b/stubs/monitor-core.c
> > @@ -3,7 +3,10 @@
> >   #include "qemu-common.h"
> >   #include "qapi/qapi-emit-events.h"
> > -__thread Monitor *cur_mon;
> > +Monitor *monitor_cur(void)
> > +{
> > +    return NULL;
> > +}
> 
> monitor_set_cur() didn't need a stub?  Odd, but I guess it's okay.

Not that odd, only the code to handle monitor commands ever sets the
current monitor. But if you use the stubs, you don't have a monitor.

Kevin

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Eric Blake]

On 5/28/20 10:37 AM, Kevin Wolf wrote:
> cur_mon really needs to be coroutine-local as soon as we move monitor
> command handlers to coroutines and let them yield. As a first step, just
> remove all direct accesses to cur_mon so that we can implement this in
> the getter function later.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +++ b/monitor/misc.c

> @@ -258,6 +258,7 @@ static void monitor_init_qmp_commands(void)
>   /* Set the current CPU defined by the user. Callers must hold BQL. */
>   int monitor_set_cpu(Monitor *mon, int cpu_index)
>   {
> +    Monitor *cur_mon = monitor_cur();
>       CPUState *cpu;
>   
>       cpu = qemu_get_cpu(cpu_index);

Bogus hunk, after patch 1/12.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> cur_mon really needs to be coroutine-local as soon as we move monitor
> command handlers to coroutines and let them yield. As a first step, just
> remove all direct accesses to cur_mon so that we can implement this in
> the getter function later.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/monitor/monitor.h  |  3 ++-
>  audio/wavcapture.c         |  8 ++++----
>  dump/dump.c                |  2 +-
>  hw/scsi/vhost-scsi.c       |  2 +-
>  hw/virtio/vhost-vsock.c    |  2 +-
>  migration/fd.c             |  4 ++--
>  monitor/hmp.c              | 10 +++++-----
>  monitor/misc.c             | 14 +++++++++-----
>  monitor/monitor.c          | 15 ++++++++++++++-
>  monitor/qmp-cmds-control.c |  2 ++
>  monitor/qmp-cmds.c         |  2 +-
>  monitor/qmp.c              |  6 +++---
>  net/socket.c               |  2 +-
>  net/tap.c                  |  6 +++---
>  stubs/monitor-core.c       |  5 ++++-
>  tests/test-util-sockets.c  | 22 +++++++++++-----------
>  trace/control.c            |  2 +-
>  util/qemu-error.c          |  4 ++--
>  util/qemu-print.c          |  3 ++-
>  util/qemu-sockets.c        |  1 +
>  20 files changed, 70 insertions(+), 45 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 0dcaefd4f9..43cc746078 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -5,7 +5,6 @@
>  #include "qapi/qapi-types-misc.h"
>  #include "qemu/readline.h"
>  
> -extern __thread Monitor *cur_mon;
>  typedef struct MonitorHMP MonitorHMP;
>  typedef struct MonitorOptions MonitorOptions;
>  
> @@ -13,6 +12,8 @@ typedef struct MonitorOptions MonitorOptions;
>  
>  extern QemuOptsList qemu_mon_opts;
>  
> +Monitor *monitor_cur(void);
> +void monitor_set_cur(Monitor *mon);
>  bool monitor_cur_is_qmp(void);
>  
>  void monitor_init_globals(void);
> diff --git a/audio/wavcapture.c b/audio/wavcapture.c
> index 8d7ce2eda1..e7dc97d16e 100644
> --- a/audio/wavcapture.c
> +++ b/audio/wavcapture.c
> @@ -1,5 +1,5 @@
>  #include "qemu/osdep.h"
> -#include "monitor/monitor.h"
> +#include "qemu/qemu-print.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "audio.h"
> @@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
>      WAVState *wav = opaque;
>      char *path = wav->path;
>  
> -    monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> -                    wav->freq, wav->bits, wav->nchannels,
> -                    path ? path : "<not available>", wav->bytes);
> +    qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> +                wav->freq, wav->bits, wav->nchannels,
> +                path ? path : "<not available>", wav->bytes);
>  }
>  

A bit more than purely mechanical transformation.  Fine with me.

>  static struct capture_ops wav_capture_ops = {
> diff --git a/dump/dump.c b/dump/dump.c
> index 248ea06370..36d26159a0 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1989,7 +1989,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>  
>  #if !defined(WIN32)
>      if (strstart(file, "fd:", &p)) {
> -        fd = monitor_get_fd(cur_mon, p, errp);
> +        fd = monitor_get_fd(monitor_cur(), p, errp);

All callers pass cur_mon.  Perhaps we'd be better off without the
parameter.  Observation, not demand.

>          if (fd == -1) {
>              return;
>          }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index c1b012aea4..3920825bd6 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -177,7 +177,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (vs->conf.vhostfd) {
> -        vhostfd = monitor_fd_param(cur_mon, vs->conf.vhostfd, errp);
> +        vhostfd = monitor_fd_param(monitor_cur(), vs->conf.vhostfd, errp);

Likewise.

>          if (vhostfd == -1) {
>              error_prepend(errp, "vhost-scsi: unable to parse vhostfd: ");
>              return;
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 4a228f5168..e72c9005b4 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -317,7 +317,7 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
>      }
>  
>      if (vsock->conf.vhostfd) {
> -        vhostfd = monitor_fd_param(cur_mon, vsock->conf.vhostfd, errp);
> +        vhostfd = monitor_fd_param(monitor_cur(), vsock->conf.vhostfd, errp);
>          if (vhostfd == -1) {
>              error_prepend(errp, "vhost-vsock: unable to parse vhostfd: ");
>              return;
> diff --git a/migration/fd.c b/migration/fd.c
> index 0a29ecdebf..6f2f50475f 100644
> --- a/migration/fd.c
> +++ b/migration/fd.c
> @@ -26,7 +26,7 @@
>  void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
>  {
>      QIOChannel *ioc;
> -    int fd = monitor_get_fd(cur_mon, fdname, errp);
> +    int fd = monitor_get_fd(monitor_cur(), fdname, errp);
>      if (fd == -1) {
>          return;
>      }
> @@ -55,7 +55,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
>  void fd_start_incoming_migration(const char *fdname, Error **errp)
>  {
>      QIOChannel *ioc;
> -    int fd = monitor_fd_param(cur_mon, fdname, errp);
> +    int fd = monitor_fd_param(monitor_cur(), fdname, errp);
>      if (fd == -1) {
>          return;
>      }
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index d598dd02bb..f609fcf75b 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1301,11 +1301,11 @@ cleanup:
>  static void monitor_read(void *opaque, const uint8_t *buf, int size)
>  {
>      MonitorHMP *mon;
> -    Monitor *old_mon = cur_mon;
> +    Monitor *old_mon = monitor_cur();
>      int i;
>  
> -    cur_mon = opaque;
> -    mon = container_of(cur_mon, MonitorHMP, common);
> +    monitor_set_cur(opaque);
> +    mon = container_of(monitor_cur(), MonitorHMP, common);

Simpler:

       MonitorHMP *mon = container_of(opaque, MonitorHMP, common);

>  
>      if (mon->rs) {
>          for (i = 0; i < size; i++) {
> @@ -1313,13 +1313,13 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
>          }
>      } else {
>          if (size == 0 || buf[size - 1] != 0) {
> -            monitor_printf(cur_mon, "corrupted command\n");
> +            monitor_printf(&mon->common, "corrupted command\n");
>          } else {
>              handle_hmp_command(mon, (char *)buf);
>          }
>      }
>  
> -    cur_mon = old_mon;
> +    monitor_set_cur(old_mon);
>  }
>  
>  static void monitor_event(void *opaque, QEMUChrEvent event)
> diff --git a/monitor/misc.c b/monitor/misc.c
> index bdf49e49e5..6cf7f60872 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -126,13 +126,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>  
>      monitor_data_init(&hmp.common, false, true, false);
>  
> -    old_mon = cur_mon;
> -    cur_mon = &hmp.common;
> +    old_mon = monitor_cur();
> +    monitor_set_cur(&hmp.common);
>  
>      if (has_cpu_index) {
>          int ret = monitor_set_cpu(&hmp.common, cpu_index);
>          if (ret < 0) {
> -            cur_mon = old_mon;
> +            monitor_set_cur(old_mon);
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
>                         "a CPU number");
>              goto out;
> @@ -140,7 +140,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      }
>  
>      handle_hmp_command(&hmp, command_line);
> -    cur_mon = old_mon;
> +    monitor_set_cur(old_mon);
>  
>      qemu_mutex_lock(&hmp.common.mon_lock);
>      if (qstring_get_length(hmp.common.outbuf) > 0) {
> @@ -258,6 +258,7 @@ static void monitor_init_qmp_commands(void)
>  /* Set the current CPU defined by the user. Callers must hold BQL. */
>  int monitor_set_cpu(Monitor *mon, int cpu_index)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      CPUState *cpu;
>  
>      cpu = qemu_get_cpu(cpu_index);

As Eric observed, this hunk adds dead code.

> @@ -272,6 +273,7 @@ int monitor_set_cpu(Monitor *mon, int cpu_index)
>  /* Callers must hold BQL. */
>  static CPUState *mon_get_cpu_sync(bool synchronize)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      CPUState *cpu = NULL;
>  
>      if (cur_mon->mon_cpu_path) {
> @@ -1232,6 +1234,7 @@ static void hmp_acl_remove(Monitor *mon, const QDict *qdict)
>  
>  void qmp_getfd(const char *fdname, Error **errp)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      mon_fd_t *monfd;
>      int fd, tmp_fd;
>  
> @@ -1272,6 +1275,7 @@ void qmp_getfd(const char *fdname, Error **errp)
>  
>  void qmp_closefd(const char *fdname, Error **errp)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      mon_fd_t *monfd;
>      int tmp_fd;
>  
> @@ -1361,7 +1365,7 @@ AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
>                        const char *opaque, Error **errp)
>  {
>      int fd;
> -    Monitor *mon = cur_mon;
> +    Monitor *mon = monitor_cur();
>      AddfdInfo *fdinfo;
>  
>      fd = qemu_chr_fe_get_msgfd(&mon->chr);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 125494410a..182ba136b4 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -66,13 +66,24 @@ MonitorList mon_list;
>  int mon_refcount;
>  static bool monitor_destroyed;
>  
> -__thread Monitor *cur_mon;
> +static __thread Monitor *cur_monitor;
> +
> +Monitor *monitor_cur(void)
> +{
> +    return cur_monitor;
> +}
> +
> +void monitor_set_cur(Monitor *mon)
> +{
> +    cur_monitor = mon;
> +}

All uses of monitor_set_cur() look like this:

    old_mon = monitor_cur();
    monitor_set_cur(new_mon);
    ...
    monitor_set_cur(old_mon);

If we let monitor_set_cur() return the old value, this becomes

    old_mon = monitor_set_cur(new_mon);
    ...
    monitor_set_cur(old_mon);

I like this better.

>  
>  /**
>   * Is the current monitor, if any, a QMP monitor?
>   */
>  bool monitor_cur_is_qmp(void)
>  {
> +    Monitor *cur_mon = monitor_cur();

Blank line between declarations and statements, please.  More of the
same below.

>      return cur_mon && monitor_is_qmp(cur_mon);
>  }
>  
> @@ -209,6 +220,7 @@ int monitor_printf(Monitor *mon, const char *fmt, ...)
>   */
>  int error_vprintf(const char *fmt, va_list ap)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      if (cur_mon && !monitor_cur_is_qmp()) {
>          return monitor_vprintf(cur_mon, fmt, ap);
>      }
> @@ -217,6 +229,7 @@ int error_vprintf(const char *fmt, va_list ap)
>  
>  int error_vprintf_unless_qmp(const char *fmt, va_list ap)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      if (!cur_mon) {
>          return vfprintf(stderr, fmt, ap);
>      }
> diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c
> index 8f04cfa6e6..a456762f6a 100644
> --- a/monitor/qmp-cmds-control.c
> +++ b/monitor/qmp-cmds-control.c
> @@ -69,6 +69,7 @@ static bool qmp_caps_accept(MonitorQMP *mon, QMPCapabilityList *list,
>  void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
>                            Error **errp)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      MonitorQMP *mon;
>  
>      assert(monitor_is_qmp(cur_mon));
> @@ -119,6 +120,7 @@ static void query_commands_cb(const QmpCommand *cmd, void *opaque)
>  CommandInfoList *qmp_query_commands(Error **errp)
>  {
>      CommandInfoList *list = NULL;
> +    Monitor *cur_mon = monitor_cur();
>      MonitorQMP *mon;
>  
>      assert(monitor_is_qmp(cur_mon));
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 864cbfa32e..c7bf6bb4dc 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -327,7 +327,7 @@ void qmp_add_client(const char *protocol, const char *fdname,
>      Chardev *s;
>      int fd;
>  
> -    fd = monitor_get_fd(cur_mon, fdname, errp);
> +    fd = monitor_get_fd(monitor_cur(), fdname, errp);
>      if (fd < 0) {
>          return;
>      }
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index d433ceae5b..5e9abd4711 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -139,12 +139,12 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>      QDict *rsp;
>      QDict *error;
>  
> -    old_mon = cur_mon;
> -    cur_mon = &mon->common;
> +    old_mon = monitor_cur();
> +    monitor_set_cur(&mon->common);
>  
>      rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
>  
> -    cur_mon = old_mon;
> +    monitor_set_cur(old_mon);
>  
>      if (mon->commands == &qmp_cap_negotiation_commands) {
>          error = qdict_get_qdict(rsp, "error");
> diff --git a/net/socket.c b/net/socket.c
> index c92354049b..93bee968a8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -727,7 +727,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
>      if (sock->has_fd) {
>          int fd;
>  
> -        fd = monitor_fd_param(cur_mon, sock->fd, errp);
> +        fd = monitor_fd_param(monitor_cur(), sock->fd, errp);
>          if (fd == -1) {
>              return -1;
>          }
> diff --git a/net/tap.c b/net/tap.c
> index 6207f61f84..fd7d15936d 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -689,7 +689,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>          }
>  
>          if (vhostfdname) {
> -            vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> +            vhostfd = monitor_fd_param(monitor_cur(), vhostfdname, &err);
>              if (vhostfd == -1) {
>                  if (tap->has_vhostforce && tap->vhostforce) {
>                      error_propagate(errp, err);
> @@ -789,7 +789,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>              return -1;
>          }
>  
> -        fd = monitor_fd_param(cur_mon, tap->fd, &err);
> +        fd = monitor_fd_param(monitor_cur(), tap->fd, &err);
>          if (fd == -1) {
>              error_propagate(errp, err);
>              return -1;
> @@ -836,7 +836,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>          }
>  
>          for (i = 0; i < nfds; i++) {
> -            fd = monitor_fd_param(cur_mon, fds[i], &err);
> +            fd = monitor_fd_param(monitor_cur(), fds[i], &err);
>              if (fd == -1) {
>                  error_propagate(errp, err);
>                  ret = -1;
> diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> index 6cff1c4e1d..0cd2d864b2 100644
> --- a/stubs/monitor-core.c
> +++ b/stubs/monitor-core.c
> @@ -3,7 +3,10 @@
>  #include "qemu-common.h"
>  #include "qapi/qapi-emit-events.h"
>  
> -__thread Monitor *cur_mon;
> +Monitor *monitor_cur(void)
> +{
> +    return NULL;
> +}

Is this meant to be called?  If not, abort().

>  
>  void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>  {
> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
> index 2ca1e99f17..36fabb5e46 100644
> --- a/tests/test-util-sockets.c
> +++ b/tests/test-util-sockets.c
> @@ -53,27 +53,27 @@ static void test_fd_is_socket_good(void)
>  static int mon_fd = -1;
>  static const char *mon_fdname;
>  
> -int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> -{
> -    g_assert(cur_mon);
> -    g_assert(mon == cur_mon);
> -    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
> -        error_setg(errp, "No fd named %s", fdname);
> -        return -1;
> -    }
> -    return dup(mon_fd);
> -}
> -
>  /* Syms in libqemustub.a are discarded at .o file granularity.
>   * To replace monitor_get_fd() we must ensure everything in
>   * stubs/monitor.c is defined, to make sure monitor.o is discarded
>   * otherwise we get duplicate syms at link time.
>   */
>  __thread Monitor *cur_mon;

Hmm.  Since monitor.o's @cur_mon now has internal linkage, the comment
doesn't apply to @cur_mon anymore.  Easy to fix: move the variable
before the comment.  Bonus: you don't have to move monitor_get_fd()
then.

Hmm^2, the comment is stale:

* "libqemustub.a"

  Gone since Commit ebedb37c8d "Makefile: Remove libqemustub.a".  Almost
  three years.  git-grep finds three more occurences, all bogus.

* "stubs/monitor.c"

  Commit 6ede81d576 "stubs: Update monitor stubs for
  qemu-storage-daemon" moved stuff from stubs/monitor.c to
  monitor-core.c.

* "we must ensure everything in stubs/monitor.c is defined"

  We don't.

Mind to clean that up beforehand?

> +Monitor *monitor_cur(void) { return cur_mon; }

Is this meant to be called?  If not, abort().

>  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
>  void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp) {}
>  void monitor_init_hmp(Chardev *chr, bool use_readline, Error **errp) {}
>  
> +int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> +{
> +    g_assert(cur_mon);
> +    g_assert(mon == cur_mon);
> +    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
> +        error_setg(errp, "No fd named %s", fdname);
> +        return -1;
> +    }
> +    return dup(mon_fd);
> +}
>  
>  static void test_socket_fd_pass_name_good(void)
>  {
> diff --git a/trace/control.c b/trace/control.c
> index 2ffe000818..62993daf64 100644
> --- a/trace/control.c
> +++ b/trace/control.c
> @@ -176,7 +176,7 @@ void trace_enable_events(const char *line_buf)
>  {
>      if (is_help_option(line_buf)) {
>          trace_list_events();
> -        if (cur_mon == NULL) {
> +        if (monitor_cur() == NULL) {
>              exit(0);
>          }
>      } else {
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index dac7c7dc50..8d4ed723f5 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -169,7 +169,7 @@ static void print_loc(void)
>      int i;
>      const char *const *argp;
>  
> -    if (!cur_mon && progname) {
> +    if (!monitor_cur() && progname) {
>          fprintf(stderr, "%s:", progname);
>          sep = " ";
>      }
> @@ -206,7 +206,7 @@ static void vreport(report_type type, const char *fmt, va_list ap)
>      GTimeVal tv;
>      gchar *timestr;
>  
> -    if (error_with_timestamp && !cur_mon) {
> +    if (error_with_timestamp && !monitor_cur()) {
>          g_get_current_time(&tv);
>          timestr = g_time_val_to_iso8601(&tv);
>          error_printf("%s ", timestr);
> diff --git a/util/qemu-print.c b/util/qemu-print.c
> index e79d6b8396..69ba612f56 100644
> --- a/util/qemu-print.c
> +++ b/util/qemu-print.c
> @@ -20,6 +20,7 @@
>   */
>  int qemu_vprintf(const char *fmt, va_list ap)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      if (cur_mon) {
>          return monitor_vprintf(cur_mon, fmt, ap);
>      }
> @@ -48,7 +49,7 @@ int qemu_printf(const char *fmt, ...)
>  int qemu_vfprintf(FILE *stream, const char *fmt, va_list ap)
>  {
>      if (!stream) {
> -        return monitor_vprintf(cur_mon, fmt, ap);
> +        return monitor_vprintf(monitor_cur(), fmt, ap);
>      }
>      return vfprintf(stream, fmt, ap);
>  }
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index b37d288866..40c18ba142 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1092,6 +1092,7 @@ fail:
>  
>  static int socket_get_fd(const char *fdstr, int num, Error **errp)
>  {
> +    Monitor *cur_mon = monitor_cur();
>      int fd;
>      if (num != 1) {
>          error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Kevin Wolf]

Am 04.08.2020 um 14:46 hat Markus Armbruster geschrieben:
> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> > index d598dd02bb..f609fcf75b 100644
> > --- a/monitor/hmp.c
> > +++ b/monitor/hmp.c
> > @@ -1301,11 +1301,11 @@ cleanup:
> >  static void monitor_read(void *opaque, const uint8_t *buf, int size)
> >  {
> >      MonitorHMP *mon;
> > -    Monitor *old_mon = cur_mon;
> > +    Monitor *old_mon = monitor_cur();
> >      int i;
> >  
> > -    cur_mon = opaque;
> > -    mon = container_of(cur_mon, MonitorHMP, common);
> > +    monitor_set_cur(opaque);
> > +    mon = container_of(monitor_cur(), MonitorHMP, common);
> 
> Simpler:
> 
>        MonitorHMP *mon = container_of(opaque, MonitorHMP, common);

opaque is void*, so it doesn't have a field 'common'.

> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 125494410a..182ba136b4 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -66,13 +66,24 @@ MonitorList mon_list;
> >  int mon_refcount;
> >  static bool monitor_destroyed;
> >  
> > -__thread Monitor *cur_mon;
> > +static __thread Monitor *cur_monitor;
> > +
> > +Monitor *monitor_cur(void)
> > +{
> > +    return cur_monitor;
> > +}
> > +
> > +void monitor_set_cur(Monitor *mon)
> > +{
> > +    cur_monitor = mon;
> > +}
> 
> All uses of monitor_set_cur() look like this:
> 
>     old_mon = monitor_cur();
>     monitor_set_cur(new_mon);
>     ...
>     monitor_set_cur(old_mon);
> 
> If we let monitor_set_cur() return the old value, this becomes
> 
>     old_mon = monitor_set_cur(new_mon);
>     ...
>     monitor_set_cur(old_mon);
> 
> I like this better.

Fine with me.

> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> > index 6cff1c4e1d..0cd2d864b2 100644
> > --- a/stubs/monitor-core.c
> > +++ b/stubs/monitor-core.c
> > @@ -3,7 +3,10 @@
> >  #include "qemu-common.h"
> >  #include "qapi/qapi-emit-events.h"
> >  
> > -__thread Monitor *cur_mon;
> > +Monitor *monitor_cur(void)
> > +{
> > +    return NULL;
> > +}
> 
> Is this meant to be called?  If not, abort().

error_report() and friends are supposed to be called pretty much
everywhere, so I'd say yes.

Kevin

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 04.08.2020 um 14:46 hat Markus Armbruster geschrieben:
>> > diff --git a/monitor/hmp.c b/monitor/hmp.c
>> > index d598dd02bb..f609fcf75b 100644
>> > --- a/monitor/hmp.c
>> > +++ b/monitor/hmp.c
>> > @@ -1301,11 +1301,11 @@ cleanup:
>> >  static void monitor_read(void *opaque, const uint8_t *buf, int size)
>> >  {
>> >      MonitorHMP *mon;
>> > -    Monitor *old_mon = cur_mon;
>> > +    Monitor *old_mon = monitor_cur();
>> >      int i;
>> >  
>> > -    cur_mon = opaque;
>> > -    mon = container_of(cur_mon, MonitorHMP, common);
>> > +    monitor_set_cur(opaque);
>> > +    mon = container_of(monitor_cur(), MonitorHMP, common);
>> 
>> Simpler:
>> 
>>        MonitorHMP *mon = container_of(opaque, MonitorHMP, common);
>
> opaque is void*, so it doesn't have a field 'common'.

I actually compile-tested before I sent this.  For once ;)

Here's container_of():

    #define container_of(ptr, type, member) ({                      \
            const typeof(((type *) 0)->member) *__mptr = (ptr);     \
            (type *) ((char *) __mptr - offsetof(type, member));})

Its first argument's only use is as an initializer for a pointer
variable.  Both type * and void * work fine there.

>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index 125494410a..182ba136b4 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -66,13 +66,24 @@ MonitorList mon_list;
>> >  int mon_refcount;
>> >  static bool monitor_destroyed;
>> >  
>> > -__thread Monitor *cur_mon;
>> > +static __thread Monitor *cur_monitor;
>> > +
>> > +Monitor *monitor_cur(void)
>> > +{
>> > +    return cur_monitor;
>> > +}
>> > +
>> > +void monitor_set_cur(Monitor *mon)
>> > +{
>> > +    cur_monitor = mon;
>> > +}
>> 
>> All uses of monitor_set_cur() look like this:
>> 
>>     old_mon = monitor_cur();
>>     monitor_set_cur(new_mon);
>>     ...
>>     monitor_set_cur(old_mon);
>> 
>> If we let monitor_set_cur() return the old value, this becomes
>> 
>>     old_mon = monitor_set_cur(new_mon);
>>     ...
>>     monitor_set_cur(old_mon);
>> 
>> I like this better.
>
> Fine with me.
>
>> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
>> > index 6cff1c4e1d..0cd2d864b2 100644
>> > --- a/stubs/monitor-core.c
>> > +++ b/stubs/monitor-core.c
>> > @@ -3,7 +3,10 @@
>> >  #include "qemu-common.h"
>> >  #include "qapi/qapi-emit-events.h"
>> >  
>> > -__thread Monitor *cur_mon;
>> > +Monitor *monitor_cur(void)
>> > +{
>> > +    return NULL;
>> > +}
>> 
>> Is this meant to be called?  If not, abort().
>
> error_report() and friends are supposed to be called pretty much
> everywhere, so I'd say yes.

Okay.

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Kevin Wolf]

Am 05.08.2020 um 09:19 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 04.08.2020 um 14:46 hat Markus Armbruster geschrieben:
> >> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> >> > index d598dd02bb..f609fcf75b 100644
> >> > --- a/monitor/hmp.c
> >> > +++ b/monitor/hmp.c
> >> > @@ -1301,11 +1301,11 @@ cleanup:
> >> >  static void monitor_read(void *opaque, const uint8_t *buf, int size)
> >> >  {
> >> >      MonitorHMP *mon;
> >> > -    Monitor *old_mon = cur_mon;
> >> > +    Monitor *old_mon = monitor_cur();
> >> >      int i;
> >> >  
> >> > -    cur_mon = opaque;
> >> > -    mon = container_of(cur_mon, MonitorHMP, common);
> >> > +    monitor_set_cur(opaque);
> >> > +    mon = container_of(monitor_cur(), MonitorHMP, common);
> >> 
> >> Simpler:
> >> 
> >>        MonitorHMP *mon = container_of(opaque, MonitorHMP, common);
> >
> > opaque is void*, so it doesn't have a field 'common'.
> 
> I actually compile-tested before I sent this.  For once ;)
> 
> Here's container_of():
> 
>     #define container_of(ptr, type, member) ({                      \
>             const typeof(((type *) 0)->member) *__mptr = (ptr);     \
>             (type *) ((char *) __mptr - offsetof(type, member));})
> 
> Its first argument's only use is as an initializer for a pointer
> variable.  Both type * and void * work fine there.

Ah, we just lose type checking.

That's what I get for replying from what I remember from over two months
ago. I was pretty sure I didn't like this way, but went with it because
the other way didn't work. Maybe I just assumed it didn't work, or tried
something different that actually fails. Who knows.

Kevin

Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
>
>> cur_mon really needs to be coroutine-local as soon as we move monitor
>> command handlers to coroutines and let them yield. As a first step, just
>> remove all direct accesses to cur_mon so that we can implement this in
>> the getter function later.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
[...]
>> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
>> index 2ca1e99f17..36fabb5e46 100644
>> --- a/tests/test-util-sockets.c
>> +++ b/tests/test-util-sockets.c
>> @@ -53,27 +53,27 @@ static void test_fd_is_socket_good(void)
>>  static int mon_fd = -1;
>>  static const char *mon_fdname;
>>  
>> -int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>> -{
>> -    g_assert(cur_mon);
>> -    g_assert(mon == cur_mon);
>> -    if (mon_fd == -1 || !g_str_equal(mon_fdname, fdname)) {
>> -        error_setg(errp, "No fd named %s", fdname);
>> -        return -1;
>> -    }
>> -    return dup(mon_fd);
>> -}
>> -
>>  /* Syms in libqemustub.a are discarded at .o file granularity.
>>   * To replace monitor_get_fd() we must ensure everything in
>>   * stubs/monitor.c is defined, to make sure monitor.o is discarded
>>   * otherwise we get duplicate syms at link time.
>>   */
>>  __thread Monitor *cur_mon;
>
> Hmm.  Since monitor.o's @cur_mon now has internal linkage, the comment
> doesn't apply to @cur_mon anymore.  Easy to fix: move the variable
> before the comment.  Bonus: you don't have to move monitor_get_fd()
> then.
>
> Hmm^2, the comment is stale:
>
> * "libqemustub.a"
>
>   Gone since Commit ebedb37c8d "Makefile: Remove libqemustub.a".  Almost
>   three years.  git-grep finds three more occurences, all bogus.

Thomas posted a patch:

    Subject: [PATCH 07/11] Get rid of the libqemustub.a remainders
    Message-Id: <20200804170055.2851-8-thuth@redhat.com>

>
> * "stubs/monitor.c"
>
>   Commit 6ede81d576 "stubs: Update monitor stubs for
>   qemu-storage-daemon" moved stuff from stubs/monitor.c to
>   monitor-core.c.
>
> * "we must ensure everything in stubs/monitor.c is defined"
>
>   We don't.

These two remain.

> Mind to clean that up beforehand?

[...]

[PATCH v6 03/12] hmp: Set cur_mon only in handle_hmp_command()

[Kevin Wolf]

cur_mon is updated relatively early in the command handling code even
though only the command handler actually needs it. Move it to
handle_hmp_command().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 monitor/hmp.c  | 23 ++++++++++++-----------
 monitor/misc.c |  7 -------
 2 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index f609fcf75b..79be6f26de 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1061,6 +1061,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
     QDict *qdict;
     const HMPCommand *cmd;
     const char *cmd_start = cmdline;
+    Monitor *old_mon;
 
     trace_handle_hmp_command(mon, cmdline);
 
@@ -1079,7 +1080,12 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
         return;
     }
 
+    /* old_mon is non-NULL when called from qmp_human_monitor_command() */
+    old_mon = monitor_cur();
+    monitor_set_cur(&mon->common);
     cmd->cmd(&mon->common, qdict);
+    monitor_set_cur(old_mon);
+
     qobject_unref(qdict);
 }
 
@@ -1300,26 +1306,21 @@ cleanup:
 
 static void monitor_read(void *opaque, const uint8_t *buf, int size)
 {
-    MonitorHMP *mon;
-    Monitor *old_mon = monitor_cur();
+    Monitor *mon = opaque;
+    MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
     int i;
 
-    monitor_set_cur(opaque);
-    mon = container_of(monitor_cur(), MonitorHMP, common);
-
-    if (mon->rs) {
+    if (hmp_mon->rs) {
         for (i = 0; i < size; i++) {
-            readline_handle_byte(mon->rs, buf[i]);
+            readline_handle_byte(hmp_mon->rs, buf[i]);
         }
     } else {
         if (size == 0 || buf[size - 1] != 0) {
-            monitor_printf(&mon->common, "corrupted command\n");
+            monitor_printf(mon, "corrupted command\n");
         } else {
-            handle_hmp_command(mon, (char *)buf);
+            handle_hmp_command(hmp_mon, (char *)buf);
         }
     }
-
-    monitor_set_cur(old_mon);
 }
 
 static void monitor_event(void *opaque, QEMUChrEvent event)
diff --git a/monitor/misc.c b/monitor/misc.c
index 6cf7f60872..e0ab265726 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -121,18 +121,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
                                 int64_t cpu_index, Error **errp)
 {
     char *output = NULL;
-    Monitor *old_mon;
     MonitorHMP hmp = {};
 
     monitor_data_init(&hmp.common, false, true, false);
 
-    old_mon = monitor_cur();
-    monitor_set_cur(&hmp.common);
-
     if (has_cpu_index) {
         int ret = monitor_set_cpu(&hmp.common, cpu_index);
         if (ret < 0) {
-            monitor_set_cur(old_mon);
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
                        "a CPU number");
             goto out;
@@ -140,7 +135,6 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     }
 
     handle_hmp_command(&hmp, command_line);
-    monitor_set_cur(old_mon);
 
     qemu_mutex_lock(&hmp.common.mon_lock);
     if (qstring_get_length(hmp.common.outbuf) > 0) {
@@ -258,7 +252,6 @@ static void monitor_init_qmp_commands(void)
 /* Set the current CPU defined by the user. Callers must hold BQL. */
 int monitor_set_cpu(Monitor *mon, int cpu_index)
 {
-    Monitor *cur_mon = monitor_cur();
     CPUState *cpu;
 
     cpu = qemu_get_cpu(cpu_index);
-- 
2.25.4

Re: [PATCH v6 03/12] hmp: Set cur_mon only in handle_hmp_command()

[Eric Blake]

On 5/28/20 10:37 AM, Kevin Wolf wrote:
> cur_mon is updated relatively early in the command handling code even
> though only the command handler actually needs it. Move it to
> handle_hmp_command().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   monitor/hmp.c  | 23 ++++++++++++-----------
>   monitor/misc.c |  7 -------
>   2 files changed, 12 insertions(+), 18 deletions(-)
> 

> +++ b/monitor/misc.c

> @@ -258,7 +252,6 @@ static void monitor_init_qmp_commands(void)
>   /* Set the current CPU defined by the user. Callers must hold BQL. */
>   int monitor_set_cpu(Monitor *mon, int cpu_index)
>   {
> -    Monitor *cur_mon = monitor_cur();
>       CPUState *cpu;
>   
>       cpu = qemu_get_cpu(cpu_index);

Bogus churn, will disappear after you fix 2/12.  For the rest of the patch:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v6 03/12] hmp: Set cur_mon only in handle_hmp_command()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> cur_mon is updated relatively early in the command handling code even

@cur_mon doesn't exist anymore (you renamed it to @cur_monitor in the
previous patch).  Either say "The current monitor", or use the actual
variable name.

> though only the command handler actually needs it. Move it to
> handle_hmp_command().

The commit message explains why moving it isn't wrong.  Can you also
explain why you want to move it?

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  monitor/hmp.c  | 23 ++++++++++++-----------
>  monitor/misc.c |  7 -------
>  2 files changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index f609fcf75b..79be6f26de 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1061,6 +1061,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>      QDict *qdict;
>      const HMPCommand *cmd;
>      const char *cmd_start = cmdline;
> +    Monitor *old_mon;
>  
>      trace_handle_hmp_command(mon, cmdline);
>  
> @@ -1079,7 +1080,12 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>          return;
>      }
>  
> +    /* old_mon is non-NULL when called from qmp_human_monitor_command() */
> +    old_mon = monitor_cur();
> +    monitor_set_cur(&mon->common);
>      cmd->cmd(&mon->common, qdict);
> +    monitor_set_cur(old_mon);
> +
>      qobject_unref(qdict);
>  }
>  
> @@ -1300,26 +1306,21 @@ cleanup:
>  
>  static void monitor_read(void *opaque, const uint8_t *buf, int size)
>  {
> -    MonitorHMP *mon;
> -    Monitor *old_mon = monitor_cur();
> +    Monitor *mon = opaque;
> +    MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
>      int i;
>  
> -    monitor_set_cur(opaque);
> -    mon = container_of(monitor_cur(), MonitorHMP, common);
> -
> -    if (mon->rs) {
> +    if (hmp_mon->rs) {
>          for (i = 0; i < size; i++) {
> -            readline_handle_byte(mon->rs, buf[i]);
> +            readline_handle_byte(hmp_mon->rs, buf[i]);
>          }
>      } else {
>          if (size == 0 || buf[size - 1] != 0) {
> -            monitor_printf(&mon->common, "corrupted command\n");
> +            monitor_printf(mon, "corrupted command\n");
>          } else {
> -            handle_hmp_command(mon, (char *)buf);
> +            handle_hmp_command(hmp_mon, (char *)buf);
>          }
>      }
> -
> -    monitor_set_cur(old_mon);
>  }

This does a bit more than just move monitor_set_cur().  Okay.

>  
>  static void monitor_event(void *opaque, QEMUChrEvent event)
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 6cf7f60872..e0ab265726 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -121,18 +121,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>                                  int64_t cpu_index, Error **errp)
>  {
>      char *output = NULL;
> -    Monitor *old_mon;
>      MonitorHMP hmp = {};
>  
>      monitor_data_init(&hmp.common, false, true, false);
>  
> -    old_mon = monitor_cur();
> -    monitor_set_cur(&hmp.common);
> -
>      if (has_cpu_index) {
>          int ret = monitor_set_cpu(&hmp.common, cpu_index);
>          if (ret < 0) {
> -            monitor_set_cur(old_mon);
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
>                         "a CPU number");
>              goto out;
> @@ -140,7 +135,6 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      }
>  
>      handle_hmp_command(&hmp, command_line);
> -    monitor_set_cur(old_mon);
>  
>      qemu_mutex_lock(&hmp.common.mon_lock);
>      if (qstring_get_length(hmp.common.outbuf) > 0) {
> @@ -258,7 +252,6 @@ static void monitor_init_qmp_commands(void)
>  /* Set the current CPU defined by the user. Callers must hold BQL. */
>  int monitor_set_cpu(Monitor *mon, int cpu_index)
>  {
> -    Monitor *cur_mon = monitor_cur();
>      CPUState *cpu;
>  
>      cpu = qemu_get_cpu(cpu_index);

[PATCH v6 04/12] qmp: Assert that no other monitor is active

[Kevin Wolf]

monitor_qmp_dispatch() is never supposed to be called in the context of
another monitor, so assert that monitor_cur() is NULL instead of saving
and restoring it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 monitor/qmp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index 5e9abd4711..a04c512e3a 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -135,16 +135,15 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
 
 static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
 {
-    Monitor *old_mon;
     QDict *rsp;
     QDict *error;
 
-    old_mon = monitor_cur();
+    assert(monitor_cur() == NULL);
     monitor_set_cur(&mon->common);
 
     rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
 
-    monitor_set_cur(old_mon);
+    monitor_set_cur(NULL);
 
     if (mon->commands == &qmp_cap_negotiation_commands) {
         error = qdict_get_qdict(rsp, "error");
-- 
2.25.4

Re: [PATCH v6 04/12] qmp: Assert that no other monitor is active

[Eric Blake]

On 5/28/20 10:37 AM, Kevin Wolf wrote:
> monitor_qmp_dispatch() is never supposed to be called in the context of
> another monitor, so assert that monitor_cur() is NULL instead of saving
> and restoring it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   monitor/qmp.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v6 04/12] qmp: Assert that no other monitor is active

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> monitor_qmp_dispatch() is never supposed to be called in the context of
> another monitor, so assert that monitor_cur() is NULL instead of saving
> and restoring it.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  monitor/qmp.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 5e9abd4711..a04c512e3a 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -135,16 +135,15 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
>  
>  static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>  {
> -    Monitor *old_mon;
>      QDict *rsp;
>      QDict *error;
>  
> -    old_mon = monitor_cur();
> +    assert(monitor_cur() == NULL);

I'd write !monitor_cur().  Matter of taste.

>      monitor_set_cur(&mon->common);
>  
>      rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
>  
> -    monitor_set_cur(old_mon);
> +    monitor_set_cur(NULL);
>  
>      if (mon->commands == &qmp_cap_negotiation_commands) {
>          error = qdict_get_qdict(rsp, "error");

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

[PATCH v6 05/12] qmp: Call monitor_set_cur() only in qmp_dispatch()

[Kevin Wolf]

The correct way to set the current monitor for a coroutine handler is
different that for a blocking handler, so monitor_set_cur() can only be
called in qmp_dispatch().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qapi/qmp/dispatch.h | 3 ++-
 monitor/qmp.c               | 7 +------
 qapi/qmp-dispatch.c         | 8 +++++++-
 qga/main.c                  | 2 +-
 stubs/monitor-core.c        | 4 ++++
 tests/test-qmp-cmds.c       | 6 +++---
 6 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 5a9cf82472..0c2f467028 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -14,6 +14,7 @@
 #ifndef QAPI_QMP_DISPATCH_H
 #define QAPI_QMP_DISPATCH_H
 
+#include "monitor/monitor.h"
 #include "qemu/queue.h"
 
 typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
@@ -49,7 +50,7 @@ const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QDict *qmp_error_response(Error *err);
 QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
-                    bool allow_oob);
+                    bool allow_oob, Monitor *cur_mon);
 bool qmp_is_oob(const QDict *dict);
 
 typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque);
diff --git a/monitor/qmp.c b/monitor/qmp.c
index a04c512e3a..922fdb5541 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -138,12 +138,7 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
     QDict *rsp;
     QDict *error;
 
-    assert(monitor_cur() == NULL);
-    monitor_set_cur(&mon->common);
-
-    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
-
-    monitor_set_cur(NULL);
+    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), &mon->common);
 
     if (mon->commands == &qmp_cap_negotiation_commands) {
         error = qdict_get_qdict(rsp, "error");
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 79347e0864..2fdbc0fba4 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -89,7 +89,7 @@ bool qmp_is_oob(const QDict *dict)
 }
 
 QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
-                    bool allow_oob)
+                    bool allow_oob, Monitor *cur_mon)
 {
     Error *err = NULL;
     bool oob;
@@ -152,7 +152,13 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
         args = qdict_get_qdict(dict, "arguments");
         qobject_ref(args);
     }
+
+    assert(monitor_cur() == NULL);
+    monitor_set_cur(cur_mon);
+
     cmd->fn(args, &ret, &err);
+
+    monitor_set_cur(NULL);
     qobject_unref(args);
     if (err) {
         /* or assert(!ret) after reviewing all handlers: */
diff --git a/qga/main.c b/qga/main.c
index f0e454f28d..1042c4e7d3 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -574,7 +574,7 @@ static void process_event(void *opaque, QObject *obj, Error *err)
     }
 
     g_debug("processing command");
-    rsp = qmp_dispatch(&ga_commands, obj, false);
+    rsp = qmp_dispatch(&ga_commands, obj, false, NULL);
 
 end:
     ret = send_response(s, rsp);
diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index 0cd2d864b2..e493df1027 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -8,6 +8,10 @@ Monitor *monitor_cur(void)
     return NULL;
 }
 
+void monitor_set_cur(Monitor *mon)
+{
+}
+
 void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
 {
 }
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index d12ff47e26..5f1b245e19 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -152,7 +152,7 @@ static QObject *do_qmp_dispatch(bool allow_oob, const char *template, ...)
     req = qdict_from_vjsonf_nofail(template, ap);
     va_end(ap);
 
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob);
+    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
     g_assert(resp);
     ret = qdict_get(resp, "return");
     g_assert(ret);
@@ -175,7 +175,7 @@ static void do_qmp_dispatch_error(bool allow_oob, ErrorClass cls,
     req = qdict_from_vjsonf_nofail(template, ap);
     va_end(ap);
 
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob);
+    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
     g_assert(resp);
     error = qdict_get_qdict(resp, "error");
     g_assert(error);
@@ -231,7 +231,7 @@ static void test_dispatch_cmd_success_response(void)
     QDict *resp;
 
     qdict_put_str(req, "execute", "cmd-success-response");
-    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false, NULL);
     g_assert_null(resp);
     qobject_unref(req);
 }
-- 
2.25.4

Re: [PATCH v6 05/12] qmp: Call monitor_set_cur() only in qmp_dispatch()

[Eric Blake]

On 5/28/20 10:37 AM, Kevin Wolf wrote:
> The correct way to set the current monitor for a coroutine handler is
> different that for a blocking handler, so monitor_set_cur() can only be

s/that/than/

> called in qmp_dispatch().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/qapi/qmp/dispatch.h | 3 ++-
>   monitor/qmp.c               | 7 +------
>   qapi/qmp-dispatch.c         | 8 +++++++-
>   qga/main.c                  | 2 +-
>   stubs/monitor-core.c        | 4 ++++
>   tests/test-qmp-cmds.c       | 6 +++---
>   6 files changed, 18 insertions(+), 12 deletions(-)
> 

> +++ b/stubs/monitor-core.c
> @@ -8,6 +8,10 @@ Monitor *monitor_cur(void)
>       return NULL;
>   }
>   
> +void monitor_set_cur(Monitor *mon)
> +{
> +}
> +

Should this stub assert(!mon)?

Otherwise looks reasonable.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v6 05/12] qmp: Call monitor_set_cur() only in qmp_dispatch()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> The correct way to set the current monitor for a coroutine handler is
> different that for a blocking handler, so monitor_set_cur() can only be

will be different

> called in qmp_dispatch().

needs to be called in

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qapi/qmp/dispatch.h | 3 ++-
>  monitor/qmp.c               | 7 +------
>  qapi/qmp-dispatch.c         | 8 +++++++-
>  qga/main.c                  | 2 +-
>  stubs/monitor-core.c        | 4 ++++
>  tests/test-qmp-cmds.c       | 6 +++---
>  6 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 5a9cf82472..0c2f467028 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -14,6 +14,7 @@
>  #ifndef QAPI_QMP_DISPATCH_H
>  #define QAPI_QMP_DISPATCH_H
>  
> +#include "monitor/monitor.h"
>  #include "qemu/queue.h"
>  
>  typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
> @@ -49,7 +50,7 @@ const char *qmp_command_name(const QmpCommand *cmd);
>  bool qmp_has_success_response(const QmpCommand *cmd);
>  QDict *qmp_error_response(Error *err);
>  QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> -                    bool allow_oob);
> +                    bool allow_oob, Monitor *cur_mon);
>  bool qmp_is_oob(const QDict *dict);
>  
>  typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void *opaque);

Tying dispatch to the current monitor when two out of three users don't
have a monitor is ugly.  Let's not worry about that now.

> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index a04c512e3a..922fdb5541 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -138,12 +138,7 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>      QDict *rsp;
>      QDict *error;
>  
> -    assert(monitor_cur() == NULL);
> -    monitor_set_cur(&mon->common);
> -
> -    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
> -
> -    monitor_set_cur(NULL);
> +    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), &mon->common);

Long line.

>  
>      if (mon->commands == &qmp_cap_negotiation_commands) {
>          error = qdict_get_qdict(rsp, "error");
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 79347e0864..2fdbc0fba4 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -89,7 +89,7 @@ bool qmp_is_oob(const QDict *dict)
>  }
>  
>  QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> -                    bool allow_oob)
> +                    bool allow_oob, Monitor *cur_mon)
>  {
>      Error *err = NULL;
>      bool oob;
> @@ -152,7 +152,13 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
>          args = qdict_get_qdict(dict, "arguments");
>          qobject_ref(args);
>      }
> +
> +    assert(monitor_cur() == NULL);
> +    monitor_set_cur(cur_mon);
> +
>      cmd->fn(args, &ret, &err);
> +
> +    monitor_set_cur(NULL);
>      qobject_unref(args);
>      if (err) {
>          /* or assert(!ret) after reviewing all handlers: */
> diff --git a/qga/main.c b/qga/main.c
> index f0e454f28d..1042c4e7d3 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -574,7 +574,7 @@ static void process_event(void *opaque, QObject *obj, Error *err)
>      }
>  
>      g_debug("processing command");
> -    rsp = qmp_dispatch(&ga_commands, obj, false);
> +    rsp = qmp_dispatch(&ga_commands, obj, false, NULL);
>  
>  end:
>      ret = send_response(s, rsp);
> diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> index 0cd2d864b2..e493df1027 100644
> --- a/stubs/monitor-core.c
> +++ b/stubs/monitor-core.c
> @@ -8,6 +8,10 @@ Monitor *monitor_cur(void)
>      return NULL;
>  }
>  
> +void monitor_set_cur(Monitor *mon)
> +{
> +}
> +
>  void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>  {
>  }
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index d12ff47e26..5f1b245e19 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -152,7 +152,7 @@ static QObject *do_qmp_dispatch(bool allow_oob, const char *template, ...)
>      req = qdict_from_vjsonf_nofail(template, ap);
>      va_end(ap);
>  
> -    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob);
> +    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
>      g_assert(resp);
>      ret = qdict_get(resp, "return");
>      g_assert(ret);
> @@ -175,7 +175,7 @@ static void do_qmp_dispatch_error(bool allow_oob, ErrorClass cls,
>      req = qdict_from_vjsonf_nofail(template, ap);
>      va_end(ap);
>  
> -    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob);
> +    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL);
>      g_assert(resp);
>      error = qdict_get_qdict(resp, "error");
>      g_assert(error);
> @@ -231,7 +231,7 @@ static void test_dispatch_cmd_success_response(void)
>      QDict *resp;
>  
>      qdict_put_str(req, "execute", "cmd-success-response");
> -    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
> +    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false, NULL);
>      g_assert_null(resp);
>      qobject_unref(req);
>  }

This works because qmp_dispatch() doesn't bother to check its @mon
argument is sane, and the test's commands don't use monitor_cur().

[PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Kevin Wolf]

This way, a monitor command handler will still be able to access the
current monitor, but when it yields, all other code code will correctly
get NULL from monitor_cur().

Outside of coroutine context, qemu_coroutine_self() returns the leader
coroutine of the current thread.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/monitor/monitor.h |  2 +-
 monitor/hmp.c             |  4 ++--
 monitor/monitor.c         | 27 +++++++++++++++++++++------
 qapi/qmp-dispatch.c       |  4 ++--
 stubs/monitor-core.c      |  2 +-
 5 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 43cc746078..16072e325c 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions;
 extern QemuOptsList qemu_mon_opts;
 
 Monitor *monitor_cur(void);
-void monitor_set_cur(Monitor *mon);
+void monitor_set_cur(Coroutine *co, Monitor *mon);
 bool monitor_cur_is_qmp(void);
 
 void monitor_init_globals(void);
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 79be6f26de..3e73a4c3ce 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1082,9 +1082,9 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
 
     /* old_mon is non-NULL when called from qmp_human_monitor_command() */
     old_mon = monitor_cur();
-    monitor_set_cur(&mon->common);
+    monitor_set_cur(qemu_coroutine_self(), &mon->common);
     cmd->cmd(&mon->common, qdict);
-    monitor_set_cur(old_mon);
+    monitor_set_cur(qemu_coroutine_self(), old_mon);
 
     qobject_unref(qdict);
 }
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 182ba136b4..35003bb486 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -58,24 +58,38 @@ IOThread *mon_iothread;
 /* Bottom half to dispatch the requests received from I/O thread */
 QEMUBH *qmp_dispatcher_bh;
 
-/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
+/*
+ * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
+ * monitor_destroyed.
+ */
 QemuMutex monitor_lock;
 static GHashTable *monitor_qapi_event_state;
+static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
 
 MonitorList mon_list;
 int mon_refcount;
 static bool monitor_destroyed;
 
-static __thread Monitor *cur_monitor;
-
 Monitor *monitor_cur(void)
 {
-    return cur_monitor;
+    Monitor *mon;
+
+    qemu_mutex_lock(&monitor_lock);
+    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
+    qemu_mutex_unlock(&monitor_lock);
+
+    return mon;
 }
 
-void monitor_set_cur(Monitor *mon)
+void monitor_set_cur(Coroutine *co, Monitor *mon)
 {
-    cur_monitor = mon;
+    qemu_mutex_lock(&monitor_lock);
+    if (mon) {
+        g_hash_table_replace(coroutine_mon, co, mon);
+    } else {
+        g_hash_table_remove(coroutine_mon, co);
+    }
+    qemu_mutex_unlock(&monitor_lock);
 }
 
 /**
@@ -613,6 +627,7 @@ void monitor_init_globals_core(void)
 {
     monitor_qapi_event_init();
     qemu_mutex_init(&monitor_lock);
+    coroutine_mon = g_hash_table_new(NULL, NULL);
 
     /*
      * The dispatcher BH must run in the main loop thread, since we
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 2fdbc0fba4..5677ba92ca 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -154,11 +154,11 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
     }
 
     assert(monitor_cur() == NULL);
-    monitor_set_cur(cur_mon);
+    monitor_set_cur(qemu_coroutine_self(), cur_mon);
 
     cmd->fn(args, &ret, &err);
 
-    monitor_set_cur(NULL);
+    monitor_set_cur(qemu_coroutine_self(), NULL);
     qobject_unref(args);
     if (err) {
         /* or assert(!ret) after reviewing all handlers: */
diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index e493df1027..635e37a6ba 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -8,7 +8,7 @@ Monitor *monitor_cur(void)
     return NULL;
 }
 
-void monitor_set_cur(Monitor *mon)
+void monitor_set_cur(Coroutine *co, Monitor *mon)
 {
 }
 
-- 
2.25.4

Re: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Eric Blake]

On 5/28/20 10:37 AM, Kevin Wolf wrote:
> This way, a monitor command handler will still be able to access the
> current monitor, but when it yields, all other code code will correctly
> get NULL from monitor_cur().
> 
> Outside of coroutine context, qemu_coroutine_self() returns the leader
> coroutine of the current thread.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This way, a monitor command handler will still be able to access the
> current monitor, but when it yields, all other code code will correctly
> get NULL from monitor_cur().
>
> Outside of coroutine context, qemu_coroutine_self() returns the leader
> coroutine of the current thread.

Unsaid: you use it as a hash table key to map from coroutine to monitor,
and for that you need it to return a value unique to the coroutine in
coroutine context, and a value unique to the thread outside coroutine
context.  Which qemu_coroutine_self() does.  Correct?

The hash table works, but I hate it just as much as I hate
pthread_getspecific() / pthread_setspecific().

What we have here is a need for coroutine-local data.  Feels like a
perfectly natural concept to me.

Are we going to create another hash table whenever we need another piece
of coroutine-local data?  Or shall we reuse the hash table, suitably
renamed and moved to another file?

Why not simply associate an opaque pointer with each coroutine?  All it
takes is one more member of struct Coroutine.  Whatever creates the
coroutine decides what to use it for.  The monitor coroutine would use
it to point to the monitor.

At least, discuss the design alternatives in the commit message.

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/monitor/monitor.h |  2 +-
>  monitor/hmp.c             |  4 ++--
>  monitor/monitor.c         | 27 +++++++++++++++++++++------
>  qapi/qmp-dispatch.c       |  4 ++--
>  stubs/monitor-core.c      |  2 +-
>  5 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 43cc746078..16072e325c 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions;
>  extern QemuOptsList qemu_mon_opts;
>  
>  Monitor *monitor_cur(void);
> -void monitor_set_cur(Monitor *mon);
> +void monitor_set_cur(Coroutine *co, Monitor *mon);
>  bool monitor_cur_is_qmp(void);
>  
>  void monitor_init_globals(void);
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 79be6f26de..3e73a4c3ce 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1082,9 +1082,9 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>  
>      /* old_mon is non-NULL when called from qmp_human_monitor_command() */
>      old_mon = monitor_cur();
> -    monitor_set_cur(&mon->common);
> +    monitor_set_cur(qemu_coroutine_self(), &mon->common);
>      cmd->cmd(&mon->common, qdict);
> -    monitor_set_cur(old_mon);
> +    monitor_set_cur(qemu_coroutine_self(), old_mon);
>  
>      qobject_unref(qdict);
>  }
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 182ba136b4..35003bb486 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -58,24 +58,38 @@ IOThread *mon_iothread;
>  /* Bottom half to dispatch the requests received from I/O thread */
>  QEMUBH *qmp_dispatcher_bh;
>  
> -/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
> +/*
> + * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> + * monitor_destroyed.
> + */
>  QemuMutex monitor_lock;
>  static GHashTable *monitor_qapi_event_state;
> +static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
>  
>  MonitorList mon_list;
>  int mon_refcount;
>  static bool monitor_destroyed;
>  
> -static __thread Monitor *cur_monitor;
> -
>  Monitor *monitor_cur(void)
>  {
> -    return cur_monitor;
> +    Monitor *mon;
> +
> +    qemu_mutex_lock(&monitor_lock);
> +    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
> +    qemu_mutex_unlock(&monitor_lock);
> +
> +    return mon;
>  }
>  
> -void monitor_set_cur(Monitor *mon)
> +void monitor_set_cur(Coroutine *co, Monitor *mon)
>  {
> -    cur_monitor = mon;
> +    qemu_mutex_lock(&monitor_lock);
> +    if (mon) {
> +        g_hash_table_replace(coroutine_mon, co, mon);
> +    } else {
> +        g_hash_table_remove(coroutine_mon, co);
> +    }
> +    qemu_mutex_unlock(&monitor_lock);
>  }

You really need a contract now: any call to monitor_set_cur() with a
non-null @mon must be followed by a call with a null @mon.

>  
>  /**
> @@ -613,6 +627,7 @@ void monitor_init_globals_core(void)
>  {
>      monitor_qapi_event_init();
>      qemu_mutex_init(&monitor_lock);
> +    coroutine_mon = g_hash_table_new(NULL, NULL);
>  
>      /*
>       * The dispatcher BH must run in the main loop thread, since we
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 2fdbc0fba4..5677ba92ca 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -154,11 +154,11 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
>      }
>  
>      assert(monitor_cur() == NULL);
> -    monitor_set_cur(cur_mon);
> +    monitor_set_cur(qemu_coroutine_self(), cur_mon);
>  
>      cmd->fn(args, &ret, &err);
>  
> -    monitor_set_cur(NULL);
> +    monitor_set_cur(qemu_coroutine_self(), NULL);
>      qobject_unref(args);
>      if (err) {
>          /* or assert(!ret) after reviewing all handlers: */
> diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> index e493df1027..635e37a6ba 100644
> --- a/stubs/monitor-core.c
> +++ b/stubs/monitor-core.c
> @@ -8,7 +8,7 @@ Monitor *monitor_cur(void)
>      return NULL;
>  }
>  
> -void monitor_set_cur(Monitor *mon)
> +void monitor_set_cur(Coroutine *co, Monitor *mon)
>  {
>  }

Re: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Kevin Wolf]

Am 04.08.2020 um 15:50 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > This way, a monitor command handler will still be able to access the
> > current monitor, but when it yields, all other code code will correctly
> > get NULL from monitor_cur().
> >
> > Outside of coroutine context, qemu_coroutine_self() returns the leader
> > coroutine of the current thread.
> 
> Unsaid: you use it as a hash table key to map from coroutine to monitor,
> and for that you need it to return a value unique to the coroutine in
> coroutine context, and a value unique to the thread outside coroutine
> context.  Which qemu_coroutine_self() does.  Correct?

Correct.

> The hash table works, but I hate it just as much as I hate
> pthread_getspecific() / pthread_setspecific().
> 
> What we have here is a need for coroutine-local data.  Feels like a
> perfectly natural concept to me.

If you have a good concept how to implement this in a generic way that
doesn't impact the I/O fast path, feel free to implement it and I'll
happily use it.

But the hash table is simple and works for this use case, so I see
little reason to invest a lot of time in something that we haven't ever
had another user for.

> Are we going to create another hash table whenever we need another piece
> of coroutine-local data?  Or shall we reuse the hash table, suitably
> renamed and moved to another file?

I think I would vote for separate hash tables rather than having a hash
table containing a struct that mixes values from all subsystems, but
this can be discussed when (if) the need arises.

> Why not simply associate an opaque pointer with each coroutine?  All it
> takes is one more member of struct Coroutine.  Whatever creates the
> coroutine decides what to use it for.  The monitor coroutine would use
> it to point to the monitor.

This doesn't work. error_report() is called from all kinds of
coroutines, not just from coroutines created from the monitor, and it
wants to know the current monitor.

> At least, discuss the design alternatives in the commit message.

*sigh* Fine. Tell me which set of alternatives to discuss.

> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/monitor/monitor.h |  2 +-
> >  monitor/hmp.c             |  4 ++--
> >  monitor/monitor.c         | 27 +++++++++++++++++++++------
> >  qapi/qmp-dispatch.c       |  4 ++--
> >  stubs/monitor-core.c      |  2 +-
> >  5 files changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 43cc746078..16072e325c 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions;
> >  extern QemuOptsList qemu_mon_opts;
> >  
> >  Monitor *monitor_cur(void);
> > -void monitor_set_cur(Monitor *mon);
> > +void monitor_set_cur(Coroutine *co, Monitor *mon);
> >  bool monitor_cur_is_qmp(void);
> >  
> >  void monitor_init_globals(void);
> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> > index 79be6f26de..3e73a4c3ce 100644
> > --- a/monitor/hmp.c
> > +++ b/monitor/hmp.c
> > @@ -1082,9 +1082,9 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
> >  
> >      /* old_mon is non-NULL when called from qmp_human_monitor_command() */
> >      old_mon = monitor_cur();
> > -    monitor_set_cur(&mon->common);
> > +    monitor_set_cur(qemu_coroutine_self(), &mon->common);
> >      cmd->cmd(&mon->common, qdict);
> > -    monitor_set_cur(old_mon);
> > +    monitor_set_cur(qemu_coroutine_self(), old_mon);
> >  
> >      qobject_unref(qdict);
> >  }
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 182ba136b4..35003bb486 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -58,24 +58,38 @@ IOThread *mon_iothread;
> >  /* Bottom half to dispatch the requests received from I/O thread */
> >  QEMUBH *qmp_dispatcher_bh;
> >  
> > -/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
> > +/*
> > + * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> > + * monitor_destroyed.
> > + */
> >  QemuMutex monitor_lock;
> >  static GHashTable *monitor_qapi_event_state;
> > +static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
> >  
> >  MonitorList mon_list;
> >  int mon_refcount;
> >  static bool monitor_destroyed;
> >  
> > -static __thread Monitor *cur_monitor;
> > -
> >  Monitor *monitor_cur(void)
> >  {
> > -    return cur_monitor;
> > +    Monitor *mon;
> > +
> > +    qemu_mutex_lock(&monitor_lock);
> > +    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
> > +    qemu_mutex_unlock(&monitor_lock);
> > +
> > +    return mon;
> >  }
> >  
> > -void monitor_set_cur(Monitor *mon)
> > +void monitor_set_cur(Coroutine *co, Monitor *mon)
> >  {
> > -    cur_monitor = mon;
> > +    qemu_mutex_lock(&monitor_lock);
> > +    if (mon) {
> > +        g_hash_table_replace(coroutine_mon, co, mon);
> > +    } else {
> > +        g_hash_table_remove(coroutine_mon, co);
> > +    }
> > +    qemu_mutex_unlock(&monitor_lock);
> >  }
> 
> You really need a contract now: any call to monitor_set_cur() with a
> non-null @mon must be followed by a call with a null @mon.

Why? g_hash_table_replace() removes the old value and replaces it with
the new one.

Kevin

Re: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 04.08.2020 um 15:50 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > This way, a monitor command handler will still be able to access the
>> > current monitor, but when it yields, all other code code will correctly
>> > get NULL from monitor_cur().
>> >
>> > Outside of coroutine context, qemu_coroutine_self() returns the leader
>> > coroutine of the current thread.
>> 
>> Unsaid: you use it as a hash table key to map from coroutine to monitor,
>> and for that you need it to return a value unique to the coroutine in
>> coroutine context, and a value unique to the thread outside coroutine
>> context.  Which qemu_coroutine_self() does.  Correct?
>
> Correct.
>
>> The hash table works, but I hate it just as much as I hate
>> pthread_getspecific() / pthread_setspecific().
>> 
>> What we have here is a need for coroutine-local data.  Feels like a
>> perfectly natural concept to me.
>
> If you have a good concept how to implement this in a generic way that
> doesn't impact the I/O fast path, feel free to implement it and I'll
> happily use it.

Fair enough; I'll give it a shot.

> But the hash table is simple and works for this use case, so I see
> little reason to invest a lot of time in something that we haven't ever
> had another user for.
>
>> Are we going to create another hash table whenever we need another piece
>> of coroutine-local data?  Or shall we reuse the hash table, suitably
>> renamed and moved to another file?
>
> I think I would vote for separate hash tables rather than having a hash
> table containing a struct that mixes values from all subsystems, but
> this can be discussed when (if) the need arises.
>
>> Why not simply associate an opaque pointer with each coroutine?  All it
>> takes is one more member of struct Coroutine.  Whatever creates the
>> coroutine decides what to use it for.  The monitor coroutine would use
>> it to point to the monitor.
>
> This doesn't work. error_report() is called from all kinds of
> coroutines, not just from coroutines created from the monitor, and it
> wants to know the current monitor.

Yup, monitor_cur() and monitor_set_cur() need to work both in coroutine
context and outside coroutine context.

>> At least, discuss the design alternatives in the commit message.
>
> *sigh* Fine. Tell me which set of alternatives to discuss.

Let me first play with the alternative I suggested.

>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >  include/monitor/monitor.h |  2 +-
>> >  monitor/hmp.c             |  4 ++--
>> >  monitor/monitor.c         | 27 +++++++++++++++++++++------
>> >  qapi/qmp-dispatch.c       |  4 ++--
>> >  stubs/monitor-core.c      |  2 +-
>> >  5 files changed, 27 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> > index 43cc746078..16072e325c 100644
>> > --- a/include/monitor/monitor.h
>> > +++ b/include/monitor/monitor.h
>> > @@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions;
>> >  extern QemuOptsList qemu_mon_opts;
>> >  
>> >  Monitor *monitor_cur(void);
>> > -void monitor_set_cur(Monitor *mon);
>> > +void monitor_set_cur(Coroutine *co, Monitor *mon);
>> >  bool monitor_cur_is_qmp(void);
>> >  
>> >  void monitor_init_globals(void);
>> > diff --git a/monitor/hmp.c b/monitor/hmp.c
>> > index 79be6f26de..3e73a4c3ce 100644
>> > --- a/monitor/hmp.c
>> > +++ b/monitor/hmp.c
>> > @@ -1082,9 +1082,9 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>> >  
>> >      /* old_mon is non-NULL when called from qmp_human_monitor_command() */
>> >      old_mon = monitor_cur();
>> > -    monitor_set_cur(&mon->common);
>> > +    monitor_set_cur(qemu_coroutine_self(), &mon->common);
>> >      cmd->cmd(&mon->common, qdict);
>> > -    monitor_set_cur(old_mon);
>> > +    monitor_set_cur(qemu_coroutine_self(), old_mon);
>> >  
>> >      qobject_unref(qdict);
>> >  }
>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index 182ba136b4..35003bb486 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -58,24 +58,38 @@ IOThread *mon_iothread;
>> >  /* Bottom half to dispatch the requests received from I/O thread */
>> >  QEMUBH *qmp_dispatcher_bh;
>> >  
>> > -/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
>> > +/*
>> > + * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
>> > + * monitor_destroyed.
>> > + */
>> >  QemuMutex monitor_lock;
>> >  static GHashTable *monitor_qapi_event_state;
>> > +static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
>> >  
>> >  MonitorList mon_list;
>> >  int mon_refcount;
>> >  static bool monitor_destroyed;
>> >  
>> > -static __thread Monitor *cur_monitor;
>> > -
>> >  Monitor *monitor_cur(void)
>> >  {
>> > -    return cur_monitor;
>> > +    Monitor *mon;
>> > +
>> > +    qemu_mutex_lock(&monitor_lock);
>> > +    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
>> > +    qemu_mutex_unlock(&monitor_lock);
>> > +
>> > +    return mon;
>> >  }
>> >  
>> > -void monitor_set_cur(Monitor *mon)
>> > +void monitor_set_cur(Coroutine *co, Monitor *mon)
>> >  {
>> > -    cur_monitor = mon;
>> > +    qemu_mutex_lock(&monitor_lock);
>> > +    if (mon) {
>> > +        g_hash_table_replace(coroutine_mon, co, mon);
>> > +    } else {
>> > +        g_hash_table_remove(coroutine_mon, co);
>> > +    }
>> > +    qemu_mutex_unlock(&monitor_lock);
>> >  }
>> 
>> You really need a contract now: any call to monitor_set_cur() with a
>> non-null @mon must be followed by a call with a null @mon.
>
> Why? g_hash_table_replace() removes the old value and replaces it with
> the new one.

If you monitor_set_cur(NULL) is forgotten or bypassed somehow, the hash
table entry stays even when the coroutine dies.  Minor memory leak.  If
another coroutine gets created at the same address, it "inherits" the
current monitor.  Not good.  If the monitor has died meanwhile, dangling
pointer.  Fortunately, monitors die only during shutdown, except for the
dummy in qmp_human_monitor_command().

Re: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Kevin Wolf]

Am 05.08.2020 um 09:28 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 04.08.2020 um 15:50 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > This way, a monitor command handler will still be able to access the
> >> > current monitor, but when it yields, all other code code will correctly
> >> > get NULL from monitor_cur().
> >> >
> >> > Outside of coroutine context, qemu_coroutine_self() returns the leader
> >> > coroutine of the current thread.
> >> 
> >> Unsaid: you use it as a hash table key to map from coroutine to monitor,
> >> and for that you need it to return a value unique to the coroutine in
> >> coroutine context, and a value unique to the thread outside coroutine
> >> context.  Which qemu_coroutine_self() does.  Correct?
> >
> > Correct.
> >
> >> The hash table works, but I hate it just as much as I hate
> >> pthread_getspecific() / pthread_setspecific().
> >> 
> >> What we have here is a need for coroutine-local data.  Feels like a
> >> perfectly natural concept to me.
> >
> > If you have a good concept how to implement this in a generic way that
> > doesn't impact the I/O fast path, feel free to implement it and I'll
> > happily use it.
> 
> Fair enough; I'll give it a shot.
> 
> > But the hash table is simple and works for this use case, so I see
> > little reason to invest a lot of time in something that we haven't ever
> > had another user for.
> >
> >> Are we going to create another hash table whenever we need another piece
> >> of coroutine-local data?  Or shall we reuse the hash table, suitably
> >> renamed and moved to another file?
> >
> > I think I would vote for separate hash tables rather than having a hash
> > table containing a struct that mixes values from all subsystems, but
> > this can be discussed when (if) the need arises.
> >
> >> Why not simply associate an opaque pointer with each coroutine?  All it
> >> takes is one more member of struct Coroutine.  Whatever creates the
> >> coroutine decides what to use it for.  The monitor coroutine would use
> >> it to point to the monitor.
> >
> > This doesn't work. error_report() is called from all kinds of
> > coroutines, not just from coroutines created from the monitor, and it
> > wants to know the current monitor.
> 
> Yup, monitor_cur() and monitor_set_cur() need to work both in coroutine
> context and outside coroutine context.

And in coroutine contexts, but in coroutine created by someone else than
the monitor.

> >> At least, discuss the design alternatives in the commit message.
> >
> > *sigh* Fine. Tell me which set of alternatives to discuss.
> 
> Let me first play with the alternative I suggested.
> 
> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> > ---
> >> >  include/monitor/monitor.h |  2 +-
> >> >  monitor/hmp.c             |  4 ++--
> >> >  monitor/monitor.c         | 27 +++++++++++++++++++++------
> >> >  qapi/qmp-dispatch.c       |  4 ++--
> >> >  stubs/monitor-core.c      |  2 +-
> >> >  5 files changed, 27 insertions(+), 12 deletions(-)
> >> >
> >> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> >> > index 43cc746078..16072e325c 100644
> >> > --- a/include/monitor/monitor.h
> >> > +++ b/include/monitor/monitor.h
> >> > @@ -13,7 +13,7 @@ typedef struct MonitorOptions MonitorOptions;
> >> >  extern QemuOptsList qemu_mon_opts;
> >> >  
> >> >  Monitor *monitor_cur(void);
> >> > -void monitor_set_cur(Monitor *mon);
> >> > +void monitor_set_cur(Coroutine *co, Monitor *mon);
> >> >  bool monitor_cur_is_qmp(void);
> >> >  
> >> >  void monitor_init_globals(void);
> >> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> >> > index 79be6f26de..3e73a4c3ce 100644
> >> > --- a/monitor/hmp.c
> >> > +++ b/monitor/hmp.c
> >> > @@ -1082,9 +1082,9 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
> >> >  
> >> >      /* old_mon is non-NULL when called from qmp_human_monitor_command() */
> >> >      old_mon = monitor_cur();
> >> > -    monitor_set_cur(&mon->common);
> >> > +    monitor_set_cur(qemu_coroutine_self(), &mon->common);
> >> >      cmd->cmd(&mon->common, qdict);
> >> > -    monitor_set_cur(old_mon);
> >> > +    monitor_set_cur(qemu_coroutine_self(), old_mon);
> >> >  
> >> >      qobject_unref(qdict);
> >> >  }
> >> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> >> > index 182ba136b4..35003bb486 100644
> >> > --- a/monitor/monitor.c
> >> > +++ b/monitor/monitor.c
> >> > @@ -58,24 +58,38 @@ IOThread *mon_iothread;
> >> >  /* Bottom half to dispatch the requests received from I/O thread */
> >> >  QEMUBH *qmp_dispatcher_bh;
> >> >  
> >> > -/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
> >> > +/*
> >> > + * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> >> > + * monitor_destroyed.
> >> > + */
> >> >  QemuMutex monitor_lock;
> >> >  static GHashTable *monitor_qapi_event_state;
> >> > +static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
> >> >  
> >> >  MonitorList mon_list;
> >> >  int mon_refcount;
> >> >  static bool monitor_destroyed;
> >> >  
> >> > -static __thread Monitor *cur_monitor;
> >> > -
> >> >  Monitor *monitor_cur(void)
> >> >  {
> >> > -    return cur_monitor;
> >> > +    Monitor *mon;
> >> > +
> >> > +    qemu_mutex_lock(&monitor_lock);
> >> > +    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
> >> > +    qemu_mutex_unlock(&monitor_lock);
> >> > +
> >> > +    return mon;
> >> >  }
> >> >  
> >> > -void monitor_set_cur(Monitor *mon)
> >> > +void monitor_set_cur(Coroutine *co, Monitor *mon)
> >> >  {
> >> > -    cur_monitor = mon;
> >> > +    qemu_mutex_lock(&monitor_lock);
> >> > +    if (mon) {
> >> > +        g_hash_table_replace(coroutine_mon, co, mon);
> >> > +    } else {
> >> > +        g_hash_table_remove(coroutine_mon, co);
> >> > +    }
> >> > +    qemu_mutex_unlock(&monitor_lock);
> >> >  }
> >> 
> >> You really need a contract now: any call to monitor_set_cur() with a
> >> non-null @mon must be followed by a call with a null @mon.
> >
> > Why? g_hash_table_replace() removes the old value and replaces it with
> > the new one.
> 
> If you monitor_set_cur(NULL) is forgotten or bypassed somehow, the hash
> table entry stays even when the coroutine dies.  Minor memory leak.  If
> another coroutine gets created at the same address, it "inherits" the
> current monitor.  Not good.  If the monitor has died meanwhile, dangling
> pointer.  Fortunately, monitors die only during shutdown, except for the
> dummy in qmp_human_monitor_command().

Ah, yes, fair. I can document this.

In practice not a problem because the QMP dispatcher coroutine and HMP
command handler coroutines are the only places that set (and reset) it.

In fact, HMP needs to be fixed to reset to NULL before the coroutine
terminates.

Kevin

Ways to do per-coroutine properties (was: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property)

[Markus Armbruster]

I called for a discussion of design alternatives, because I dislike the
one I got.  Here we go.

= Context: the "current monitor" =

Output of HMP commands needs to go to the HMP monitor executing the
command.  Trivial in HMP command handlers: the handler function takes a
monitor argument.  Not so trivial in code used both by HMP command
handlers and other users, such as CLI.  In particular, passing the
monitor through multiple layers that don't want to know anything about
monitors to the point that reports an error just so we can make the
error report go where it needs to go would be impractical.  We made
error_report() & friends do the right thing without such help.

To let them do that, we maintain a "current monitor".

    Invariant: while executing a monitor command, thread-local variable
    @cur_mon points to the monitor executing the command.  When the
    thread is not executing a monitor command, @cur_mon is null.

Now error_report() can do the right thing easily: print to @cur_mon if
non-null, else to stderr.

We also use @cur_mon for getting file descriptors stored in the monitor.
Could perhaps do without @cur_mon, but since it's there anyway...

= Problem at hand: "current monitor" for coroutine-enabled commands =

We want to be able to run monitor commands in a coroutine, so they can
yield instead of blocking the main loop.

Simply yielding in a monitor command violates the invariant: we're no
longer executing a monitor command[*], but @cur_mon is still non-null.

This is because the current monitor is no longer a property of the
thread, but a property of the coroutine.  Thread-local variable @cur_mon
doesn't fit the bill anymore.

= Solution 1: A separate map coroutine -> current monitor =

Kevin implemented this, using a hash table.

PRO:

* Stays off the coroutine switch hot path (by staying off coroutine code
  entirely).

CON

* It's a one-off (but at least it's confined to monitor.c)

* It's slow, and uses locks (but that's probably okay for this use; see
  also one-off).

* We get to worry about consistency between coroutines and the hash
  table.

While this looks servicable, I wonder whether we can we come up with
something a bit more elegant.

= Solution 2: Put the map into struct Coroutine =

The hash table can be replaced by putting a @cur_mon member right into
struct Coroutine, together with a setter and a getter function.

PRO

* Stays off the coroutine switch hot path.

CON

* It's a one off.

* HMP bleeds into the coroutine subsystem, which really doesn't want to
  know anything about monitors.

Thanks, but no thanks.

= Solution 3: Put abstract maps into struct Coroutine =

Daniel's proposal: instead of putting a Monitor * member into struct
Coroutine, put an array of void * there, indexed by well-known data
keys.  Initially, there is just one data key, for the current monitor.

This is basically pthread_setspecific(), pthread_getspecific() for
coroutines, with pthread_key_create() dumbed down to a static set of
well-known keys.

PRO

* Stays off the coroutine switch hot path.

* Similar to how thread-local storage works with traditional pthreads.

CON

* Similar to how thread-local storage works with traditional pthreads.

= Solution 4: Fixed coroutine-local storage =

Whereas solution 3 is like traditional pthreads, this solution works
more like __thread does under the hood: we allocate memory for
coroutine-local storage on coroutine creation, maintain a global pointer
on thread switch, and free the memory on destruction.

We can keep the global pointer in struct Coroutine, and have a getter
return it.

If accessing coroutine-local storage ever becomes a performance
bottleneck, we can either open-code the getter, or store the pointer in
thread-local storage (but then we need to update it in the coroutine
switch hot path).  No need to worry about all that now.

Since we don't have compiler and linker support, we have to collect the
coroutine-local variables in a struct manually.

PRO

* Stays off the coroutine switch hot path.

* Access could be made quite fast if need be.

CON

* The struct of coroutine-local variable crosses subsystem boundaries.

= Solution 5: Optional coroutine-specific storage =

When creating a coroutine, you can optionally ask for a certain amount
of coroutine-specific memory.  It's malloced, stored in struct
Coroutine, and freed when on deletion.

A getter returns the coroutine-specific memory.  To actually use it, you
have to know the coroutine's coroutine-specific memory layout.

PRO

* Stays off the coroutine switch hot path.

* Access could be made quite fast if need be.

CON

* Having to know the coroutine's coroutine-specifc memory layout could
  turn out to be impractical for some applications of "property of a
  coroutine".

This is the solution I had in mind from the start.  I have prototype
code that passes basic testing.

= Solution 6: Exploit there is just two coroutines involved =

A simpler solution is possible, but to understand it, you first have to
understand how the threads and coroutines work together.  Let me
recapitulate.

In old QEMU, all monitors run in the main thread's main loop, and
together execute one command after the other.  @cur_mon was a global
variable, to be accessed only by the main thread.

Commit 62aa1d887f "monitor: Fix unsafe sharing of @cur_mon among
threads" (v3.0.0) made @cur_mon thread-local.  "Fix" was a bit of an
overstatement; no unsafe access was known.

The OOB work moved a part of the QMP monitor work from the main loop
into @mon_iothread.  @mon_iothread sends commands to the main thread for
execution, except for commands executed "out-of-band".

This series moves the main thread's QMP command dispatch into coroutine
@qmp_dispatcher_co.  Commands that aren't coroutine-capable get
dispatched to a one-shot bottom half, also in the main thread.

The series modifies the main thread's HMP command dispatch to wrap
execution of each coroutine-capable command in a newly created
coroutine.

We have:

* OOB commands running in @mon_iothread, outside coroutine context

* Coroutine-incapable QMP commands running in the main thread, outside
  coroutine context (detail: in a bottom half)

* Coroutine-incapable HMP commands running in the main thread, outside
  coroutine-incapable context

* Coroutine-capable QMP commands running in the main thread, in
  coroutine @qmp_dispatcher_co

* Coroutine-capable HMP commands runnning in the main thread, in a
  coroutine created just for the command

* At most one non-OOB command is executing at any time.

Let's ignore HMP for now.  Observe:

* As long as there is just one @qmp_dispatcher_co, there is just one
  current monitor for coroutine-capable QMP commands at any time.  It
  can therefore be stored in a simple global variable
  @qmp_dispatcher_co_mon.

* For the coroutine-incapable commands, thread-local variable @cur_mon
  suffices.

* If qemu_coroutine_self() == qmp_dispatcher_co, the current monitor is
  @qmp_dispatcher_co_mon.  Else it's @cur_mon.

To extend this to HMP, we have to make the handle_hmp_command()'s local
variable @co a global one.

PRO:

* Stays off the coroutine switch hot path (by staying off coroutine code
  entirely).

* Simple code.

CON

* It's a one-off (but at least it's confined to monitor.c).

* The argument behind the code is less than simple (see above).

* Should our monitor coroutines multiply, say because we pull off
  executing (some) in-band commands in monitor I/O thread(s), the
  solution falls apart.

I have prototype code that passes basic testing.

Opinions?

I'll post my two prototypes shortly.


[*] In theory, we could yield to a coroutine that is executing another
monitor's monitor command.  In practice, we haven't implemented that.

[PATCH] Simple & stupid coroutine-aware monitor_cur()

[Markus Armbruster]

This is just a sketch.  It's incomplete, needs comments and a real
commit message.

Support for "[PATCH v6 09/12] hmp: Add support for coroutine command
handlers" is missing.  Marked FIXME.

As is, it goes on top of Kevin's series.  It is meant to be squashed
into PATCH 06, except for the FIXME, which needs to be resolved in PATCH
09 instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/monitor.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 50fb5b20d3..8601340285 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -82,38 +82,34 @@ bool qmp_dispatcher_co_shutdown;
  */
 bool qmp_dispatcher_co_busy;
 
-/*
- * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
- * monitor_destroyed.
- */
+/* Protects mon_list, monitor_qapi_event_state, * monitor_destroyed. */
 QemuMutex monitor_lock;
 static GHashTable *monitor_qapi_event_state;
-static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
 
 MonitorList mon_list;
 int mon_refcount;
 static bool monitor_destroyed;
 
+static Monitor **monitor_curp(Coroutine *co)
+{
+    static __thread Monitor *thread_local_mon;
+    static Monitor *qmp_dispatcher_co_mon;
+
+    if (qemu_coroutine_self() == qmp_dispatcher_co) {
+        return &qmp_dispatcher_co_mon;
+    }
+    /* FIXME the coroutine hidden in handle_hmp_command() */
+    return &thread_local_mon;
+}
+
 Monitor *monitor_cur(void)
 {
-    Monitor *mon;
-
-    qemu_mutex_lock(&monitor_lock);
-    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
-    qemu_mutex_unlock(&monitor_lock);
-
-    return mon;
+    return *monitor_curp(qemu_coroutine_self());
 }
 
 void monitor_set_cur(Coroutine *co, Monitor *mon)
 {
-    qemu_mutex_lock(&monitor_lock);
-    if (mon) {
-        g_hash_table_replace(coroutine_mon, co, mon);
-    } else {
-        g_hash_table_remove(coroutine_mon, co);
-    }
-    qemu_mutex_unlock(&monitor_lock);
+    *monitor_curp(co) = mon;
 }
 
 /**
@@ -666,7 +662,6 @@ void monitor_init_globals_core(void)
 {
     monitor_qapi_event_init();
     qemu_mutex_init(&monitor_lock);
-    coroutine_mon = g_hash_table_new(NULL, NULL);
 
     /*
      * The dispatcher BH must run in the main loop thread, since we
-- 
2.26.2

Re: [PATCH] Simple & stupid coroutine-aware monitor_cur()

[Kevin Wolf]

Am 07.08.2020 um 15:27 hat Markus Armbruster geschrieben:
> This is just a sketch.  It's incomplete, needs comments and a real
> commit message.
> 
> Support for "[PATCH v6 09/12] hmp: Add support for coroutine command
> handlers" is missing.  Marked FIXME.
> 
> As is, it goes on top of Kevin's series.  It is meant to be squashed
> into PATCH 06, except for the FIXME, which needs to be resolved in PATCH
> 09 instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor/monitor.c | 35 +++++++++++++++--------------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 50fb5b20d3..8601340285 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -82,38 +82,34 @@ bool qmp_dispatcher_co_shutdown;
>   */
>  bool qmp_dispatcher_co_busy;
>  
> -/*
> - * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> - * monitor_destroyed.
> - */
> +/* Protects mon_list, monitor_qapi_event_state, * monitor_destroyed. */
>  QemuMutex monitor_lock;
>  static GHashTable *monitor_qapi_event_state;
> -static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
>  
>  MonitorList mon_list;
>  int mon_refcount;
>  static bool monitor_destroyed;
>  
> +static Monitor **monitor_curp(Coroutine *co)
> +{
> +    static __thread Monitor *thread_local_mon;
> +    static Monitor *qmp_dispatcher_co_mon;
> +
> +    if (qemu_coroutine_self() == qmp_dispatcher_co) {
> +        return &qmp_dispatcher_co_mon;
> +    }
> +    /* FIXME the coroutine hidden in handle_hmp_command() */
> +    return &thread_local_mon;
> +}

Is thread_local_mon supposed to ever be set? The only callers of
monitor_set_cur() are the HMP and QMP dispatchers, which will return
something different.

So should we return NULL insetad of thread_local_mon...

>  Monitor *monitor_cur(void)
>  {
> -    Monitor *mon;
> -
> -    qemu_mutex_lock(&monitor_lock);
> -    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
> -    qemu_mutex_unlock(&monitor_lock);
> -
> -    return mon;
> +    return *monitor_curp(qemu_coroutine_self());
>  }

...and return NULL here if monitor_curp() returned NULL...

>  void monitor_set_cur(Coroutine *co, Monitor *mon)
>  {
> -    qemu_mutex_lock(&monitor_lock);
> -    if (mon) {
> -        g_hash_table_replace(coroutine_mon, co, mon);
> -    } else {
> -        g_hash_table_remove(coroutine_mon, co);
> -    }
> -    qemu_mutex_unlock(&monitor_lock);
> +    *monitor_curp(co) = mon;

...and assert(monitor_curp(co) != NULL) here?

This approach looks workable, though the implementation of
monitor_curp() feels a bit brittle. The code is not significantly
simpler than the hash table based approach, but the assumptions it makes
are a bit more hidden.

Saving the locks is more a theoretical improvement because all callers
are slows paths anyway.

Kevin

Re: [PATCH] Simple & stupid coroutine-aware monitor_cur()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 07.08.2020 um 15:27 hat Markus Armbruster geschrieben:
>> This is just a sketch.  It's incomplete, needs comments and a real
>> commit message.
>> 
>> Support for "[PATCH v6 09/12] hmp: Add support for coroutine command
>> handlers" is missing.  Marked FIXME.
>> 
>> As is, it goes on top of Kevin's series.  It is meant to be squashed
>> into PATCH 06, except for the FIXME, which needs to be resolved in PATCH
>> 09 instead.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor/monitor.c | 35 +++++++++++++++--------------------
>>  1 file changed, 15 insertions(+), 20 deletions(-)
>> 
>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>> index 50fb5b20d3..8601340285 100644
>> --- a/monitor/monitor.c
>> +++ b/monitor/monitor.c
>> @@ -82,38 +82,34 @@ bool qmp_dispatcher_co_shutdown;
>>   */
>>  bool qmp_dispatcher_co_busy;
>>  
>> -/*
>> - * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
>> - * monitor_destroyed.
>> - */
>> +/* Protects mon_list, monitor_qapi_event_state, * monitor_destroyed. */
>>  QemuMutex monitor_lock;
>>  static GHashTable *monitor_qapi_event_state;
>> -static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
>>  
>>  MonitorList mon_list;
>>  int mon_refcount;
>>  static bool monitor_destroyed;
>>  
>> +static Monitor **monitor_curp(Coroutine *co)
>> +{
>> +    static __thread Monitor *thread_local_mon;
>> +    static Monitor *qmp_dispatcher_co_mon;
>> +
>> +    if (qemu_coroutine_self() == qmp_dispatcher_co) {
>> +        return &qmp_dispatcher_co_mon;
>> +    }
>> +    /* FIXME the coroutine hidden in handle_hmp_command() */
>> +    return &thread_local_mon;
>> +}
>
> Is thread_local_mon supposed to ever be set? The only callers of
> monitor_set_cur() are the HMP and QMP dispatchers, which will return
> something different.

OOB commands are executed in @mon_iothread, outside coroutine context.
qmp_dispatch() calls monitor_set_cur(), which sets thread_local_mon
then.

Since there is just one @mon_iothread, a @global_mon without __thread
would do, but I don't see a need to exploit that here.

> So should we return NULL insetad of thread_local_mon...
>
>>  Monitor *monitor_cur(void)
>>  {
>> -    Monitor *mon;
>> -
>> -    qemu_mutex_lock(&monitor_lock);
>> -    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
>> -    qemu_mutex_unlock(&monitor_lock);
>> -
>> -    return mon;
>> +    return *monitor_curp(qemu_coroutine_self());
>>  }
>
> ...and return NULL here if monitor_curp() returned NULL...
>
>>  void monitor_set_cur(Coroutine *co, Monitor *mon)
>>  {
>> -    qemu_mutex_lock(&monitor_lock);
>> -    if (mon) {
>> -        g_hash_table_replace(coroutine_mon, co, mon);
>> -    } else {
>> -        g_hash_table_remove(coroutine_mon, co);
>> -    }
>> -    qemu_mutex_unlock(&monitor_lock);
>> +    *monitor_curp(co) = mon;
>
> ...and assert(monitor_curp(co) != NULL) here?
>
> This approach looks workable, though the implementation of
> monitor_curp() feels a bit brittle. The code is not significantly
> simpler than the hash table based approach, but the assumptions it makes
> are a bit more hidden.
>
> Saving the locks is more a theoretical improvement because all callers
> are slows paths anyway.

The hash table only ever has three keys: qmp_dispatcher_co, the
coroutine hidden in handle_hmp_command(), and mon_iothread's leader (not
in coroutine context).

My version replaces the hash table by three pointer variables (two in
the sketch above, because I didn't implement the third).

You point out my code relies on an argument about which coroutines can
execute commands.  True.  But I have to make that argument anyway to
understand how the coroutine-enabled monitor works.

On the other hand, it doesn't rely on an argument about the consistency
of the hash table with the coroutines.

[PATCH] Coroutine-aware monitor_cur() with coroutine-specific data

[Markus Armbruster]

This is just a sketch.  It needs comments and a real commit message.

As is, it goes on top of Kevin's series.  It is meant to be squashed
into PATCH 06.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/coroutine.h     |  4 ++++
 include/qemu/coroutine_int.h |  2 ++
 monitor/monitor.c            | 36 +++++++++++++++---------------------
 util/qemu-coroutine.c        | 20 ++++++++++++++++++++
 4 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index dfd261c5b1..11da47092c 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -65,6 +65,10 @@ typedef void coroutine_fn CoroutineEntry(void *opaque);
  */
 Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque);
 
+Coroutine *qemu_coroutine_create_with_storage(CoroutineEntry *entry,
+                                              void *opaque, size_t storage);
+void *qemu_coroutine_local_storage(Coroutine *co);
+
 /**
  * Transfer control to a coroutine
  */
diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index bd6b0468e1..7d7865a02f 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -41,6 +41,8 @@ struct Coroutine {
     void *entry_arg;
     Coroutine *caller;
 
+    void *coroutine_local_storage;
+
     /* Only used when the coroutine has terminated.  */
     QSLIST_ENTRY(Coroutine) pool_next;
 
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 50fb5b20d3..047a8fb380 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -82,38 +82,32 @@ bool qmp_dispatcher_co_shutdown;
  */
 bool qmp_dispatcher_co_busy;
 
-/*
- * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
- * monitor_destroyed.
- */
+/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */
 QemuMutex monitor_lock;
 static GHashTable *monitor_qapi_event_state;
-static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
 
 MonitorList mon_list;
 int mon_refcount;
 static bool monitor_destroyed;
 
+static Monitor **monitor_curp(Coroutine *co)
+{
+    static __thread Monitor *global_cur_mon;
+
+    if (co == qmp_dispatcher_co) {
+        return qemu_coroutine_local_storage(co);
+    }
+    return &global_cur_mon;
+}
+
 Monitor *monitor_cur(void)
 {
-    Monitor *mon;
-
-    qemu_mutex_lock(&monitor_lock);
-    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
-    qemu_mutex_unlock(&monitor_lock);
-
-    return mon;
+    return *monitor_curp(qemu_coroutine_self());
 }
 
 void monitor_set_cur(Coroutine *co, Monitor *mon)
 {
-    qemu_mutex_lock(&monitor_lock);
-    if (mon) {
-        g_hash_table_replace(coroutine_mon, co, mon);
-    } else {
-        g_hash_table_remove(coroutine_mon, co);
-    }
-    qemu_mutex_unlock(&monitor_lock);
+    *monitor_curp(co) = mon;
 }
 
 /**
@@ -666,14 +660,14 @@ void monitor_init_globals_core(void)
 {
     monitor_qapi_event_init();
     qemu_mutex_init(&monitor_lock);
-    coroutine_mon = g_hash_table_new(NULL, NULL);
 
     /*
      * The dispatcher BH must run in the main loop thread, since we
      * have commands assuming that context.  It would be nice to get
      * rid of those assumptions.
      */
-    qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL);
+    qmp_dispatcher_co = qemu_coroutine_create_with_storage(
+        monitor_qmp_dispatcher_co, NULL, sizeof(Monitor **));
     atomic_mb_set(&qmp_dispatcher_co_busy, true);
     aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
 }
diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
index c3caa6c770..87bf7f0fc0 100644
--- a/util/qemu-coroutine.c
+++ b/util/qemu-coroutine.c
@@ -81,8 +81,28 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque)
     return co;
 }
 
+Coroutine *qemu_coroutine_create_with_storage(CoroutineEntry *entry,
+                                              void *opaque, size_t storage)
+{
+    Coroutine *co = qemu_coroutine_create(entry, opaque);
+
+    if (!co) {
+        return NULL;
+    }
+
+    co->coroutine_local_storage = g_malloc0(storage);
+    return co;
+}
+
+void *qemu_coroutine_local_storage(Coroutine *co)
+{
+    return co->coroutine_local_storage;
+}
+
 static void coroutine_delete(Coroutine *co)
 {
+    g_free(co->coroutine_local_storage);
+    co->coroutine_local_storage = NULL;
     co->caller = NULL;
 
     if (CONFIG_COROUTINE_POOL) {
-- 
2.26.2

Re: [PATCH] Coroutine-aware monitor_cur() with coroutine-specific data

[Kevin Wolf]

Am 07.08.2020 um 15:29 hat Markus Armbruster geschrieben:
> This is just a sketch.  It needs comments and a real commit message.
> 
> As is, it goes on top of Kevin's series.  It is meant to be squashed
> into PATCH 06.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qemu/coroutine.h     |  4 ++++
>  include/qemu/coroutine_int.h |  2 ++
>  monitor/monitor.c            | 36 +++++++++++++++---------------------
>  util/qemu-coroutine.c        | 20 ++++++++++++++++++++
>  4 files changed, 41 insertions(+), 21 deletions(-)
> 
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index dfd261c5b1..11da47092c 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -65,6 +65,10 @@ typedef void coroutine_fn CoroutineEntry(void *opaque);
>   */
>  Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque);
>  
> +Coroutine *qemu_coroutine_create_with_storage(CoroutineEntry *entry,
> +                                              void *opaque, size_t storage);
> +void *qemu_coroutine_local_storage(Coroutine *co);
> +
>  /**
>   * Transfer control to a coroutine
>   */
> diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
> index bd6b0468e1..7d7865a02f 100644
> --- a/include/qemu/coroutine_int.h
> +++ b/include/qemu/coroutine_int.h
> @@ -41,6 +41,8 @@ struct Coroutine {
>      void *entry_arg;
>      Coroutine *caller;
>  
> +    void *coroutine_local_storage;
> +
>      /* Only used when the coroutine has terminated.  */
>      QSLIST_ENTRY(Coroutine) pool_next;

This increases the size of Coroutine objects typically by 8 bytes and
shifts the following fields by the same amount. On my x86_64 build, we
have exactly those 8 bytes left in CoroutineUContext until a new
cacheline would start. With different CONFIG_* settings, it could be the
change that increases the size to a new cacheline. No idea what this
looks like on other architectures.

Does this or the shifting of fields matter for performance? I don't
know. It might even be unlikely. But cache effects are hard to predict
and not wanting to do the work of proving that it's indeed harmless is
one of the reasons why for the slow paths in question I preferred a
solution that doesn't touch the coroutine core at all.

> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 50fb5b20d3..047a8fb380 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -82,38 +82,32 @@ bool qmp_dispatcher_co_shutdown;
>   */
>  bool qmp_dispatcher_co_busy;
>  
> -/*
> - * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> - * monitor_destroyed.
> - */
> +/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */
>  QemuMutex monitor_lock;
>  static GHashTable *monitor_qapi_event_state;
> -static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
>  
>  MonitorList mon_list;
>  int mon_refcount;
>  static bool monitor_destroyed;
>  
> +static Monitor **monitor_curp(Coroutine *co)
> +{
> +    static __thread Monitor *global_cur_mon;
> +
> +    if (co == qmp_dispatcher_co) {
> +        return qemu_coroutine_local_storage(co);
> +    }
> +    return &global_cur_mon;
> +}

Like the other patch, this needs to be extended for HMP. global_cur_mon
is never meant to be set.

The solution fails as soon as we have more than a single monitor
coroutine running at the same time because it relies on
qmp_dispatcher_co. In this respect, it makes the same assumptions as the
simple hack.

Only knowing that qmp_dispatcher_co is always created with storage
containing a Monitor** makes this safe.

>  Monitor *monitor_cur(void)
>  {
> -    Monitor *mon;
> -
> -    qemu_mutex_lock(&monitor_lock);
> -    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
> -    qemu_mutex_unlock(&monitor_lock);
> -
> -    return mon;
> +    return *monitor_curp(qemu_coroutine_self());
>  }
>  
>  void monitor_set_cur(Coroutine *co, Monitor *mon)
>  {
> -    qemu_mutex_lock(&monitor_lock);
> -    if (mon) {
> -        g_hash_table_replace(coroutine_mon, co, mon);
> -    } else {
> -        g_hash_table_remove(coroutine_mon, co);
> -    }
> -    qemu_mutex_unlock(&monitor_lock);
> +    *monitor_curp(co) = mon;
>  }
>  
>  /**
> @@ -666,14 +660,14 @@ void monitor_init_globals_core(void)
>  {
>      monitor_qapi_event_init();
>      qemu_mutex_init(&monitor_lock);
> -    coroutine_mon = g_hash_table_new(NULL, NULL);
>  
>      /*
>       * The dispatcher BH must run in the main loop thread, since we
>       * have commands assuming that context.  It would be nice to get
>       * rid of those assumptions.
>       */
> -    qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL);
> +    qmp_dispatcher_co = qemu_coroutine_create_with_storage(
> +        monitor_qmp_dispatcher_co, NULL, sizeof(Monitor **));
>      atomic_mb_set(&qmp_dispatcher_co_busy, true);
>      aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
>  }
> diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> index c3caa6c770..87bf7f0fc0 100644
> --- a/util/qemu-coroutine.c
> +++ b/util/qemu-coroutine.c
> @@ -81,8 +81,28 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque)
>      return co;
>  }
>  
> +Coroutine *qemu_coroutine_create_with_storage(CoroutineEntry *entry,
> +                                              void *opaque, size_t storage)
> +{
> +    Coroutine *co = qemu_coroutine_create(entry, opaque);
> +
> +    if (!co) {
> +        return NULL;
> +    }
> +
> +    co->coroutine_local_storage = g_malloc0(storage);
> +    return co;
> +}

As the code above shows, this interface is only useful if you can
identify the coroutine. It cannot be used in code that didn't create the
current coroutine because then it can't know whether or not the
coroutine has coroutine local storage, and if it has, what its structure
is.

For a supposedly generic solution, I think this is a bit weak.
Effectively, this might be a one-off solution in disguise because
it's a big restriction on the possible use cases.

> +void *qemu_coroutine_local_storage(Coroutine *co)
> +{
> +    return co->coroutine_local_storage;
> +}
> +
>  static void coroutine_delete(Coroutine *co)
>  {
> +    g_free(co->coroutine_local_storage);
> +    co->coroutine_local_storage = NULL;
>      co->caller = NULL;
>  
>      if (CONFIG_COROUTINE_POOL) {

Your list of pros/cons didn't mention coroutine creation/deletion as a
hot path at all (which it is, we have one coroutine per request).

You leave qemu_coroutine_create() untouched (except indirectly by a
larger g_malloc0() in the non-pooled case, which is negligible) and I
assume that g_free(NULL) is cheap, so at least this is probably as good
as it gets for something integrated in the coroutine core. Maybe an
explicit if (co->coroutine_local_storage) would improve it slightly.

Kevin

Re: [PATCH] Coroutine-aware monitor_cur() with coroutine-specific data

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 07.08.2020 um 15:29 hat Markus Armbruster geschrieben:
>> This is just a sketch.  It needs comments and a real commit message.
>> 
>> As is, it goes on top of Kevin's series.  It is meant to be squashed
>> into PATCH 06.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/qemu/coroutine.h     |  4 ++++
>>  include/qemu/coroutine_int.h |  2 ++
>>  monitor/monitor.c            | 36 +++++++++++++++---------------------
>>  util/qemu-coroutine.c        | 20 ++++++++++++++++++++
>>  4 files changed, 41 insertions(+), 21 deletions(-)
>> 
>> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
>> index dfd261c5b1..11da47092c 100644
>> --- a/include/qemu/coroutine.h
>> +++ b/include/qemu/coroutine.h
>> @@ -65,6 +65,10 @@ typedef void coroutine_fn CoroutineEntry(void *opaque);
>>   */
>>  Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque);
>>  
>> +Coroutine *qemu_coroutine_create_with_storage(CoroutineEntry *entry,
>> +                                              void *opaque, size_t storage);
>> +void *qemu_coroutine_local_storage(Coroutine *co);
>> +
>>  /**
>>   * Transfer control to a coroutine
>>   */
>> diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
>> index bd6b0468e1..7d7865a02f 100644
>> --- a/include/qemu/coroutine_int.h
>> +++ b/include/qemu/coroutine_int.h
>> @@ -41,6 +41,8 @@ struct Coroutine {
>>      void *entry_arg;
>>      Coroutine *caller;
>>  
>> +    void *coroutine_local_storage;
>> +
>>      /* Only used when the coroutine has terminated.  */
>>      QSLIST_ENTRY(Coroutine) pool_next;
>
> This increases the size of Coroutine objects typically by 8 bytes and
> shifts the following fields by the same amount. On my x86_64 build, we
> have exactly those 8 bytes left in CoroutineUContext until a new
> cacheline would start. With different CONFIG_* settings, it could be the
> change that increases the size to a new cacheline. No idea what this
> looks like on other architectures.
>
> Does this or the shifting of fields matter for performance? I don't
> know. It might even be unlikely. But cache effects are hard to predict
> and not wanting to do the work of proving that it's indeed harmless is
> one of the reasons why for the slow paths in question I preferred a
> solution that doesn't touch the coroutine core at all.

Point taken.

Possible mitigation: add at the end rather than in the middle.

>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>> index 50fb5b20d3..047a8fb380 100644
>> --- a/monitor/monitor.c
>> +++ b/monitor/monitor.c
>> @@ -82,38 +82,32 @@ bool qmp_dispatcher_co_shutdown;
>>   */
>>  bool qmp_dispatcher_co_busy;
>>  
>> -/*
>> - * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
>> - * monitor_destroyed.
>> - */
>> +/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */
>>  QemuMutex monitor_lock;
>>  static GHashTable *monitor_qapi_event_state;
>> -static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
>>  
>>  MonitorList mon_list;
>>  int mon_refcount;
>>  static bool monitor_destroyed;
>>  
>> +static Monitor **monitor_curp(Coroutine *co)
>> +{
>> +    static __thread Monitor *global_cur_mon;
>> +
>> +    if (co == qmp_dispatcher_co) {
>> +        return qemu_coroutine_local_storage(co);
>> +    }
>> +    return &global_cur_mon;
>> +}
>
> Like the other patch, this needs to be extended for HMP. global_cur_mon
> is never meant to be set.

It is, for OOB commands.

> The solution fails as soon as we have more than a single monitor
> coroutine running at the same time because it relies on
> qmp_dispatcher_co.

Yes, but pretty much everything below handle_qmp_command() falls apart
then.  Remembering to update monitor_curp() would be the least of my
worries :)

>                    In this respect, it makes the same assumptions as the
> simple hack.
>
> Only knowing that qmp_dispatcher_co is always created with storage
> containing a Monitor** makes this safe.

Correct.

>>  Monitor *monitor_cur(void)
>>  {
>> -    Monitor *mon;
>> -
>> -    qemu_mutex_lock(&monitor_lock);
>> -    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
>> -    qemu_mutex_unlock(&monitor_lock);
>> -
>> -    return mon;
>> +    return *monitor_curp(qemu_coroutine_self());
>>  }
>>  
>>  void monitor_set_cur(Coroutine *co, Monitor *mon)
>>  {
>> -    qemu_mutex_lock(&monitor_lock);
>> -    if (mon) {
>> -        g_hash_table_replace(coroutine_mon, co, mon);
>> -    } else {
>> -        g_hash_table_remove(coroutine_mon, co);
>> -    }
>> -    qemu_mutex_unlock(&monitor_lock);
>> +    *monitor_curp(co) = mon;
>>  }
>>  
>>  /**
>> @@ -666,14 +660,14 @@ void monitor_init_globals_core(void)
>>  {
>>      monitor_qapi_event_init();
>>      qemu_mutex_init(&monitor_lock);
>> -    coroutine_mon = g_hash_table_new(NULL, NULL);
>>  
>>      /*
>>       * The dispatcher BH must run in the main loop thread, since we
>>       * have commands assuming that context.  It would be nice to get
>>       * rid of those assumptions.
>>       */
>> -    qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL);
>> +    qmp_dispatcher_co = qemu_coroutine_create_with_storage(
>> +        monitor_qmp_dispatcher_co, NULL, sizeof(Monitor **));
>>      atomic_mb_set(&qmp_dispatcher_co_busy, true);
>>      aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
>>  }
>> diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
>> index c3caa6c770..87bf7f0fc0 100644
>> --- a/util/qemu-coroutine.c
>> +++ b/util/qemu-coroutine.c
>> @@ -81,8 +81,28 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque)
>>      return co;
>>  }
>>  
>> +Coroutine *qemu_coroutine_create_with_storage(CoroutineEntry *entry,
>> +                                              void *opaque, size_t storage)
>> +{
>> +    Coroutine *co = qemu_coroutine_create(entry, opaque);
>> +
>> +    if (!co) {
>> +        return NULL;
>> +    }
>> +
>> +    co->coroutine_local_storage = g_malloc0(storage);
>> +    return co;
>> +}
>
> As the code above shows, this interface is only useful if you can
> identify the coroutine. It cannot be used in code that didn't create the
> current coroutine because then it can't know whether or not the
> coroutine has coroutine local storage, and if it has, what its structure
> is.
>
> For a supposedly generic solution, I think this is a bit weak.

Yes, that's fair.

The solution Daniel proposed is makes the weakness more explicit:
instead of relying on "coroutine was created with this coroutine-local
storage", we'd rely on "coroutine_getspecific(key) does not fail".  It
can fail only if coroutine_setspecific(key, ...) was not called.  Not
much better in practice.

> Effectively, this might be a one-off solution in disguise because
> it's a big restriction on the possible use cases.

Daniel's solution is basically pthread_getspecific() for coroutines,
with the keys dumbed down.

If pthread_getspecific() was good enough for pthreads...

Well, it wasn't, or rather it was only because something better could
not be had with just a library, without toolchain support.  And that's
where we are with coroutines.

>> +void *qemu_coroutine_local_storage(Coroutine *co)
>> +{
>> +    return co->coroutine_local_storage;
>> +}
>> +
>>  static void coroutine_delete(Coroutine *co)
>>  {
>> +    g_free(co->coroutine_local_storage);
>> +    co->coroutine_local_storage = NULL;
>>      co->caller = NULL;
>>  
>>      if (CONFIG_COROUTINE_POOL) {
>
> Your list of pros/cons didn't mention coroutine creation/deletion as a
> hot path at all (which it is, we have one coroutine per request).

I did not expect coroutine creation / deletion to be a hot path.

It is not a hot path for QMP, because QMP is not a hot path.

I'm ready to accept the proposition that it's a hot path elsewhere.

> You leave qemu_coroutine_create() untouched (except indirectly by a
> larger g_malloc0() in the non-pooled case, which is negligible) and I
> assume that g_free(NULL) is cheap, so at least this is probably as good
> as it gets for something integrated in the coroutine core. Maybe an
> explicit if (co->coroutine_local_storage) would improve it slightly.
>
> Kevin

Re: [PATCH] Coroutine-aware monitor_cur() with coroutine-specific data

[Kevin Wolf]

Am 26.08.2020 um 14:40 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 07.08.2020 um 15:29 hat Markus Armbruster geschrieben:
> >> This is just a sketch.  It needs comments and a real commit message.
> >> 
> >> As is, it goes on top of Kevin's series.  It is meant to be squashed
> >> into PATCH 06.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  include/qemu/coroutine.h     |  4 ++++
> >>  include/qemu/coroutine_int.h |  2 ++
> >>  monitor/monitor.c            | 36 +++++++++++++++---------------------
> >>  util/qemu-coroutine.c        | 20 ++++++++++++++++++++
> >>  4 files changed, 41 insertions(+), 21 deletions(-)
> >> 
> >> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> >> index dfd261c5b1..11da47092c 100644
> >> --- a/include/qemu/coroutine.h
> >> +++ b/include/qemu/coroutine.h
> >> @@ -65,6 +65,10 @@ typedef void coroutine_fn CoroutineEntry(void *opaque);
> >>   */
> >>  Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque);
> >>  
> >> +Coroutine *qemu_coroutine_create_with_storage(CoroutineEntry *entry,
> >> +                                              void *opaque, size_t storage);
> >> +void *qemu_coroutine_local_storage(Coroutine *co);
> >> +
> >>  /**
> >>   * Transfer control to a coroutine
> >>   */
> >> diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
> >> index bd6b0468e1..7d7865a02f 100644
> >> --- a/include/qemu/coroutine_int.h
> >> +++ b/include/qemu/coroutine_int.h
> >> @@ -41,6 +41,8 @@ struct Coroutine {
> >>      void *entry_arg;
> >>      Coroutine *caller;
> >>  
> >> +    void *coroutine_local_storage;
> >> +
> >>      /* Only used when the coroutine has terminated.  */
> >>      QSLIST_ENTRY(Coroutine) pool_next;
> >
> > This increases the size of Coroutine objects typically by 8 bytes and
> > shifts the following fields by the same amount. On my x86_64 build, we
> > have exactly those 8 bytes left in CoroutineUContext until a new
> > cacheline would start. With different CONFIG_* settings, it could be the
> > change that increases the size to a new cacheline. No idea what this
> > looks like on other architectures.
> >
> > Does this or the shifting of fields matter for performance? I don't
> > know. It might even be unlikely. But cache effects are hard to predict
> > and not wanting to do the work of proving that it's indeed harmless is
> > one of the reasons why for the slow paths in question I preferred a
> > solution that doesn't touch the coroutine core at all.
> 
> Point taken.
> 
> Possible mitigation: add at the end rather than in the middle.

Doesn't work: This is a struct that is embedded at the start of
CoroutineUContext, so while you can move it down a bit, you'll never get
to the end of the actual struct used at runtime.

> >> diff --git a/monitor/monitor.c b/monitor/monitor.c
> >> index 50fb5b20d3..047a8fb380 100644
> >> --- a/monitor/monitor.c
> >> +++ b/monitor/monitor.c
> >> @@ -82,38 +82,32 @@ bool qmp_dispatcher_co_shutdown;
> >>   */
> >>  bool qmp_dispatcher_co_busy;
> >>  
> >> -/*
> >> - * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> >> - * monitor_destroyed.
> >> - */
> >> +/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed. */
> >>  QemuMutex monitor_lock;
> >>  static GHashTable *monitor_qapi_event_state;
> >> -static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
> >>  
> >>  MonitorList mon_list;
> >>  int mon_refcount;
> >>  static bool monitor_destroyed;
> >>  
> >> +static Monitor **monitor_curp(Coroutine *co)
> >> +{
> >> +    static __thread Monitor *global_cur_mon;
> >> +
> >> +    if (co == qmp_dispatcher_co) {
> >> +        return qemu_coroutine_local_storage(co);
> >> +    }
> >> +    return &global_cur_mon;
> >> +}
> >
> > Like the other patch, this needs to be extended for HMP. global_cur_mon
> > is never meant to be set.
> 
> It is, for OOB commands.

Right, I missed this.

> > The solution fails as soon as we have more than a single monitor
> > coroutine running at the same time because it relies on
> > qmp_dispatcher_co.
> 
> Yes, but pretty much everything below handle_qmp_command() falls apart
> then.  Remembering to update monitor_curp() would be the least of my
> worries :)

Fair enough.

> >                    In this respect, it makes the same assumptions as the
> > simple hack.
> >
> > Only knowing that qmp_dispatcher_co is always created with storage
> > containing a Monitor** makes this safe.
> 
> Correct.
> 
> >>  Monitor *monitor_cur(void)
> >>  {
> >> -    Monitor *mon;
> >> -
> >> -    qemu_mutex_lock(&monitor_lock);
> >> -    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
> >> -    qemu_mutex_unlock(&monitor_lock);
> >> -
> >> -    return mon;
> >> +    return *monitor_curp(qemu_coroutine_self());
> >>  }
> >>  
> >>  void monitor_set_cur(Coroutine *co, Monitor *mon)
> >>  {
> >> -    qemu_mutex_lock(&monitor_lock);
> >> -    if (mon) {
> >> -        g_hash_table_replace(coroutine_mon, co, mon);
> >> -    } else {
> >> -        g_hash_table_remove(coroutine_mon, co);
> >> -    }
> >> -    qemu_mutex_unlock(&monitor_lock);
> >> +    *monitor_curp(co) = mon;
> >>  }
> >>  
> >>  /**
> >> @@ -666,14 +660,14 @@ void monitor_init_globals_core(void)
> >>  {
> >>      monitor_qapi_event_init();
> >>      qemu_mutex_init(&monitor_lock);
> >> -    coroutine_mon = g_hash_table_new(NULL, NULL);
> >>  
> >>      /*
> >>       * The dispatcher BH must run in the main loop thread, since we
> >>       * have commands assuming that context.  It would be nice to get
> >>       * rid of those assumptions.
> >>       */
> >> -    qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL);
> >> +    qmp_dispatcher_co = qemu_coroutine_create_with_storage(
> >> +        monitor_qmp_dispatcher_co, NULL, sizeof(Monitor **));
> >>      atomic_mb_set(&qmp_dispatcher_co_busy, true);
> >>      aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
> >>  }
> >> diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> >> index c3caa6c770..87bf7f0fc0 100644
> >> --- a/util/qemu-coroutine.c
> >> +++ b/util/qemu-coroutine.c
> >> @@ -81,8 +81,28 @@ Coroutine *qemu_coroutine_create(CoroutineEntry *entry, void *opaque)
> >>      return co;
> >>  }
> >>  
> >> +Coroutine *qemu_coroutine_create_with_storage(CoroutineEntry *entry,
> >> +                                              void *opaque, size_t storage)
> >> +{
> >> +    Coroutine *co = qemu_coroutine_create(entry, opaque);
> >> +
> >> +    if (!co) {
> >> +        return NULL;
> >> +    }
> >> +
> >> +    co->coroutine_local_storage = g_malloc0(storage);
> >> +    return co;
> >> +}
> >
> > As the code above shows, this interface is only useful if you can
> > identify the coroutine. It cannot be used in code that didn't create the
> > current coroutine because then it can't know whether or not the
> > coroutine has coroutine local storage, and if it has, what its structure
> > is.
> >
> > For a supposedly generic solution, I think this is a bit weak.
> 
> Yes, that's fair.
> 
> The solution Daniel proposed is makes the weakness more explicit:
> instead of relying on "coroutine was created with this coroutine-local
> storage", we'd rely on "coroutine_getspecific(key) does not fail".  It
> can fail only if coroutine_setspecific(key, ...) was not called.  Not
> much better in practice.

It would be a little more generic, I guess. So for a solution that wants
to look generic, it might be better.

But as long as we don't have a second user (not even in our
imagination), I'm not sure how important it is to have something that
looks generic. With the simple and stupid patch, it would be more
obvious that it doesn't hurt cases that are unrelated to the monitor.

> > Effectively, this might be a one-off solution in disguise because
> > it's a big restriction on the possible use cases.
> 
> Daniel's solution is basically pthread_getspecific() for coroutines,
> with the keys dumbed down.
> 
> If pthread_getspecific() was good enough for pthreads...
> 
> Well, it wasn't, or rather it was only because something better could
> not be had with just a library, without toolchain support.  And that's
> where we are with coroutines.
> 
> >> +void *qemu_coroutine_local_storage(Coroutine *co)
> >> +{
> >> +    return co->coroutine_local_storage;
> >> +}
> >> +
> >>  static void coroutine_delete(Coroutine *co)
> >>  {
> >> +    g_free(co->coroutine_local_storage);
> >> +    co->coroutine_local_storage = NULL;
> >>      co->caller = NULL;
> >>  
> >>      if (CONFIG_COROUTINE_POOL) {
> >
> > Your list of pros/cons didn't mention coroutine creation/deletion as a
> > hot path at all (which it is, we have one coroutine per request).
> 
> I did not expect coroutine creation / deletion to be a hot path.
> 
> It is not a hot path for QMP, because QMP is not a hot path.
> 
> I'm ready to accept the proposition that it's a hot path elsewhere.

It is a hot path for block device requests. To be more specific, the
part that is executed when taking a coroutine from the pool or putting
it back to the pool is. The code to create a coroutine from scratch or
to actually free it may be less relevant.

Kevin

> > You leave qemu_coroutine_create() untouched (except indirectly by a
> > larger g_malloc0() in the non-pooled case, which is negligible) and I
> > assume that g_free(NULL) is cheap, so at least this is probably as good
> > as it gets for something integrated in the coroutine core. Maybe an
> > explicit if (co->coroutine_local_storage) would improve it slightly.
> >
> > Kevin

Re: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property

[Daniel P. Berrangé]

On Tue, Aug 04, 2020 at 03:50:54PM +0200, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > This way, a monitor command handler will still be able to access the
> > current monitor, but when it yields, all other code code will correctly
> > get NULL from monitor_cur().
> >
> > Outside of coroutine context, qemu_coroutine_self() returns the leader
> > coroutine of the current thread.
> 
> Unsaid: you use it as a hash table key to map from coroutine to monitor,
> and for that you need it to return a value unique to the coroutine in
> coroutine context, and a value unique to the thread outside coroutine
> context.  Which qemu_coroutine_self() does.  Correct?
> 
> The hash table works, but I hate it just as much as I hate
> pthread_getspecific() / pthread_setspecific().
> 
> What we have here is a need for coroutine-local data.  Feels like a
> perfectly natural concept to me.
> 
> Are we going to create another hash table whenever we need another piece
> of coroutine-local data?  Or shall we reuse the hash table, suitably
> renamed and moved to another file?
> 
> Why not simply associate an opaque pointer with each coroutine?  All it
> takes is one more member of struct Coroutine.  Whatever creates the
> coroutine decides what to use it for.  The monitor coroutine would use
> it to point to the monitor.

Possible benefit of having the coroutine-local data stored in the
coroutine stack is that we can probably make it lock-less. Using
the hash table in monitor.c results in a serialization of across
all coroutines & threads.

Also, by providing a GDestroyNotify against the coroutine-local
data we can easily guarantee cleanup with the coroutine is freed.

Since we'll have a limited number of data items, we could make do
with a simple array in the coroutine struct, instead of a hashtable.
eg

  enum CoroutineLocalKeys {
     CO_LOCAL_CUR_MONITOR = 0,

     CO_LOCAL_LAST,
  };

  struct Coroutine {
    ...
    gpointer localData[CO_LOCAL_LAST];
    GDestroyNotify localDataFree[CO_LOCAL_LAST];
  };


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v6 07/12] qapi: Add a 'coroutine' flag for commands

[Kevin Wolf]

This patch adds a new 'coroutine' flag to QMP command definitions that
tells the QMP dispatcher that the command handler is safe to be run in a
coroutine.

The documentation of the new flag pretends that this flag is already
used as intended, which it isn't yet after this patch. We'll implement
this in another patch in this series.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.txt            | 12 ++++++++++++
 include/qapi/qmp/dispatch.h             |  1 +
 tests/test-qmp-cmds.c                   |  4 ++++
 scripts/qapi/commands.py                | 10 +++++++---
 scripts/qapi/doc.py                     |  2 +-
 scripts/qapi/expr.py                    | 10 ++++++++--
 scripts/qapi/introspect.py              |  2 +-
 scripts/qapi/schema.py                  | 12 ++++++++----
 tests/qapi-schema/test-qapi.py          |  7 ++++---
 tests/Makefile.include                  |  1 +
 tests/qapi-schema/oob-coroutine.err     |  2 ++
 tests/qapi-schema/oob-coroutine.json    |  2 ++
 tests/qapi-schema/oob-coroutine.out     |  0
 tests/qapi-schema/qapi-schema-test.json |  1 +
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 15 files changed, 54 insertions(+), 14 deletions(-)
 create mode 100644 tests/qapi-schema/oob-coroutine.err
 create mode 100644 tests/qapi-schema/oob-coroutine.json
 create mode 100644 tests/qapi-schema/oob-coroutine.out

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index a7794ef658..b8eb370a13 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -472,6 +472,7 @@ Syntax:
                 '*gen': false,
                 '*allow-oob': true,
                 '*allow-preconfig': true,
+                '*coroutine': true,
                 '*if': COND,
                 '*features': FEATURES }
 
@@ -596,6 +597,17 @@ before the machine is built.  It defaults to false.  For example:
 QMP is available before the machine is built only when QEMU was
 started with --preconfig.
 
+Member 'coroutine' tells the QMP dispatcher whether the command handler
+is safe to be run in a coroutine.  It defaults to false.  If it is true,
+the command handler is called from coroutine context and may yield while
+waiting for an external event (such as I/O completion) in order to avoid
+blocking the guest and other background operations.
+
+It is an error to specify both 'coroutine': true and 'allow-oob': true
+for a command.  We don't currently have a use case for both together and
+without a use case, it's not entirely clear what the semantics should
+be.
+
 The optional 'if' member specifies a conditional.  See "Configuring
 the schema" below for more on this.
 
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 0c2f467028..9fd2b720a7 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -25,6 +25,7 @@ typedef enum QmpCommandOptions
     QCO_NO_SUCCESS_RESP       =  (1U << 0),
     QCO_ALLOW_OOB             =  (1U << 1),
     QCO_ALLOW_PRECONFIG       =  (1U << 2),
+    QCO_COROUTINE             =  (1U << 3),
 } QmpCommandOptions;
 
 typedef struct QmpCommand
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 5f1b245e19..d3413bfef0 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -36,6 +36,10 @@ void qmp_cmd_success_response(Error **errp)
 {
 }
 
+void qmp_coroutine_cmd(Error **errp)
+{
+}
+
 Empty2 *qmp_user_def_cmd0(Error **errp)
 {
     return g_new0(Empty2, 1);
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 6809b0fb6e..f50710306d 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -180,7 +180,8 @@ out:
     return ret
 
 
-def gen_register_command(name, success_response, allow_oob, allow_preconfig):
+def gen_register_command(name, success_response, allow_oob, allow_preconfig,
+                         coroutine):
     options = []
 
     if not success_response:
@@ -189,6 +190,8 @@ def gen_register_command(name, success_response, allow_oob, allow_preconfig):
         options += ['QCO_ALLOW_OOB']
     if allow_preconfig:
         options += ['QCO_ALLOW_PRECONFIG']
+    if coroutine:
+        options += ['QCO_COROUTINE']
 
     if not options:
         options = ['QCO_NO_OPTIONS']
@@ -271,7 +274,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         if not gen:
             return
         # FIXME: If T is a user-defined type, the user is responsible
@@ -289,7 +292,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
             self._genh.add(gen_marshal_decl(name))
             self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
             self._regy.add(gen_register_command(name, success_response,
-                                                allow_oob, allow_preconfig))
+                                                allow_oob, allow_preconfig,
+                                                coroutine))
 
 
 def gen_commands(schema, output_dir, prefix):
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 92f584edcf..11771de923 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -264,7 +264,7 @@ class QAPISchemaGenDocVisitor(QAPISchemaVisitor):
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         doc = self.cur_doc
         self._gen.add(texi_msg('Command', doc, ifcond,
                                texi_arguments(doc,
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 2942520399..928cd1eb5c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -88,10 +88,16 @@ def check_flags(expr, info):
         if key in expr and expr[key] is not False:
             raise QAPISemError(
                 info, "flag '%s' may only use false value" % key)
-    for key in ['boxed', 'allow-oob', 'allow-preconfig']:
+    for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:
         if key in expr and expr[key] is not True:
             raise QAPISemError(
                 info, "flag '%s' may only use true value" % key)
+    if 'allow-oob' in expr and 'coroutine' in expr:
+        # This is not necessarily a fundamental incompatibility, but we don't
+        # have a use case and the desired semantics isn't obvious. The simplest
+        # solution is to forbid it until we get a use case for it.
+        raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
+                                 "are incompatible")
 
 
 def check_if(expr, info, source):
@@ -342,7 +348,7 @@ def check_exprs(exprs):
                        ['command'],
                        ['data', 'returns', 'boxed', 'if', 'features',
                         'gen', 'success-response', 'allow-oob',
-                        'allow-preconfig'])
+                        'allow-preconfig', 'coroutine'])
             normalize_members(expr.get('data'))
             check_command(expr, info)
         elif meta == 'event':
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 23652be810..5907b09cd5 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -216,7 +216,7 @@ const QLitObject %(c_name)s = %(c_string)s;
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         obj = {'arg-type': self._use_type(arg_type),
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 78309a00f0..c44d391c3f 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -128,7 +128,7 @@ class QAPISchemaVisitor:
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         pass
 
     def visit_event(self, name, info, ifcond, features, arg_type, boxed):
@@ -713,7 +713,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
 
     def __init__(self, name, info, doc, ifcond, features,
                  arg_type, ret_type,
-                 gen, success_response, boxed, allow_oob, allow_preconfig):
+                 gen, success_response, boxed, allow_oob, allow_preconfig,
+                 coroutine):
         super().__init__(name, info, doc, ifcond, features)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -726,6 +727,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         self.boxed = boxed
         self.allow_oob = allow_oob
         self.allow_preconfig = allow_preconfig
+        self.coroutine = coroutine
 
     def check(self, schema):
         super().check(schema)
@@ -768,7 +770,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         visitor.visit_command(
             self.name, self.info, self.ifcond, self.features,
             self.arg_type, self.ret_type, self.gen, self.success_response,
-            self.boxed, self.allow_oob, self.allow_preconfig)
+            self.boxed, self.allow_oob, self.allow_preconfig, self.coroutine)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1074,6 +1076,7 @@ class QAPISchema:
         boxed = expr.get('boxed', False)
         allow_oob = expr.get('allow-oob', False)
         allow_preconfig = expr.get('allow-preconfig', False)
+        coroutine = expr.get('coroutine', False)
         ifcond = expr.get('if')
         features = self._make_features(expr.get('features'), info)
         if isinstance(data, OrderedDict):
@@ -1086,7 +1089,8 @@ class QAPISchema:
         self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features,
                                            data, rets,
                                            gen, success_response,
-                                           boxed, allow_oob, allow_preconfig))
+                                           boxed, allow_oob, allow_preconfig,
+                                           coroutine))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index f396b471eb..e8db9d09d9 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -68,12 +68,13 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         print('command %s %s -> %s'
               % (name, arg_type and arg_type.name,
                  ret_type and ret_type.name))
-        print('    gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
-              % (gen, success_response, boxed, allow_oob, allow_preconfig))
+        print('    gen=%s success_response=%s boxed=%s oob=%s preconfig=%s%s'
+              % (gen, success_response, boxed, allow_oob, allow_preconfig,
+                 " coroutine=True" if coroutine else ""))
         self._print_if(ifcond)
         self._print_features(features)
 
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 03a74b60f6..bb69487436 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -290,6 +290,7 @@ qapi-schema += missing-type.json
 qapi-schema += nested-struct-data.json
 qapi-schema += nested-struct-data-invalid-dict.json
 qapi-schema += non-objects.json
+qapi-schema += oob-coroutine.json
 qapi-schema += oob-test.json
 qapi-schema += allow-preconfig-test.json
 qapi-schema += pragma-doc-required-crap.json
diff --git a/tests/qapi-schema/oob-coroutine.err b/tests/qapi-schema/oob-coroutine.err
new file mode 100644
index 0000000000..c01a4992bd
--- /dev/null
+++ b/tests/qapi-schema/oob-coroutine.err
@@ -0,0 +1,2 @@
+oob-coroutine.json: In command 'oob-command-1':
+oob-coroutine.json:2: flags 'allow-oob' and 'coroutine' are incompatible
diff --git a/tests/qapi-schema/oob-coroutine.json b/tests/qapi-schema/oob-coroutine.json
new file mode 100644
index 0000000000..0f67663bcd
--- /dev/null
+++ b/tests/qapi-schema/oob-coroutine.json
@@ -0,0 +1,2 @@
+# Check that incompatible flags allow-oob and coroutine are rejected
+{ 'command': 'oob-command-1', 'allow-oob': true, 'coroutine': true }
diff --git a/tests/qapi-schema/oob-coroutine.out b/tests/qapi-schema/oob-coroutine.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 6b1f05afa7..3e29fe5b40 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -147,6 +147,7 @@
   'returns': 'UserDefTwo' }
 
 { 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
+{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true }
 
 # Returning a non-dictionary requires a name from the whitelist
 { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 891b4101e0..8868ca0dca 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -203,6 +203,8 @@ command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
     gen=True success_response=True boxed=False oob=False preconfig=False
 command cmd-success-response None -> None
     gen=True success_response=False boxed=False oob=False preconfig=False
+command coroutine-cmd None -> None
+    gen=True success_response=True boxed=False oob=False preconfig=False coroutine=True
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True
-- 
2.25.4

[PATCH v6 08/12] qmp: Move dispatcher to a coroutine

[Kevin Wolf]

This moves the QMP dispatcher to a coroutine and runs all QMP command
handlers that declare 'coroutine': true in coroutine context so they
can avoid blocking the main loop while doing I/O or waiting for other
events.

For commands that are not declared safe to run in a coroutine, the
dispatcher drops out of coroutine context by calling the QMP command
handler from a bottom half.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qapi/qmp/dispatch.h |   1 +
 monitor/monitor-internal.h  |   6 +-
 monitor/monitor.c           |  55 +++++++++++++---
 monitor/qmp.c               | 122 +++++++++++++++++++++++++++---------
 qapi/qmp-dispatch.c         |  52 +++++++++++++--
 qapi/qmp-registry.c         |   3 +
 util/aio-posix.c            |   8 ++-
 7 files changed, 201 insertions(+), 46 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 9fd2b720a7..af8d96c570 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -31,6 +31,7 @@ typedef enum QmpCommandOptions
 typedef struct QmpCommand
 {
     const char *name;
+    /* Runs in coroutine context if QCO_COROUTINE is set */
     QmpCommandFunc *fn;
     QmpCommandOptions options;
     QTAILQ_ENTRY(QmpCommand) node;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index b39e03b744..b55d6df07f 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon)
 
 typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
 extern IOThread *mon_iothread;
-extern QEMUBH *qmp_dispatcher_bh;
+extern Coroutine *qmp_dispatcher_co;
+extern bool qmp_dispatcher_co_shutdown;
+extern bool qmp_dispatcher_co_busy;
 extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
 extern QemuMutex monitor_lock;
 extern MonitorList mon_list;
@@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void);
 
 void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
 void monitor_data_destroy_qmp(MonitorQMP *mon);
-void monitor_qmp_bh_dispatcher(void *data);
+void coroutine_fn monitor_qmp_dispatcher_co(void *data);
 
 int get_monitor_def(int64_t *pval, const char *name);
 void help_cmd(Monitor *mon, const char *name);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 35003bb486..50fb5b20d3 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -55,8 +55,32 @@ typedef struct {
 /* Shared monitor I/O thread */
 IOThread *mon_iothread;
 
-/* Bottom half to dispatch the requests received from I/O thread */
-QEMUBH *qmp_dispatcher_bh;
+/* Coroutine to dispatch the requests received from I/O thread */
+Coroutine *qmp_dispatcher_co;
+
+/* Set to true when the dispatcher coroutine should terminate */
+bool qmp_dispatcher_co_shutdown;
+
+/*
+ * qmp_dispatcher_co_busy is used for synchronisation between the
+ * monitor thread and the main thread to ensure that the dispatcher
+ * coroutine never gets scheduled a second time when it's already
+ * scheduled (scheduling the same coroutine twice is forbidden).
+ *
+ * It is true if the coroutine is active and processing requests.
+ * Additional requests may then be pushed onto a mon->qmp_requests,
+ * and @qmp_dispatcher_co_shutdown may be set without further ado.
+ * @qmp_dispatcher_co_busy must not be woken up in this case.
+ *
+ * If false, you also have to set @qmp_dispatcher_co_busy to true and
+ * wake up @qmp_dispatcher_co after pushing the new requests.
+ *
+ * The coroutine will automatically change this variable back to false
+ * before it yields.  Nobody else may set the variable to false.
+ *
+ * Access must be atomic for thread safety.
+ */
+bool qmp_dispatcher_co_busy;
 
 /*
  * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
@@ -608,9 +632,24 @@ void monitor_cleanup(void)
     }
     qemu_mutex_unlock(&monitor_lock);
 
-    /* QEMUBHs needs to be deleted before destroying the I/O thread */
-    qemu_bh_delete(qmp_dispatcher_bh);
-    qmp_dispatcher_bh = NULL;
+    /*
+     * The dispatcher needs to stop before destroying the I/O thread.
+     *
+     * We need to poll both qemu_aio_context and iohandler_ctx to make
+     * sure that the dispatcher coroutine keeps making progress and
+     * eventually terminates.  qemu_aio_context is automatically
+     * polled by calling AIO_WAIT_WHILE on it, but we must poll
+     * iohandler_ctx manually.
+     */
+    qmp_dispatcher_co_shutdown = true;
+    if (!atomic_xchg(&qmp_dispatcher_co_busy, true)) {
+        aio_co_wake(qmp_dispatcher_co);
+    }
+
+    AIO_WAIT_WHILE(qemu_get_aio_context(),
+                   (aio_poll(iohandler_get_aio_context(), false),
+                    atomic_mb_read(&qmp_dispatcher_co_busy)));
+
     if (mon_iothread) {
         iothread_destroy(mon_iothread);
         mon_iothread = NULL;
@@ -634,9 +673,9 @@ void monitor_init_globals_core(void)
      * have commands assuming that context.  It would be nice to get
      * rid of those assumptions.
      */
-    qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
-                                   monitor_qmp_bh_dispatcher,
-                                   NULL);
+    qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL);
+    atomic_mb_set(&qmp_dispatcher_co_busy, true);
+    aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
 }
 
 int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp)
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 922fdb5541..5a14062a5b 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -133,6 +133,10 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
     }
 }
 
+/*
+ * Runs outside of coroutine context for OOB commands, but in
+ * coroutine context for everything else.
+ */
 static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
 {
     QDict *rsp;
@@ -205,43 +209,99 @@ static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void)
     return req_obj;
 }
 
-void monitor_qmp_bh_dispatcher(void *data)
+void coroutine_fn monitor_qmp_dispatcher_co(void *data)
 {
-    QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
+    QMPRequest *req_obj = NULL;
     QDict *rsp;
     bool need_resume;
     MonitorQMP *mon;
 
-    if (!req_obj) {
-        return;
-    }
+    while (true) {
+        assert(atomic_mb_read(&qmp_dispatcher_co_busy) == true);
 
-    mon = req_obj->mon;
-    /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(mon) ||
-        mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
-    qemu_mutex_unlock(&mon->qmp_queue_lock);
-    if (req_obj->req) {
-        QDict *qdict = qobject_to(QDict, req_obj->req);
-        QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
-        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
-        monitor_qmp_dispatch(mon, req_obj->req);
-    } else {
-        assert(req_obj->err);
-        rsp = qmp_error_response(req_obj->err);
-        req_obj->err = NULL;
-        monitor_qmp_respond(mon, rsp);
-        qobject_unref(rsp);
-    }
+        /*
+         * Mark the dispatcher as not busy already here so that we
+         * don't miss any new requests coming in the middle of our
+         * processing.
+         */
+        atomic_mb_set(&qmp_dispatcher_co_busy, false);
+
+        while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) {
+            /*
+             * No more requests to process.  Wait to be reentered from
+             * handle_qmp_command() when it pushes more requests, or
+             * from monitor_cleanup() when it requests shutdown.
+             */
+            if (!qmp_dispatcher_co_shutdown) {
+                qemu_coroutine_yield();
+
+                /*
+                 * busy must be set to true again by whoever
+                 * rescheduled us to avoid double scheduling
+                 */
+                assert(atomic_xchg(&qmp_dispatcher_co_busy, false) == true);
+            }
+
+            /*
+             * qmp_dispatcher_co_shutdown may have changed if we
+             * yielded and were reentered from monitor_cleanup()
+             */
+            if (qmp_dispatcher_co_shutdown) {
+                return;
+            }
+        }
 
-    if (need_resume) {
-        /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(&mon->common);
-    }
-    qmp_request_free(req_obj);
+        if (atomic_xchg(&qmp_dispatcher_co_busy, true) == true) {
+            /*
+             * Someone rescheduled us (probably because a new requests
+             * came in), but we didn't actually yield. Do that now,
+             * only to be immediately reentered and removed from the
+             * list of scheduled coroutines.
+             */
+            qemu_coroutine_yield();
+        }
 
-    /* Reschedule instead of looping so the main loop stays responsive */
-    qemu_bh_schedule(qmp_dispatcher_bh);
+        /*
+         * Move the coroutine from iohandler_ctx to qemu_aio_context for
+         * executing the command handler so that it can make progress if it
+         * involves an AIO_WAIT_WHILE().
+         */
+        aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co);
+        qemu_coroutine_yield();
+
+        mon = req_obj->mon;
+        /*  qmp_oob_enabled() might change after "qmp_capabilities" */
+        need_resume = !qmp_oob_enabled(mon) ||
+            mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
+        qemu_mutex_unlock(&mon->qmp_queue_lock);
+        if (req_obj->req) {
+            QDict *qdict = qobject_to(QDict, req_obj->req);
+            QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
+            trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
+            monitor_qmp_dispatch(mon, req_obj->req);
+        } else {
+            assert(req_obj->err);
+            rsp = qmp_error_response(req_obj->err);
+            req_obj->err = NULL;
+            monitor_qmp_respond(mon, rsp);
+            qobject_unref(rsp);
+        }
+
+        if (need_resume) {
+            /* Pairs with the monitor_suspend() in handle_qmp_command() */
+            monitor_resume(&mon->common);
+        }
+        qmp_request_free(req_obj);
+
+        /*
+         * Yield and reschedule so the main loop stays responsive.
+         *
+         * Move back to iohandler_ctx so that nested event loops for
+         * qemu_aio_context don't start new monitor commands.
+         */
+        aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
+        qemu_coroutine_yield();
+    }
 }
 
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
@@ -302,7 +362,9 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
     qemu_mutex_unlock(&mon->qmp_queue_lock);
 
     /* Kick the dispatcher routine */
-    qemu_bh_schedule(qmp_dispatcher_bh);
+    if (!atomic_xchg(&qmp_dispatcher_co_busy, true)) {
+        aio_co_wake(qmp_dispatcher_co);
+    }
 }
 
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 5677ba92ca..8ae5e59648 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -12,12 +12,16 @@
  */
 
 #include "qemu/osdep.h"
+
+#include "block/aio.h"
 #include "qapi/error.h"
 #include "qapi/qmp/dispatch.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "sysemu/runstate.h"
 #include "qapi/qmp/qbool.h"
+#include "qemu/coroutine.h"
+#include "qemu/main-loop.h"
 
 static QDict *qmp_dispatch_check_obj(QDict *dict, bool allow_oob,
                                      Error **errp)
@@ -88,6 +92,30 @@ bool qmp_is_oob(const QDict *dict)
         && !qdict_haskey(dict, "execute");
 }
 
+typedef struct QmpDispatchBH {
+    const QmpCommand *cmd;
+    Monitor *cur_mon;
+    QDict *args;
+    QObject **ret;
+    Error **errp;
+    Coroutine *co;
+} QmpDispatchBH;
+
+static void do_qmp_dispatch_bh(void *opaque)
+{
+    QmpDispatchBH *data = opaque;
+
+    assert(monitor_cur() == NULL);
+    monitor_set_cur(qemu_coroutine_self(), data->cur_mon);
+    data->cmd->fn(data->args, data->ret, data->errp);
+    monitor_set_cur(qemu_coroutine_self(), NULL);
+    aio_co_wake(data->co);
+}
+
+/*
+ * Runs outside of coroutine context for OOB commands, but in coroutine context
+ * for everything else.
+ */
 QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
                     bool allow_oob, Monitor *cur_mon)
 {
@@ -153,12 +181,26 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
         qobject_ref(args);
     }
 
+    assert(!(oob && qemu_in_coroutine()));
     assert(monitor_cur() == NULL);
-    monitor_set_cur(qemu_coroutine_self(), cur_mon);
-
-    cmd->fn(args, &ret, &err);
-
-    monitor_set_cur(qemu_coroutine_self(), NULL);
+    if ((cmd->options & QCO_COROUTINE) || !qemu_in_coroutine()) {
+        monitor_set_cur(qemu_coroutine_self(), cur_mon);
+        cmd->fn(args, &ret, &err);
+        monitor_set_cur(qemu_coroutine_self(), NULL);
+    } else {
+        /* Must drop out of coroutine context for this one */
+        QmpDispatchBH data = {
+            .cur_mon    = cur_mon,
+            .cmd        = cmd,
+            .args       = args,
+            .ret        = &ret,
+            .errp       = &err,
+            .co         = qemu_coroutine_self(),
+        };
+        aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh,
+                                &data);
+        qemu_coroutine_yield();
+    }
     qobject_unref(args);
     if (err) {
         /* or assert(!ret) after reviewing all handlers: */
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index d0f9a1d3e3..58c65b5052 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -20,6 +20,9 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
 {
     QmpCommand *cmd = g_malloc0(sizeof(*cmd));
 
+    /* QCO_COROUTINE and QCO_ALLOW_OOB are incompatible for now */
+    assert(!((options & QCO_COROUTINE) && (options & QCO_ALLOW_OOB)));
+
     cmd->name = name;
     cmd->fn = fn;
     cmd->enabled = true;
diff --git a/util/aio-posix.c b/util/aio-posix.c
index 1b2a3af65b..d427908415 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -15,6 +15,7 @@
 
 #include "qemu/osdep.h"
 #include "block/block.h"
+#include "qemu/main-loop.h"
 #include "qemu/rcu.h"
 #include "qemu/rcu_queue.h"
 #include "qemu/sockets.h"
@@ -563,8 +564,13 @@ bool aio_poll(AioContext *ctx, bool blocking)
      * There cannot be two concurrent aio_poll calls for the same AioContext (or
      * an aio_poll concurrent with a GSource prepare/check/dispatch callback).
      * We rely on this below to avoid slow locked accesses to ctx->notify_me.
+     *
+     * aio_poll() may only be called in the AioContext's thread. iohandler_ctx
+     * is special in that it runs in the main thread, but that thread's context
+     * is qemu_aio_context.
      */
-    assert(in_aio_context_home_thread(ctx));
+    assert(in_aio_context_home_thread(ctx == iohandler_get_aio_context() ?
+                                      qemu_get_aio_context() : ctx));
 
     /* aio_notify can avoid the expensive event_notifier_set if
      * everything (file descriptors, bottom halves, timers) will
-- 
2.25.4

Re: [PATCH v6 08/12] qmp: Move dispatcher to a coroutine

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This moves the QMP dispatcher to a coroutine and runs all QMP command
> handlers that declare 'coroutine': true in coroutine context so they
> can avoid blocking the main loop while doing I/O or waiting for other
> events.
>
> For commands that are not declared safe to run in a coroutine, the
> dispatcher drops out of coroutine context by calling the QMP command
> handler from a bottom half.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qapi/qmp/dispatch.h |   1 +
>  monitor/monitor-internal.h  |   6 +-
>  monitor/monitor.c           |  55 +++++++++++++---
>  monitor/qmp.c               | 122 +++++++++++++++++++++++++++---------
>  qapi/qmp-dispatch.c         |  52 +++++++++++++--
>  qapi/qmp-registry.c         |   3 +
>  util/aio-posix.c            |   8 ++-
>  7 files changed, 201 insertions(+), 46 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 9fd2b720a7..af8d96c570 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -31,6 +31,7 @@ typedef enum QmpCommandOptions
>  typedef struct QmpCommand
>  {
>      const char *name;
> +    /* Runs in coroutine context if QCO_COROUTINE is set */
>      QmpCommandFunc *fn;
>      QmpCommandOptions options;
>      QTAILQ_ENTRY(QmpCommand) node;
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index b39e03b744..b55d6df07f 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -155,7 +155,9 @@ static inline bool monitor_is_qmp(const Monitor *mon)
>  
>  typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
>  extern IOThread *mon_iothread;
> -extern QEMUBH *qmp_dispatcher_bh;
> +extern Coroutine *qmp_dispatcher_co;
> +extern bool qmp_dispatcher_co_shutdown;
> +extern bool qmp_dispatcher_co_busy;
>  extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>  extern QemuMutex monitor_lock;
>  extern MonitorList mon_list;
> @@ -173,7 +175,7 @@ void monitor_fdsets_cleanup(void);
>  
>  void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
>  void monitor_data_destroy_qmp(MonitorQMP *mon);
> -void monitor_qmp_bh_dispatcher(void *data);
> +void coroutine_fn monitor_qmp_dispatcher_co(void *data);
>  
>  int get_monitor_def(int64_t *pval, const char *name);
>  void help_cmd(Monitor *mon, const char *name);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 35003bb486..50fb5b20d3 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -55,8 +55,32 @@ typedef struct {
>  /* Shared monitor I/O thread */
>  IOThread *mon_iothread;
>  
> -/* Bottom half to dispatch the requests received from I/O thread */
> -QEMUBH *qmp_dispatcher_bh;
> +/* Coroutine to dispatch the requests received from I/O thread */
> +Coroutine *qmp_dispatcher_co;
> +
> +/* Set to true when the dispatcher coroutine should terminate */
> +bool qmp_dispatcher_co_shutdown;
> +
> +/*
> + * qmp_dispatcher_co_busy is used for synchronisation between the
> + * monitor thread and the main thread to ensure that the dispatcher
> + * coroutine never gets scheduled a second time when it's already
> + * scheduled (scheduling the same coroutine twice is forbidden).
> + *
> + * It is true if the coroutine is active and processing requests.
> + * Additional requests may then be pushed onto a mon->qmp_requests,

s/onto a/onto/

> + * and @qmp_dispatcher_co_shutdown may be set without further ado.
> + * @qmp_dispatcher_co_busy must not be woken up in this case.
> + *
> + * If false, you also have to set @qmp_dispatcher_co_busy to true and
> + * wake up @qmp_dispatcher_co after pushing the new requests.
> + *
> + * The coroutine will automatically change this variable back to false
> + * before it yields.  Nobody else may set the variable to false.
> + *
> + * Access must be atomic for thread safety.
> + */
> +bool qmp_dispatcher_co_busy;
>  
>  /*
>   * Protects mon_list, monitor_qapi_event_state, coroutine_mon,
> @@ -608,9 +632,24 @@ void monitor_cleanup(void)
>      }
>      qemu_mutex_unlock(&monitor_lock);
>  
> -    /* QEMUBHs needs to be deleted before destroying the I/O thread */
> -    qemu_bh_delete(qmp_dispatcher_bh);
> -    qmp_dispatcher_bh = NULL;
> +    /*
> +     * The dispatcher needs to stop before destroying the I/O thread.
> +     *
> +     * We need to poll both qemu_aio_context and iohandler_ctx to make
> +     * sure that the dispatcher coroutine keeps making progress and
> +     * eventually terminates.  qemu_aio_context is automatically
> +     * polled by calling AIO_WAIT_WHILE on it, but we must poll
> +     * iohandler_ctx manually.
> +     */
> +    qmp_dispatcher_co_shutdown = true;
> +    if (!atomic_xchg(&qmp_dispatcher_co_busy, true)) {
> +        aio_co_wake(qmp_dispatcher_co);
> +    }
> +
> +    AIO_WAIT_WHILE(qemu_get_aio_context(),
> +                   (aio_poll(iohandler_get_aio_context(), false),
> +                    atomic_mb_read(&qmp_dispatcher_co_busy)));
> +
>      if (mon_iothread) {
>          iothread_destroy(mon_iothread);
>          mon_iothread = NULL;
> @@ -634,9 +673,9 @@ void monitor_init_globals_core(void)
>       * have commands assuming that context.  It would be nice to get
>       * rid of those assumptions.
>       */
> -    qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
> -                                   monitor_qmp_bh_dispatcher,
> -                                   NULL);
> +    qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL);
> +    atomic_mb_set(&qmp_dispatcher_co_busy, true);
> +    aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
>  }
>  
>  int monitor_init(MonitorOptions *opts, bool allow_hmp, Error **errp)
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 922fdb5541..5a14062a5b 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -133,6 +133,10 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
>      }
>  }
>  
> +/*
> + * Runs outside of coroutine context for OOB commands, but in
> + * coroutine context for everything else.
> + */
>  static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>  {
>      QDict *rsp;
> @@ -205,43 +209,99 @@ static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void)
>      return req_obj;
>  }
>  
> -void monitor_qmp_bh_dispatcher(void *data)
> +void coroutine_fn monitor_qmp_dispatcher_co(void *data)
>  {
> -    QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
> +    QMPRequest *req_obj = NULL;
>      QDict *rsp;
>      bool need_resume;
>      MonitorQMP *mon;
>  
> -    if (!req_obj) {
> -        return;
> -    }
> +    while (true) {
> +        assert(atomic_mb_read(&qmp_dispatcher_co_busy) == true);
>  
> -    mon = req_obj->mon;
> -    /*  qmp_oob_enabled() might change after "qmp_capabilities" */
> -    need_resume = !qmp_oob_enabled(mon) ||
> -        mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> -    qemu_mutex_unlock(&mon->qmp_queue_lock);
> -    if (req_obj->req) {
> -        QDict *qdict = qobject_to(QDict, req_obj->req);
> -        QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
> -        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
> -        monitor_qmp_dispatch(mon, req_obj->req);
> -    } else {
> -        assert(req_obj->err);
> -        rsp = qmp_error_response(req_obj->err);
> -        req_obj->err = NULL;
> -        monitor_qmp_respond(mon, rsp);
> -        qobject_unref(rsp);
> -    }
> +        /*
> +         * Mark the dispatcher as not busy already here so that we
> +         * don't miss any new requests coming in the middle of our
> +         * processing.
> +         */
> +        atomic_mb_set(&qmp_dispatcher_co_busy, false);
> +
> +        while (!(req_obj = monitor_qmp_requests_pop_any_with_lock())) {
> +            /*
> +             * No more requests to process.  Wait to be reentered from
> +             * handle_qmp_command() when it pushes more requests, or
> +             * from monitor_cleanup() when it requests shutdown.
> +             */
> +            if (!qmp_dispatcher_co_shutdown) {
> +                qemu_coroutine_yield();
> +
> +                /*
> +                 * busy must be set to true again by whoever
> +                 * rescheduled us to avoid double scheduling
> +                 */
> +                assert(atomic_xchg(&qmp_dispatcher_co_busy, false) == true);
> +            }
> +
> +            /*
> +             * qmp_dispatcher_co_shutdown may have changed if we
> +             * yielded and were reentered from monitor_cleanup()
> +             */
> +            if (qmp_dispatcher_co_shutdown) {
> +                return;
> +            }
> +        }
>  
> -    if (need_resume) {
> -        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> -        monitor_resume(&mon->common);
> -    }
> -    qmp_request_free(req_obj);
> +        if (atomic_xchg(&qmp_dispatcher_co_busy, true) == true) {
> +            /*
> +             * Someone rescheduled us (probably because a new requests
> +             * came in), but we didn't actually yield. Do that now,
> +             * only to be immediately reentered and removed from the
> +             * list of scheduled coroutines.
> +             */
> +            qemu_coroutine_yield();
> +        }
>  
> -    /* Reschedule instead of looping so the main loop stays responsive */
> -    qemu_bh_schedule(qmp_dispatcher_bh);
> +        /*
> +         * Move the coroutine from iohandler_ctx to qemu_aio_context for
> +         * executing the command handler so that it can make progress if it
> +         * involves an AIO_WAIT_WHILE().
> +         */
> +        aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co);
> +        qemu_coroutine_yield();
> +
> +        mon = req_obj->mon;
> +        /*  qmp_oob_enabled() might change after "qmp_capabilities" */

Extra space after /*

> +        need_resume = !qmp_oob_enabled(mon) ||
> +            mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> +        qemu_mutex_unlock(&mon->qmp_queue_lock);
> +        if (req_obj->req) {
> +            QDict *qdict = qobject_to(QDict, req_obj->req);
> +            QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
> +            trace_monitor_qmp_cmd_in_band(qobject_get_try_str(id) ?: "");
> +            monitor_qmp_dispatch(mon, req_obj->req);
> +        } else {
> +            assert(req_obj->err);
> +            rsp = qmp_error_response(req_obj->err);
> +            req_obj->err = NULL;
> +            monitor_qmp_respond(mon, rsp);
> +            qobject_unref(rsp);
> +        }
> +
> +        if (need_resume) {
> +            /* Pairs with the monitor_suspend() in handle_qmp_command() */
> +            monitor_resume(&mon->common);
> +        }
> +        qmp_request_free(req_obj);
> +
> +        /*
> +         * Yield and reschedule so the main loop stays responsive.
> +         *
> +         * Move back to iohandler_ctx so that nested event loops for
> +         * qemu_aio_context don't start new monitor commands.
> +         */
> +        aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
> +        qemu_coroutine_yield();
> +    }
>  }
>  
>  static void handle_qmp_command(void *opaque, QObject *req, Error *err)
> @@ -302,7 +362,9 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
>      qemu_mutex_unlock(&mon->qmp_queue_lock);
>  
>      /* Kick the dispatcher routine */
> -    qemu_bh_schedule(qmp_dispatcher_bh);
> +    if (!atomic_xchg(&qmp_dispatcher_co_busy, true)) {
> +        aio_co_wake(qmp_dispatcher_co);
> +    }
>  }
>  
>  static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 5677ba92ca..8ae5e59648 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -12,12 +12,16 @@
>   */
>  
>  #include "qemu/osdep.h"
> +
> +#include "block/aio.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/dispatch.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qjson.h"
>  #include "sysemu/runstate.h"
>  #include "qapi/qmp/qbool.h"
> +#include "qemu/coroutine.h"
> +#include "qemu/main-loop.h"
>  
>  static QDict *qmp_dispatch_check_obj(QDict *dict, bool allow_oob,
>                                       Error **errp)
> @@ -88,6 +92,30 @@ bool qmp_is_oob(const QDict *dict)
>          && !qdict_haskey(dict, "execute");
>  }
>  
> +typedef struct QmpDispatchBH {
> +    const QmpCommand *cmd;
> +    Monitor *cur_mon;
> +    QDict *args;
> +    QObject **ret;
> +    Error **errp;
> +    Coroutine *co;
> +} QmpDispatchBH;
> +
> +static void do_qmp_dispatch_bh(void *opaque)
> +{
> +    QmpDispatchBH *data = opaque;
> +
> +    assert(monitor_cur() == NULL);
> +    monitor_set_cur(qemu_coroutine_self(), data->cur_mon);
> +    data->cmd->fn(data->args, data->ret, data->errp);
> +    monitor_set_cur(qemu_coroutine_self(), NULL);
> +    aio_co_wake(data->co);
> +}
> +
> +/*
> + * Runs outside of coroutine context for OOB commands, but in coroutine context

Long line.

> + * for everything else.
> + */
>  QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
>                      bool allow_oob, Monitor *cur_mon)
>  {
> @@ -153,12 +181,26 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
>          qobject_ref(args);
>      }
>  
> +    assert(!(oob && qemu_in_coroutine()));
>      assert(monitor_cur() == NULL);
> -    monitor_set_cur(qemu_coroutine_self(), cur_mon);
> -
> -    cmd->fn(args, &ret, &err);
> -
> -    monitor_set_cur(qemu_coroutine_self(), NULL);
> +    if ((cmd->options & QCO_COROUTINE) || !qemu_in_coroutine()) {
> +        monitor_set_cur(qemu_coroutine_self(), cur_mon);
> +        cmd->fn(args, &ret, &err);
> +        monitor_set_cur(qemu_coroutine_self(), NULL);
> +    } else {
> +        /* Must drop out of coroutine context for this one */
> +        QmpDispatchBH data = {
> +            .cur_mon    = cur_mon,
> +            .cmd        = cmd,
> +            .args       = args,
> +            .ret        = &ret,
> +            .errp       = &err,
> +            .co         = qemu_coroutine_self(),
> +        };
> +        aio_bh_schedule_oneshot(qemu_get_aio_context(), do_qmp_dispatch_bh,
> +                                &data);
> +        qemu_coroutine_yield();
> +    }

Hmm, how does this conditional work?  Command wants coroutine × running
in coroutine (which is the same as command is OOB) -> four cases.

Not OOB:

* (true, true): call right away

* (false, true): drop out of coroutine context

OOB:

* (false, false): call right away

* (true, false): would have to enter a coroutine, but the previous patch
  outlawed this case.

The conditional does the right thing for the first three.  For the last
one, it doesn't, but it doesn't matter, because the case is impossible.

Okay.

Would the following be easier to understand?

       if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) {
           call right away...
       } else {
           /*
            * The case "@cmd wants a coroutine, but we're not running in
            * coroutine context" is impossible, because the latter
            * implies oob, and OOB commands must not want a coroutine.
            */
           assert(!(cmd->options & QCO_COROUTINE));
           drop out...
       }

>      qobject_unref(args);
>      if (err) {
>          /* or assert(!ret) after reviewing all handlers: */
> diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
> index d0f9a1d3e3..58c65b5052 100644
> --- a/qapi/qmp-registry.c
> +++ b/qapi/qmp-registry.c
> @@ -20,6 +20,9 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
>  {
>      QmpCommand *cmd = g_malloc0(sizeof(*cmd));
>  
> +    /* QCO_COROUTINE and QCO_ALLOW_OOB are incompatible for now */
> +    assert(!((options & QCO_COROUTINE) && (options & QCO_ALLOW_OOB)));
> +
>      cmd->name = name;
>      cmd->fn = fn;
>      cmd->enabled = true;
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index 1b2a3af65b..d427908415 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -15,6 +15,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "block/block.h"
> +#include "qemu/main-loop.h"
>  #include "qemu/rcu.h"
>  #include "qemu/rcu_queue.h"
>  #include "qemu/sockets.h"
> @@ -563,8 +564,13 @@ bool aio_poll(AioContext *ctx, bool blocking)
>       * There cannot be two concurrent aio_poll calls for the same AioContext (or
>       * an aio_poll concurrent with a GSource prepare/check/dispatch callback).
>       * We rely on this below to avoid slow locked accesses to ctx->notify_me.
> +     *
> +     * aio_poll() may only be called in the AioContext's thread. iohandler_ctx
> +     * is special in that it runs in the main thread, but that thread's context
> +     * is qemu_aio_context.
>       */
> -    assert(in_aio_context_home_thread(ctx));
> +    assert(in_aio_context_home_thread(ctx == iohandler_get_aio_context() ?
> +                                      qemu_get_aio_context() : ctx));
>  
>      /* aio_notify can avoid the expensive event_notifier_set if
>       * everything (file descriptors, bottom halves, timers) will

The code is hairy, but the comments help.  At least they helped when I
grappled with the code for my review of v5; I didn't re-grapple with the
parts that haven't changed since.  I can't see how to make the code less
hairy.

With the typos fixed, the long line wrapped, and with or without my
suggestion on qmp_dispatch():
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v6 09/12] hmp: Add support for coroutine command handlers

[Kevin Wolf]

Often, QMP command handlers are not only called to handle QMP commands,
but also from a corresponding HMP command handler. In order to give them
a consistent environment, optionally run HMP command handlers in a
coroutine, too.

The implementation is a lot simpler than in QMP because for HMP, we
still block the VM while the coroutine is running.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 monitor/monitor-internal.h |  1 +
 monitor/hmp.c              | 38 ++++++++++++++++++++++++++++++++------
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index b55d6df07f..ad2e64be13 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -74,6 +74,7 @@ typedef struct HMPCommand {
     const char *help;
     const char *flags; /* p=preconfig */
     void (*cmd)(Monitor *mon, const QDict *qdict);
+    bool coroutine;
     /*
      * @sub_table is a list of 2nd level of commands. If it does not exist,
      * cmd should be used. If it exists, sub_table[?].cmd should be
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 3e73a4c3ce..ab0e3e279f 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1056,12 +1056,25 @@ fail:
     return NULL;
 }
 
+typedef struct HandleHmpCommandCo {
+    Monitor *mon;
+    const HMPCommand *cmd;
+    QDict *qdict;
+    bool done;
+} HandleHmpCommandCo;
+
+static void handle_hmp_command_co(void *opaque)
+{
+    HandleHmpCommandCo *data = opaque;
+    data->cmd->cmd(data->mon, data->qdict);
+    data->done = true;
+}
+
 void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
 {
     QDict *qdict;
     const HMPCommand *cmd;
     const char *cmd_start = cmdline;
-    Monitor *old_mon;
 
     trace_handle_hmp_command(mon, cmdline);
 
@@ -1080,11 +1093,24 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
         return;
     }
 
-    /* old_mon is non-NULL when called from qmp_human_monitor_command() */
-    old_mon = monitor_cur();
-    monitor_set_cur(qemu_coroutine_self(), &mon->common);
-    cmd->cmd(&mon->common, qdict);
-    monitor_set_cur(qemu_coroutine_self(), old_mon);
+    if (!cmd->coroutine) {
+        /* old_mon is non-NULL when called from qmp_human_monitor_command() */
+        Monitor *old_mon = monitor_cur();
+        monitor_set_cur(qemu_coroutine_self(), &mon->common);
+        cmd->cmd(&mon->common, qdict);
+        monitor_set_cur(qemu_coroutine_self(), old_mon);
+    } else {
+        HandleHmpCommandCo data = {
+            .mon = &mon->common,
+            .cmd = cmd,
+            .qdict = qdict,
+            .done = false,
+        };
+        Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
+        monitor_set_cur(co, &mon->common);
+        aio_co_enter(qemu_get_aio_context(), co);
+        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
+    }
 
     qobject_unref(qdict);
 }
-- 
2.25.4

Re: [PATCH v6 09/12] hmp: Add support for coroutine command handlers

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Often, QMP command handlers are not only called to handle QMP commands,
> but also from a corresponding HMP command handler. In order to give them
> a consistent environment, optionally run HMP command handlers in a
> coroutine, too.
>
> The implementation is a lot simpler than in QMP because for HMP, we
> still block the VM while the coroutine is running.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  monitor/monitor-internal.h |  1 +
>  monitor/hmp.c              | 38 ++++++++++++++++++++++++++++++++------
>  2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index b55d6df07f..ad2e64be13 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -74,6 +74,7 @@ typedef struct HMPCommand {
>      const char *help;
>      const char *flags; /* p=preconfig */
>      void (*cmd)(Monitor *mon, const QDict *qdict);
> +    bool coroutine;
>      /*
>       * @sub_table is a list of 2nd level of commands. If it does not exist,
>       * cmd should be used. If it exists, sub_table[?].cmd should be
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 3e73a4c3ce..ab0e3e279f 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1056,12 +1056,25 @@ fail:
>      return NULL;
>  }
>  
> +typedef struct HandleHmpCommandCo {
> +    Monitor *mon;
> +    const HMPCommand *cmd;
> +    QDict *qdict;
> +    bool done;
> +} HandleHmpCommandCo;
> +
> +static void handle_hmp_command_co(void *opaque)
> +{
> +    HandleHmpCommandCo *data = opaque;
> +    data->cmd->cmd(data->mon, data->qdict);
> +    data->done = true;
> +}
> +
>  void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>  {
>      QDict *qdict;
>      const HMPCommand *cmd;
>      const char *cmd_start = cmdline;
> -    Monitor *old_mon;
>  
>      trace_handle_hmp_command(mon, cmdline);
>  
> @@ -1080,11 +1093,24 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>          return;
>      }
>  
> -    /* old_mon is non-NULL when called from qmp_human_monitor_command() */
> -    old_mon = monitor_cur();
> -    monitor_set_cur(qemu_coroutine_self(), &mon->common);
> -    cmd->cmd(&mon->common, qdict);
> -    monitor_set_cur(qemu_coroutine_self(), old_mon);
> +    if (!cmd->coroutine) {
> +        /* old_mon is non-NULL when called from qmp_human_monitor_command() */
> +        Monitor *old_mon = monitor_cur();
> +        monitor_set_cur(qemu_coroutine_self(), &mon->common);
> +        cmd->cmd(&mon->common, qdict);
> +        monitor_set_cur(qemu_coroutine_self(), old_mon);
> +    } else {
> +        HandleHmpCommandCo data = {
> +            .mon = &mon->common,
> +            .cmd = cmd,
> +            .qdict = qdict,
> +            .done = false,
> +        };
> +        Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
> +        monitor_set_cur(co, &mon->common);

Where is the matching monitor_set_cur(co, NULL)?

> +        aio_co_enter(qemu_get_aio_context(), co);
> +        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
> +    }
>  
>      qobject_unref(qdict);
>  }

[PATCH v6 10/12] util/async: Add aio_co_reschedule_self()

[Kevin Wolf]

Add a function that can be used to move the currently running coroutine
to a different AioContext (and therefore potentially a different
thread).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/aio.h | 10 ++++++++++
 util/async.c        | 30 ++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/block/aio.h b/include/block/aio.h
index b2f703fa3f..c37617b404 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -17,6 +17,7 @@
 #ifdef CONFIG_LINUX_IO_URING
 #include <liburing.h>
 #endif
+#include "qemu/coroutine.h"
 #include "qemu/queue.h"
 #include "qemu/event_notifier.h"
 #include "qemu/thread.h"
@@ -654,6 +655,15 @@ static inline bool aio_node_check(AioContext *ctx, bool is_external)
  */
 void aio_co_schedule(AioContext *ctx, struct Coroutine *co);
 
+/**
+ * aio_co_reschedule_self:
+ * @new_ctx: the new context
+ *
+ * Move the currently running coroutine to new_ctx. If the coroutine is already
+ * running in new_ctx, do nothing.
+ */
+void coroutine_fn aio_co_reschedule_self(AioContext *new_ctx);
+
 /**
  * aio_co_wake:
  * @co: the coroutine
diff --git a/util/async.c b/util/async.c
index 1319eee3bc..b68e73f488 100644
--- a/util/async.c
+++ b/util/async.c
@@ -559,6 +559,36 @@ void aio_co_schedule(AioContext *ctx, Coroutine *co)
     aio_context_unref(ctx);
 }
 
+typedef struct AioCoRescheduleSelf {
+    Coroutine *co;
+    AioContext *new_ctx;
+} AioCoRescheduleSelf;
+
+static void aio_co_reschedule_self_bh(void *opaque)
+{
+    AioCoRescheduleSelf *data = opaque;
+    aio_co_schedule(data->new_ctx, data->co);
+}
+
+void coroutine_fn aio_co_reschedule_self(AioContext *new_ctx)
+{
+    AioContext *old_ctx = qemu_get_current_aio_context();
+
+    if (old_ctx != new_ctx) {
+        AioCoRescheduleSelf data = {
+            .co = qemu_coroutine_self(),
+            .new_ctx = new_ctx,
+        };
+        /*
+         * We can't directly schedule the coroutine in the target context
+         * because this would be racy: The other thread could try to enter the
+         * coroutine before it has yielded in this one.
+         */
+        aio_bh_schedule_oneshot(old_ctx, aio_co_reschedule_self_bh, &data);
+        qemu_coroutine_yield();
+    }
+}
+
 void aio_co_wake(struct Coroutine *co)
 {
     AioContext *ctx;
-- 
2.25.4

[PATCH v6 11/12] block: Add bdrv_co_move_to_aio_context()

[Kevin Wolf]

Add a function to move the current coroutine to the AioContext of a
given BlockDriverState.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h |  6 ++++++
 block.c               | 10 ++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 25e299605e..fb77062a1e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -631,6 +631,12 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag);
  */
 AioContext *bdrv_get_aio_context(BlockDriverState *bs);
 
+/**
+ * Move the current coroutine to the AioContext of @bs and return the old
+ * AioContext of the coroutine.
+ */
+AioContext *coroutine_fn bdrv_co_move_to_aio_context(BlockDriverState *bs);
+
 /**
  * Transfer control to @co in the aio context of @bs
  */
diff --git a/block.c b/block.c
index 8416376c9b..cd576b991c 100644
--- a/block.c
+++ b/block.c
@@ -6215,6 +6215,16 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs)
     return bs ? bs->aio_context : qemu_get_aio_context();
 }
 
+AioContext *coroutine_fn bdrv_co_move_to_aio_context(BlockDriverState *bs)
+{
+    Coroutine *self = qemu_coroutine_self();
+    AioContext *old_ctx = qemu_coroutine_get_aio_context(self);
+    AioContext *new_ctx = bdrv_get_aio_context(bs);
+
+    aio_co_reschedule_self(new_ctx);
+    return old_ctx;
+}
+
 void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co)
 {
     aio_co_enter(bdrv_get_aio_context(bs), co);
-- 
2.25.4

[PATCH v6 12/12] block: Convert 'block_resize' to coroutine

[Kevin Wolf]

block_resize performs some I/O that could potentially take quite some
time, so use it as an example for the new 'coroutine': true annotation
in the QAPI schema.

bdrv_truncate() requires that we're already in the right AioContext for
the BlockDriverState if called in coroutine context. So instead of just
taking the AioContext lock, move the QMP handler coroutine to the
context.

Call blk_unref() only after switching back because blk_unref() may only
be called in the main thread.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json |  3 ++-
 blockdev.c           | 13 ++++++-------
 hmp-commands.hx      |  1 +
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6fbacddab2..a6003d8a92 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1291,7 +1291,8 @@
 { 'command': 'block_resize',
   'data': { '*device': 'str',
             '*node-name': 'str',
-            'size': 'int' } }
+            'size': 'int' },
+  'coroutine': true }
 
 ##
 # @NewImageMode:
diff --git a/blockdev.c b/blockdev.c
index 72df193ca7..9b9287b5d6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2410,14 +2410,14 @@ BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
     return ret;
 }
 
-void qmp_block_resize(bool has_device, const char *device,
-                      bool has_node_name, const char *node_name,
-                      int64_t size, Error **errp)
+void coroutine_fn qmp_block_resize(bool has_device, const char *device,
+                                   bool has_node_name, const char *node_name,
+                                   int64_t size, Error **errp)
 {
     Error *local_err = NULL;
     BlockBackend *blk = NULL;
     BlockDriverState *bs;
-    AioContext *aio_context;
+    AioContext *old_ctx;
 
     bs = bdrv_lookup_bs(has_device ? device : NULL,
                         has_node_name ? node_name : NULL,
@@ -2427,8 +2427,7 @@ void qmp_block_resize(bool has_device, const char *device,
         return;
     }
 
-    aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(aio_context);
+    old_ctx = bdrv_co_move_to_aio_context(bs);
 
     if (size < 0) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 size");
@@ -2450,8 +2449,8 @@ void qmp_block_resize(bool has_device, const char *device,
     bdrv_drained_end(bs);
 
 out:
+    aio_co_reschedule_self(old_ctx);
     blk_unref(blk);
-    aio_context_release(aio_context);
 }
 
 void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 7f0f3974ad..4e56fe08ca 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -76,6 +76,7 @@ ERST
         .params     = "device size",
         .help       = "resize a block image",
         .cmd        = hmp_block_resize,
+        .coroutine  = true,
     },
 
 SRST
-- 
2.25.4

Re: [PATCH v6 00/12] monitor: Optionally run handlers in coroutines

[Markus Armbruster]

I let this series slide to get my Error API rework done, along with much
else.  My sincere apologies!

Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
is first.

Re: [PATCH v6 00/12] monitor: Optionally run handlers in coroutines

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> I let this series slide to get my Error API rework done, along with much
> else.  My sincere apologies!
>
> Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
> is first.

I'm done with v6.  Summary:

* A few trivial things to correct here and there.

* A few ideas to improve things in relatively minor ways.

* PATCH 03 looks "why bother" to me until PATCH 09 makes me suspect you
  did the former to enable the latter.  If you had captured that in your
  commit message back then, like you did for the similar PATCH 05, I
  wouldn't be scratching my head now :)

* I dislike PATCH 06, and would like to explore an alternative idea.

* PATCH 08 makes hairy monitor code even hairier, but I don't have
  better ideas.

* I don't feel comfortable as a sole reviewer of the AIO magic in PATCH
  10-12.  Let's ask Stefan for an eye-over.

I'd like to proceed as follows.  You rebase, and address "easy" review
comments (you decide what's easy).  Post as v7, cc'ing Stefan for the
AIO magic and David Gilbert for HMP.  While they review (hopefully), I
explore a replacement for PATCH 06.  And then we touch bases and decide
how to get this thing wrapped.

Okay?

Re: [PATCH v6 00/12] monitor: Optionally run handlers in coroutines

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> I let this series slide to get my Error API rework done, along with much
>> else.  My sincere apologies!
>>
>> Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
>> is first.
>
> I'm done with v6.  Summary:
>
> * A few trivial things to correct here and there.
>
> * A few ideas to improve things in relatively minor ways.
>
> * PATCH 03 looks "why bother" to me until PATCH 09 makes me suspect you
>   did the former to enable the latter.  If you had captured that in your
>   commit message back then, like you did for the similar PATCH 05, I
>   wouldn't be scratching my head now :)
>
> * I dislike PATCH 06, and would like to explore an alternative idea.
>
> * PATCH 08 makes hairy monitor code even hairier, but I don't have
>   better ideas.
>
> * I don't feel comfortable as a sole reviewer of the AIO magic in PATCH
>   10-12.  Let's ask Stefan for an eye-over.
>
> I'd like to proceed as follows.  You rebase, and address "easy" review
> comments (you decide what's easy).  Post as v7, cc'ing Stefan for the
> AIO magic and David Gilbert for HMP.  While they review (hopefully), I
> explore a replacement for PATCH 06.  And then we touch bases and decide
> how to get this thing wrapped.

I explored:

    Subject: Ways to do per-coroutine properties (was: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property)
    Date: Fri, 07 Aug 2020 15:09:19 +0200 (3 weeks, 5 days, 21 hours ago)
    Message-ID: <87a6z6wqkg.fsf_-_@dusky.pond.sub.org>

May I have v7?  Feel free to keep your PATCH 06.  If I decide to replace
it, I can do it myself, possibly on top.

Re: [PATCH v6 00/12] monitor: Optionally run handlers in coroutines

[Kevin Wolf]

Am 03.09.2020 um 12:49 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> I let this series slide to get my Error API rework done, along with much
> >> else.  My sincere apologies!
> >>
> >> Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
> >> is first.
> >
> > I'm done with v6.  Summary:
> >
> > * A few trivial things to correct here and there.
> >
> > * A few ideas to improve things in relatively minor ways.
> >
> > * PATCH 03 looks "why bother" to me until PATCH 09 makes me suspect you
> >   did the former to enable the latter.  If you had captured that in your
> >   commit message back then, like you did for the similar PATCH 05, I
> >   wouldn't be scratching my head now :)
> >
> > * I dislike PATCH 06, and would like to explore an alternative idea.
> >
> > * PATCH 08 makes hairy monitor code even hairier, but I don't have
> >   better ideas.
> >
> > * I don't feel comfortable as a sole reviewer of the AIO magic in PATCH
> >   10-12.  Let's ask Stefan for an eye-over.
> >
> > I'd like to proceed as follows.  You rebase, and address "easy" review
> > comments (you decide what's easy).  Post as v7, cc'ing Stefan for the
> > AIO magic and David Gilbert for HMP.  While they review (hopefully), I
> > explore a replacement for PATCH 06.  And then we touch bases and decide
> > how to get this thing wrapped.
> 
> I explored:
> 
>     Subject: Ways to do per-coroutine properties (was: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property)
>     Date: Fri, 07 Aug 2020 15:09:19 +0200 (3 weeks, 5 days, 21 hours ago)
>     Message-ID: <87a6z6wqkg.fsf_-_@dusky.pond.sub.org>
> 
> May I have v7?  Feel free to keep your PATCH 06.  If I decide to replace
> it, I can do it myself, possibly on top.

It's one of the next things on my list. I can't promise anything more
specific, though.

Kevin

Re: [PATCH v6 00/12] monitor: Optionally run handlers in coroutines

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 03.09.2020 um 12:49 hat Markus Armbruster geschrieben:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Markus Armbruster <armbru@redhat.com> writes:
>> >
>> >> I let this series slide to get my Error API rework done, along with much
>> >> else.  My sincere apologies!
>> >>
>> >> Unsurprisingly, it needs a rebase now.  I suggest to let me review it as
>> >> is first.
>> >
>> > I'm done with v6.  Summary:
>> >
>> > * A few trivial things to correct here and there.
>> >
>> > * A few ideas to improve things in relatively minor ways.
>> >
>> > * PATCH 03 looks "why bother" to me until PATCH 09 makes me suspect you
>> >   did the former to enable the latter.  If you had captured that in your
>> >   commit message back then, like you did for the similar PATCH 05, I
>> >   wouldn't be scratching my head now :)
>> >
>> > * I dislike PATCH 06, and would like to explore an alternative idea.
>> >
>> > * PATCH 08 makes hairy monitor code even hairier, but I don't have
>> >   better ideas.
>> >
>> > * I don't feel comfortable as a sole reviewer of the AIO magic in PATCH
>> >   10-12.  Let's ask Stefan for an eye-over.
>> >
>> > I'd like to proceed as follows.  You rebase, and address "easy" review
>> > comments (you decide what's easy).  Post as v7, cc'ing Stefan for the
>> > AIO magic and David Gilbert for HMP.  While they review (hopefully), I
>> > explore a replacement for PATCH 06.  And then we touch bases and decide
>> > how to get this thing wrapped.
>> 
>> I explored:
>> 
>>     Subject: Ways to do per-coroutine properties (was: [PATCH v6 06/12] monitor: Make current monitor a per-coroutine property)
>>     Date: Fri, 07 Aug 2020 15:09:19 +0200 (3 weeks, 5 days, 21 hours ago)
>>     Message-ID: <87a6z6wqkg.fsf_-_@dusky.pond.sub.org>
>> 
>> May I have v7?  Feel free to keep your PATCH 06.  If I decide to replace
>> it, I can do it myself, possibly on top.
>
> It's one of the next things on my list. I can't promise anything more
> specific, though.

Take your time.  I've certainly taken mine and then some.