Re: [PATCH V17 04/13] monitor: track IOThread users with QOM paths

[Markus Armbruster <armbru@redhat.com>]

Zhang Chen <zhangckid@gmail.com> 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 <zhangckid@gmail.com>
> ---
>  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 <armbru@redhat.com>