Additional nitpick detail on Kevin's request.
Kevin Wolf <kw...@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 <kw...@redhat.com>
> Reviewed-by: Markus Armbruster <arm...@redhat.com>
[...]
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 629aa073ee..ac2722bf91 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 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,
> @@ -623,9 +647,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;
> @@ -649,9 +688,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);
Rather long line, caused by rather long identifiers.
Not your fault; you imitated the existing pattern static variable
qmp_dispatcher_bh / extern function monitor_qmp_bh_dispatcher(). But
the prefix monitor_qmp_ is awfully long, and not just here. Let's leave
this for another day.
> + 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/util/aio-posix.c b/util/aio-posix.c
> index f7f13ebfc2..30bb21d699 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"
> @@ -558,8 +559,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.
Wrapping the comment around column 70 or so would make it easier to
read. Up to you.
> */
> - assert(in_aio_context_home_thread(ctx));
> + assert(in_aio_context_home_thread(ctx == iohandler_get_aio_context() ?
> + qemu_get_aio_context() : ctx));
>
> qemu_lockcnt_inc(&ctx->list_lock);
