Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

2020-03-30 Thread Markus Armbruster
Marc-André Lureau  writes:

> Hi
>
> On Mon, Mar 23, 2020 at 6:41 PM Kevin Wolf  wrote:
>>
>> Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben:
>> > Kevin Wolf  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 
>> >
>> > Uh, what about @cur_mon?
>> >
>> > @cur_mon points to the current monitor while a command executes.
>> > Initial value is null.  It is set in three places (not counting unit
>> > tests), and all three save, set, do something that may use @cur_mon,
>> > restore:
>> >
>> > * monitor_qmp_dispatch(), for use within qmp_dispatch()
>> > * monitor_read(), for use within handle_hmp_command()
>> > * qmp_human_monitor_command(), also for use within handle_hmp_command()
>> >
>> > Therefore, @cur_mon is null unless we're running within qmp_dispatch()
>> > or handle_hmp_command().
>>
>> Can we make it NULL for coroutine-enabled handlers?
>
> fwiw, I think /dev/fdset doesn't care about cur_mon. However, qmp
> handlers that use monitor_get_fd() usually depend on cur_mon.
>
> Note: I wonder if add-fd (fdsets) and getfd (monitor fds) deserve to co-exist.

Beats me.

If one of them is more general, we could consider deprecating the other
one.




Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

2020-03-30 Thread Markus Armbruster
Kevin Wolf  writes:

> Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben:
>> Kevin Wolf  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 
>> 
>> Uh, what about @cur_mon?
>> 
>> @cur_mon points to the current monitor while a command executes.
>> Initial value is null.  It is set in three places (not counting unit
>> tests), and all three save, set, do something that may use @cur_mon,
>> restore:
>> 
>> * monitor_qmp_dispatch(), for use within qmp_dispatch()
>> * monitor_read(), for use within handle_hmp_command()
>> * qmp_human_monitor_command(), also for use within handle_hmp_command()
>> 
>> Therefore, @cur_mon is null unless we're running within qmp_dispatch()
>> or handle_hmp_command().
>
> Can we make it NULL for coroutine-enabled handlers?

Sets up and arms a bear trap, I'm afraid.

We can rely on code review to ensure a handler we want to
coroutine-enable does not access @cur_mon.  But how can we ensure no
uses creep back?  Not even on error paths, where testing tends to be
ineffective?

@cur_mon goes back to Jan's commit 376253ece48 (March 2009).  Meant to
be merely a stop gap:

For the case that monitor outputs so far happen without clearly
identifiable context, the global variable cur_mon is introduced that
shall once provide a pointer either to the current active monitor (while
processing commands) or to the default one. On the mid or long term,
those use case will be obsoleted so that this variable can be removed
again.

It's been eleven years, and we haven't really gotten closer to getting
them "obsoleted".  I can't stop you if you want to try.  Myself, I'd
rather give up and eliminate the Monitor * parameter passing.

>> Example of use: error_report() & friends print "to current monitor if we
>> have one, else to stderr."  Makes sharing code between HMP and CLI
>> easier.  Uses @cur_mon under the hood.
>
> error_report() eventually prints to stderr both for cur_mon == NULL and
> for QMP monitors. Is there an important difference between both cases?

This goes back to

commit 4ad417baa43424b6b988c52b83989fd95670c113
Author: Cole Robinson 
Date:   Fri Mar 21 19:42:24 2014 -0400

error: Print error_report() to stderr if using qmp

monitor_printf will drop the requested output if cur_mon is qmp (for
good reason). However these messages are often helpful for debugging
issues with via libvirt.

If we know the message won't hit the monitor, send it to stderr.

Cc: Luiz Capitulino 
Cc: Markus Armbruster 
Signed-off-by: Cole Robinson 
Reviewed-by: Paolo Bonzini 
Signed-off-by: Luiz Capitulino 

Use of error_report() in QMP context is wrong more often than not.
Still, spewing the message to stderr is less bad than throwing it away.

> There is error_vprintf_unless_qmp(), which behaves differently for both
> cases. However, it is only used in VNC code, so that code would have to
> keep coroutines disabled.

It has seen other users over the years.

error_printf_unless_qmp() is a convenient way to print hints for humans.
In HMP context, you reach the human by printing to @cur_mon.  Outside
monitor context, you reach the human by printing to stdout or stderr.
In QMP context, we're talking to a machine.

> Is cur_mon used much in other functions called by QMP handlers?

Marc-André pointed to the file-descriptor passing stuff.

To be sure that's all, we'd have to review all uses of @cur_mon.

>> @cur_mon is thread-local.
>> 
>> I'm afraid we have to save, clear and restore @cur_mon around a yield.
>
> That sounds painful enough that I'd rather avoid it.

I've seen coroutine implementations for C that provide coroutine-local
variables of sorts by managing a thread-local opaque pointer.




Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

2020-03-23 Thread Marc-André Lureau
Hi

On Mon, Mar 23, 2020 at 6:41 PM Kevin Wolf  wrote:
>
> Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben:
> > Kevin Wolf  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 
> >
> > Uh, what about @cur_mon?
> >
> > @cur_mon points to the current monitor while a command executes.
> > Initial value is null.  It is set in three places (not counting unit
> > tests), and all three save, set, do something that may use @cur_mon,
> > restore:
> >
> > * monitor_qmp_dispatch(), for use within qmp_dispatch()
> > * monitor_read(), for use within handle_hmp_command()
> > * qmp_human_monitor_command(), also for use within handle_hmp_command()
> >
> > Therefore, @cur_mon is null unless we're running within qmp_dispatch()
> > or handle_hmp_command().
>
> Can we make it NULL for coroutine-enabled handlers?

fwiw, I think /dev/fdset doesn't care about cur_mon. However, qmp
handlers that use monitor_get_fd() usually depend on cur_mon.

Note: I wonder if add-fd (fdsets) and getfd (monitor fds) deserve to co-exist.

>
> > Example of use: error_report() & friends print "to current monitor if we
> > have one, else to stderr."  Makes sharing code between HMP and CLI
> > easier.  Uses @cur_mon under the hood.
>
> error_report() eventually prints to stderr both for cur_mon == NULL and
> for QMP monitors. Is there an important difference between both cases?
>
> There is error_vprintf_unless_qmp(), which behaves differently for both
> cases. However, it is only used in VNC code, so that code would have to
> keep coroutines disabled.
>
> Is cur_mon used much in other functions called by QMP handlers?
>
> > @cur_mon is thread-local.
> >
> > I'm afraid we have to save, clear and restore @cur_mon around a yield.
>
> That sounds painful enough that I'd rather avoid it.
>
> Kevin
>


-- 
Marc-André Lureau



Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

2020-03-23 Thread Kevin Wolf
Am 18.03.2020 um 16:36 hat Markus Armbruster geschrieben:
> Kevin Wolf  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 
> 
> Uh, what about @cur_mon?
> 
> @cur_mon points to the current monitor while a command executes.
> Initial value is null.  It is set in three places (not counting unit
> tests), and all three save, set, do something that may use @cur_mon,
> restore:
> 
> * monitor_qmp_dispatch(), for use within qmp_dispatch()
> * monitor_read(), for use within handle_hmp_command()
> * qmp_human_monitor_command(), also for use within handle_hmp_command()
> 
> Therefore, @cur_mon is null unless we're running within qmp_dispatch()
> or handle_hmp_command().

Can we make it NULL for coroutine-enabled handlers?

> Example of use: error_report() & friends print "to current monitor if we
> have one, else to stderr."  Makes sharing code between HMP and CLI
> easier.  Uses @cur_mon under the hood.

error_report() eventually prints to stderr both for cur_mon == NULL and
for QMP monitors. Is there an important difference between both cases?

There is error_vprintf_unless_qmp(), which behaves differently for both
cases. However, it is only used in VNC code, so that code would have to
keep coroutines disabled.

Is cur_mon used much in other functions called by QMP handlers?

> @cur_mon is thread-local.
> 
> I'm afraid we have to save, clear and restore @cur_mon around a yield.

That sounds painful enough that I'd rather avoid it.

Kevin




Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

2020-03-18 Thread Markus Armbruster
Kevin Wolf  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 

Uh, what about @cur_mon?

@cur_mon points to the current monitor while a command executes.
Initial value is null.  It is set in three places (not counting unit
tests), and all three save, set, do something that may use @cur_mon,
restore:

* monitor_qmp_dispatch(), for use within qmp_dispatch()
* monitor_read(), for use within handle_hmp_command()
* qmp_human_monitor_command(), also for use within handle_hmp_command()

Therefore, @cur_mon is null unless we're running within qmp_dispatch()
or handle_hmp_command().

Example of use: error_report() & friends print "to current monitor if we
have one, else to stderr."  Makes sharing code between HMP and CLI
easier.  Uses @cur_mon under the hood.

@cur_mon is thread-local.

I'm afraid we have to save, clear and restore @cur_mon around a yield.




Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

2020-03-03 Thread Kevin Wolf
Am 03.03.2020 um 09:49 hat Markus Armbruster geschrieben:
> Kevin Wolf  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 
> > ---
> >  include/qapi/qmp/dispatch.h |   1 +
> >  monitor/monitor-internal.h  |   6 +-
> >  monitor/monitor.c   |  55 +---
> >  monitor/qmp.c   | 122 +++-
> >  qapi/qmp-dispatch.c |  44 -
> >  qapi/qmp-registry.c |   3 +
> >  util/aio-posix.c|   7 ++-
> >  7 files changed, 196 insertions(+), 42 deletions(-)
> >
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index d6ce9efc8e..6812e49b5f 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -30,6 +30,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 3e6baba88f..f8123b151a 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 c1a6c4460f..72d57b5cd2 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -53,8 +53,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.
> 
> Also after setting @qmp_dispatcher_co_shutdown, right?
> 
> Happy to tweak the comment without a respin.

I understood a shutdown request as just another kind of request, but if
you want the comment to be more detailed, feel free.

Kevin




Re: [PATCH v5 3/4] qmp: Move dispatcher to a coroutine

2020-03-03 Thread Markus Armbruster
Kevin Wolf  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 
> ---
>  include/qapi/qmp/dispatch.h |   1 +
>  monitor/monitor-internal.h  |   6 +-
>  monitor/monitor.c   |  55 +---
>  monitor/qmp.c   | 122 +++-
>  qapi/qmp-dispatch.c |  44 -
>  qapi/qmp-registry.c |   3 +
>  util/aio-posix.c|   7 ++-
>  7 files changed, 196 insertions(+), 42 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index d6ce9efc8e..6812e49b5f 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -30,6 +30,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 3e6baba88f..f8123b151a 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 c1a6c4460f..72d57b5cd2 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -53,8 +53,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.

Also after setting @qmp_dispatcher_co_shutdown, right?

Happy to tweak the comment without a respin.

> + *
> + * 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, monitor_destroyed.  */
>  QemuMutex monitor_lock;
[...]

Reviewed-by: Markus Armbruster 




[PATCH v5 3/4] qmp: Move dispatcher to a coroutine

2020-02-18 Thread 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 
---
 include/qapi/qmp/dispatch.h |   1 +
 monitor/monitor-internal.h  |   6 +-
 monitor/monitor.c   |  55 +---
 monitor/qmp.c   | 122 +++-
 qapi/qmp-dispatch.c |  44 -
 qapi/qmp-registry.c |   3 +
 util/aio-posix.c|   7 ++-
 7 files changed, 196 insertions(+), 42 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index d6ce9efc8e..6812e49b5f 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -30,6 +30,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 3e6baba88f..f8123b151a 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 c1a6c4460f..72d57b5cd2 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -53,8 +53,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, monitor_destroyed.  */
 QemuMutex monitor_lock;
@@ -579,9 +603,24 @@ void monitor_cleanup(void)
 }
 qemu_mutex_unlock(_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(_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(_dispatcher_co_busy)));
+
 if (mon_iothread) {
 iothread_destroy(mon_iothread);
 mon_iothread = NULL;
@@ -604,9 +643,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 =