Re: [PATCH V6 06/14] monitor: Update tracking iothread users with holder name

[Markus Armbruster <armbru@redhat.com>]

Zhang Chen <zhangckid@gmail.com> writes:

> Since the Monitor struct is not a QOM Object, we cannot use
> object_get_canonical_path(). Instead, this patch uses the monitor's
> name (or a default type-based name) as the holder identifier.
>
> Cache the AioContext in the Monitor struct to avoid repeated calls to
> iothread_get_aio_context() and ensure symmetrical ref/unref during
> monitor lifecycle.
>
> Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> ---
>  monitor/monitor-internal.h |  3 +++
>  monitor/monitor.c          | 24 ++++++++++++++++++++++--
>  monitor/qmp.c              |  3 ++-
>  3 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index feca111ae3..51cedf90e4 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -116,6 +116,9 @@ struct Monitor {
>      guint out_watch;
>      int mux_out;
>      int reset_seen;
> +
> +    /* iothread context */
> +    AioContext *ctx;
>  };
>  
>  struct MonitorHMP {
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 00b93ed612..b6efe776d6 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -529,7 +529,7 @@ int 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);
> @@ -564,7 +564,7 @@ void monitor_resume(Monitor *mon)
>          AioContext *ctx;
>  
>          if (mon->use_io_thread) {
> -            ctx = iothread_get_aio_context(mon_iothread);
> +            ctx = mon->ctx;
>          } else {
>              ctx = qemu_get_aio_context();
>          }
> @@ -612,6 +612,18 @@ void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
>  {
>      if (use_io_thread && !mon_iothread) {
>          monitor_iothread_init();
> +        /*
> +         * Because of current Monitor is not a QOM Object,
> +         * so using OBJECT(mon) is undefined behavior and may crash.
> +         * Try using a hard-coded future implementation of the qom path instead.
> +         * (Like the name of the "mon_iothread").
> +         * long-term solution would be making Monitor QOM, after that change
> +         * here to:
> +         * g_autofree path = object_get_canonical_path(OBJECT(mon));
> +         */
> +        g_autofree char *path = g_strdup(is_qmp ? "/monitor/qmp_mon0" :
> +                                                  "/monitor/hmp_mon0");
> +        mon->ctx = iothread_ref_and_get_aio_context(mon_iothread, path);

Oh.

PATCH 02 defines QAPI type IoThreadHolder as a union with two branches
for the two kinds of holders: one for block node holder, and one for QOM
object holder.  Both branches use a string to identify the holder.

The C code in PATCH 02 uses just a string to identify the holder.  This
silently assumes that the strings for block nodes are distinct from the
strings for QOM objects.  True as long as we only use absolute QOM
paths: these start with '/', and block nodes can't.

This patch shows me there's actually a third kind: monitor.  You use the
QOM object branch for it.  That's confusing; a monitor is not a QOM
object.  Moreover, you make yet another silent assumption: absolute QOM
paths do not start with "/monitor/".

This is too much for me.

Please use a separate IoThreadHolderKind value and IoThreadHolder branch
for each kind of holder.  The kinds I've seen so far are block-node, QOM
object, monitor.

If Daniel Berrangé's "[PATCH RFC 00/17] monitor: turn QMP and HMP into
QOM objects" gets merged, kind monitor can go away.

Use of just a string to identify the holder requires strings for
different kinds to be distinct.  The argument why they are must be
written down, simple, and likely to remain true.

But I'd prefer to use IoThreadHolder instead of string.  No assumptions
necessary then.

If this IoThreadHolder arguments make the calls ugly or overly verbose,
consider thin wrappers for each kind, i.e.

    new_ctx = iothread_get_aio_context_block(iothread, bs);

instead of

    holder_name = bdrv_get_node_name(bs);
    new_ctx = iothread_get_aio_context(iothread, holder_name);

and so forth.

Questions?

>      }
>      qemu_mutex_init(&mon->mon_lock);
>      mon->is_qmp = is_qmp;
> @@ -631,6 +643,14 @@ void monitor_data_destroy(Monitor *mon)
>      }
>      g_string_free(mon->outbuf, true);
>      qemu_mutex_destroy(&mon->mon_lock);
> +
> +    if (mon->ctx && mon_iothread) {
> +        g_autofree char *path = g_strdup(monitor_is_qmp(mon) ?
> +                                        "/monitor/qmp_mon0" :
> +                                        "/monitor/hmp_mon0");
> +        iothread_put_aio_context(mon_iothread, path);
> +        mon->ctx = NULL;
> +    }
>  }
>  
>  void monitor_cleanup(void)
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 687019811f..8e4a775fac 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -549,7 +549,8 @@ void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp)
>           * since chardev might be running in the monitor I/O
>           * thread.  Schedule a bottom half.
>           */
> -        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> +        Monitor *mon_p = &mon->common;
> +        aio_bh_schedule_oneshot(mon_p->ctx,
>                                  monitor_qmp_setup_handlers_bh, mon);
>          /* The bottom half will add @mon to @mon_list */
>      } else {