Zhang Chen <[email protected]> writes:
> Monitors that require an IOThread share the internal mon_iothread.
> Account for each monitor explicitly in IOThread holder tracking, using
> the monitor's canonical QOM path to distinguish holders.
>
> Acquire the shared monitor IOThread AioContext once and store the
> IOThread and canonical QOM path so the holder can be released after the
> monitor has been removed from the QOM tree. Use the stored IOThread for
> per-monitor checks and accesses after initialization, and reuse the
> stored context for later operations.
>
> Release the holder from monitor_finalize(), after resources associated
> with the AioContext have been destroyed. This pairs the reference taken
> in complete() with instance finalization and keeps the IOThread alive
> throughout monitor teardown.
>
> The internal mon_iothread remains hidden from query-iothreads, as before.
> Management applications therefore cannot use it to determine which
> monitors are using the IOThread. Nevertheless, track the monitor
> holders so that they follow the same explicit holder and
> IOThread reference lifecycle as other AioContext users.
>
> Signed-off-by: Zhang Chen <[email protected]>
> ---
> monitor/monitor-internal.h | 7 +++++--
> monitor/monitor.c | 36 ++++++++++++++++++++++++++----------
> monitor/qmp.c | 14 +++++++-------
> 3 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 822a66d06d..020afdaebf 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -77,6 +77,11 @@ struct Monitor {
> GString *outbuf;
> guint out_watch;
> int mux_out;
> +
> + /* iothread context and holder identity */
> + IOThread *iothread;
> + char *iothread_qom_path;
> + AioContext *ctx;
> };
>
> struct MonitorQMPClass {
> @@ -109,14 +114,12 @@ struct MonitorQMP {
> };
>
> typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
> -extern IOThread *mon_iothread;
> extern Coroutine *qmp_dispatcher_co;
> extern bool qmp_dispatcher_co_shutdown;
> extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> extern QemuMutex monitor_lock;
> extern MonitorList mon_list;
>
> -bool monitor_requires_iothread(const Monitor *mon);
> int monitor_can_read(void *opaque);
> void monitor_cancel_out_watch(Monitor *mon);
> void monitor_list_append(Monitor *mon);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 2654b81890..a80d8dbf45 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -55,7 +55,7 @@ typedef struct {
> } MonitorQAPIEventConf;
>
> /* Shared monitor I/O thread */
> -IOThread *mon_iothread;
> +static IOThread *mon_iothread;
>
> /* Coroutine to dispatch the requests received from I/O thread */
> Coroutine *qmp_dispatcher_co;
> @@ -93,6 +93,16 @@ static void monitor_finalize(Object *obj)
> qemu_chr_fe_deinit(&mon->chr, false);
> g_string_free(mon->outbuf, true);
> qemu_mutex_destroy(&mon->mon_lock);
> +
> + if (mon->iothread) {
> + const IOThreadHolder io_holder = {
> + .type = IO_THREAD_HOLDER_KIND_QOM_OBJECT,
> + .u.qom_object.qom_path = mon->iothread_qom_path,
> + };
> +
> + iothread_unref_and_put_aio_context(mon->iothread, &io_holder);
> + }
> + g_free(mon->iothread_qom_path);
> }
>
> static char *monitor_get_chardev_id(Object *obj, Error **errp)
> @@ -164,7 +174,7 @@ Monitor *monitor_set_cur(Coroutine *co, Monitor *mon)
> return old_monitor;
> }
>
> -bool monitor_requires_iothread(const Monitor *mon)
> +static bool monitor_requires_iothread(const Monitor *mon)
> {
> MonitorClass *cls = MONITOR_GET_CLASS(mon);
> return cls->requires_iothread && cls->requires_iothread(mon);
> @@ -188,8 +198,8 @@ void monitor_cancel_out_watch(Monitor *mon)
> GMainContext *ctx = NULL;
> GSource *src;
>
> - if (monitor_requires_iothread(mon)) {
> - ctx = iothread_get_g_main_context(mon_iothread);
> + if (mon->iothread) {
> + ctx = iothread_get_g_main_context(mon->iothread);
> }
> src = g_main_context_find_source_by_id(ctx, mon->out_watch);
> if (!src && ctx) {
> @@ -516,12 +526,12 @@ void monitor_suspend(Monitor *mon)
> {
> qatomic_inc(&mon->suspend_cnt);
>
> - if (monitor_requires_iothread(mon)) {
> + if (mon->iothread) {
> /*
> * Kick I/O thread to make sure this takes effect. It'll be
> * evaluated again in prepare() of the watch object.
> */
> - aio_notify(iothread_get_aio_context(mon_iothread));
> + aio_notify(mon->ctx);
> }
>
> trace_monitor_suspend(mon, 1);
> @@ -661,7 +671,6 @@ char *monitor_compat_id(void)
> static void monitor_complete(UserCreatable *uc, Error **errp)
> {
> Monitor *mon = MONITOR(uc);
> - AioContext *ctx;
>
> if (mon->chardev_id) {
> Chardev *chr = qemu_chr_find(mon->chardev_id);
> @@ -680,11 +689,18 @@ static void monitor_complete(UserCreatable *uc, Error
> **errp)
> mon_iothread = iothread_create("mon_iothread", &error_abort);
> }
>
> - ctx = iothread_get_aio_context(mon_iothread);
> + mon->iothread = mon_iothread;
> + mon->iothread_qom_path = object_get_canonical_path(OBJECT(mon));
Hmm, this is the monitor's QOM path, not the I/O threads. Let's rename
Monitor member iothread_qom_path to monitor_qom_path or just qom_path.
> + const IOThreadHolder io_holder = {
> + .type = IO_THREAD_HOLDER_KIND_QOM_OBJECT,
> + .u.qom_object.qom_path = mon->iothread_qom_path,
> + };
> +
> + mon->ctx = iothread_ref_and_get_aio_context(mon->iothread,
> &io_holder);
> } else {
> - ctx = qemu_get_aio_context();
> + mon->ctx = qemu_get_aio_context();
> }
> - mon->accept_input_bh = aio_bh_new(ctx, monitor_accept_input, mon);
> + mon->accept_input_bh = aio_bh_new(mon->ctx, monitor_accept_input, mon);
> }
>
> int monitor_new(MonitorOptions *opts, bool allow_hmp, Error **errp)
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index aec0315775..e235302fb3 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -187,7 +187,7 @@ static void monitor_qmp_caps_reset(MonitorQMP *mon)
> memset(mon->capab_offered, 0, sizeof(mon->capab_offered));
> memset(mon->capab, 0, sizeof(mon->capab));
> mon->capab_offered[QMP_CAPABILITY_OOB] =
> - monitor_requires_iothread(MONITOR(mon));
> + MONITOR(mon)->iothread != NULL;
> }
>
> static void qmp_request_free(QMPRequest *req)
> @@ -678,8 +678,8 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
> MonitorQMP *mon = opaque;
> GMainContext *context;
>
> - assert(monitor_requires_iothread(MONITOR(mon)));
> - context = iothread_get_g_main_context(mon_iothread);
> + assert(MONITOR(mon)->iothread);
> + context = iothread_get_g_main_context(MONITOR(mon)->iothread);
> assert(context);
> qemu_chr_fe_set_handlers(&mon->parent_obj.chr, monitor_can_read,
> monitor_qmp_read, monitor_qmp_event,
> @@ -717,7 +717,7 @@ static void monitor_qmp_complete(UserCreatable *uc, Error
> **errp)
>
> qemu_chr_fe_set_echo(&mon->parent_obj.chr, true);
>
> - if (monitor_requires_iothread(MONITOR(mon))) {
> + if (MONITOR(mon)->iothread) {
> /*
> * Make sure the old iowatch is gone. It's possible when
> * e.g. the chardev is in client mode, with wait=on.
> @@ -734,7 +734,7 @@ static void monitor_qmp_complete(UserCreatable *uc, Error
> **errp)
> * thread. Schedule a bottom half.
> */
> mon->setup_pending = true;
> - aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> + aio_bh_schedule_oneshot(MONITOR(mon)->ctx,
> monitor_qmp_setup_handlers_bh, mon);
> /* The bottom half will add @mon to @mon_list */
> } else {
> @@ -788,8 +788,8 @@ static bool monitor_qmp_prepare_delete(UserCreatable *uc,
> Error **errp)
> }
>
> /* Synchronize with in-flight iothread callbacks. */
> - if (monitor_requires_iothread(mon)) {
> - aio_wait_bh_oneshot(iothread_get_aio_context(mon_iothread),
> + if (mon->iothread) {
> + aio_wait_bh_oneshot(mon->ctx,
> monitor_qmp_iothread_quiesce, NULL);
> }
With the rename
Reviewed-by: Markus Armbruster <[email protected]>
