Re: [PATCH V14 06/15] 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, using the monitor
> QOM path as the holder, and reuse the stored context for later
> operations.
>
> Release the holder from the Monitor ObjectClass::unparent callback,
> while the canonical QOM path is still available. This covers every
> object_unparent() path and avoids duplicating the cleanup in
> monitor_cleanup() and monitor_qmp_prepare_delete().
>
> 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>

I have a question for our QOM experts (Paolo and Daniel).

This patch takes a reference in its UserCreatableClass method
complete(), and relinquishes it in its ObjectClass unparent() method.
Is that correct and clean?  Or should complete() pair with
prepare_delete()?

> ---
>  monitor/monitor-internal.h |  3 +++
>  monitor/monitor.c          | 33 ++++++++++++++++++++++++++++-----
>  monitor/qmp.c              |  5 +++--
>  3 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index e0a124a317..74ea15c85f 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -154,6 +154,9 @@ struct Monitor {
>      guint out_watch;
>      int mux_out;
>      int reset_seen;
> +
> +    /* iothread context */
> +    AioContext *ctx;
>  };
>  
>  struct MonitorHMPClass {
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 6af09f2f6e..9e3b639eff 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -81,6 +81,22 @@ int monitor_device_index;
>  OBJECT_DEFINE_TYPE_EXTENDED(Monitor, monitor, MONITOR, OBJECT, true,
>                              { TYPE_USER_CREATABLE }, {});
>  
> +static void monitor_unparent(Object *obj)
> +{
> +    Monitor *mon = MONITOR(obj);
> +
> +    if (mon->ctx && monitor_requires_iothread(mon)) {
> +        g_autofree char *path = object_get_canonical_path(obj);
> +        const IOThreadHolder io_holder = {
> +            .type = IO_THREAD_HOLDER_KIND_QOM_OBJECT,
> +            .u.qom_object.qom_path = path,
> +        };
> +
> +        iothread_unref_and_put_aio_context(mon_iothread, &io_holder);
> +        mon->ctx = NULL;
> +    }
> +}
> +
>  static void monitor_finalize(Object *obj)
>  {
>      Monitor *mon = MONITOR(obj);
> @@ -116,6 +132,8 @@ static void monitor_class_init(ObjectClass *cls, const void *data)
>  {
>      UserCreatableClass *ucc = USER_CREATABLE_CLASS(cls);
>  
> +    cls->unparent = monitor_unparent;
> +
>      object_class_property_add_str(cls, "chardev",
>                                    monitor_get_chardev_id,
>                                    monitor_set_chardev_id);
> @@ -575,7 +593,7 @@ void monitor_suspend(Monitor *mon)
>           * 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);
> @@ -715,7 +733,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);
> @@ -734,11 +751,17 @@ static void monitor_complete(UserCreatable *uc, Error **errp)
>              mon_iothread = iothread_create("mon_iothread", &error_abort);
>          }
>  
> -        ctx = iothread_get_aio_context(mon_iothread);
> +        g_autofree char *path = object_get_canonical_path(OBJECT(mon));
> +        const IOThreadHolder io_holder = {
> +            .type = IO_THREAD_HOLDER_KIND_QOM_OBJECT,
> +            .u.qom_object.qom_path = 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..de56345456 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -734,7 +734,8 @@ 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 {
> @@ -789,7 +790,7 @@ 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),
> +        aio_wait_bh_oneshot(MONITOR(mon)->ctx,
>                              monitor_qmp_iothread_quiesce, NULL);
>      }