Re: [PATCH V7 02/14] iothread: introduce iothread_ref/unref to track attached devices

[Markus Armbruster <armbru@redhat.com>]

Zhang Chen <zhangckid@gmail.com> writes:

> Currently, IOThreads do not maintain a record of which devices are
> associated with them. This makes it difficult to monitor the
> workload distribution of IOThreads, especially in complex
> hotplug scenarios involving multiple virtio-blk or virtio-scsi devices.
>
> This patch introduces a reference counting and tracking mechanism
> within the IOThread object:
>
> - iothread_ref(): Prepends the device's QOM path to a list.
> - iothread_unref(): Searches for the device path using a custom
>   string comparison (g_strcmp0), releases the associated memory
>   upon a successful match.
> - holders: A GList storing the QOM paths of attached devices
>   for runtime introspection.
>
> A later commit will add QMP commands to let management applications
> query the attachment status of IOThreads.
>
> Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> ---
>  include/system/iothread.h |  5 +++
>  iothread.c                | 67 +++++++++++++++++++++++++++++++++++++++
>  qapi/misc.json            | 48 ++++++++++++++++++++++++++++
>  3 files changed, 120 insertions(+)
>
> diff --git a/include/system/iothread.h b/include/system/iothread.h
> index a1ef7696cb..2871b06edc 100644
> --- a/include/system/iothread.h
> +++ b/include/system/iothread.h
> @@ -50,6 +50,11 @@ struct IOThread {
>      bool stopping;              /* has iothread_stop() been called? */
>      bool running;               /* should iothread_run() continue? */
>      int thread_id;
> +    /*
> +     * The list elements are of type IoThreadHolder, which can
> +     * represent either a QOM path or a block node name.
> +     */
> +    GList *holders;
>  
>      /* AioContext poll parameters */
>      int64_t poll_max_ns;
> diff --git a/iothread.c b/iothread.c
> index 3558535b40..b805e4f97d 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -25,6 +25,66 @@
>  #include "qemu/rcu.h"
>  #include "qemu/main-loop.h"
>  
> +/*
> + * Add the @holder path to the iothread's tracking list.
> + * The @holder is a QOM path if it starts with '/', else a block node name.
> + */

This interface overloads two string keys: QOM paths and block node
names.  For that to work, a key value must be unambiguously either QOM
path or block node name.  To get that, you restrict QOM path to
absolute.  Not exactly pretty, but it works.

What if you ever need to add another kind of holder?

Any new kind of holder would need a string key that cannot start with
'/', and cannot clash with block node names.  You'd either have to redo
this interface, or outlaw the clashes.

PATCH 06 in fact adds a new kind of holder: monitors.  To keep this
interface working, it uses made-up QOM paths as keys.  This is cheating.

Daniel Berrangé posted a series that turns monitors into QOM objects.
If it gets merged, your series can be fixed not to cheat.

More in review of PATCH 06.

> +static void iothread_ref(IOThread *iothread, const char *holder)
> +{
> +    IoThreadHolder *h = g_new0(IoThreadHolder, 1);
> +
> +    assert(holder);
> +
> +    if (holder[0] == '/') {
> +        h->type = IO_THREAD_HOLDER_KIND_QOM_OBJECT;
> +        h->u.qom_object.data = g_strdup(holder);
> +    } else {
> +        h->type = IO_THREAD_HOLDER_KIND_BLOCK_NODE;
> +        h->u.block_node.data = g_strdup(holder);
> +    }
> +
> +    iothread->holders = g_list_prepend(iothread->holders, h);
> +}
> +
> +static int iothread_holder_compare(gconstpointer a, gconstpointer b)
> +{
> +    const IoThreadHolder *holder_node = a;
> +    const char *target_name = b;
> +    const char *current_name;
> +
> +    if (holder_node->type == IO_THREAD_HOLDER_KIND_QOM_OBJECT) {
> +        current_name = holder_node->u.qom_object.data;
> +    } else if (holder_node->type == IO_THREAD_HOLDER_KIND_BLOCK_NODE) {
> +        current_name = holder_node->u.block_node.data;
> +    } else {
> +        /*
> +         * This should not happen. If it does, current_name remains
> +         * NULL and g_strcmp0 will handle it safely.
> +         */
> +        current_name = NULL;
> +    }
> +
> +    return g_strcmp0(current_name, target_name);
> +}
> +
> +/*
> + * This function removes the @holder from the @iothread's tracking list.
> + * The @holder string must match the one used previously in iothread_ref).
> + * It is a programming error to call this with a @holder that is not
> + * currently associated with the @iothread.
> + */
> +static void iothread_unref(IOThread *iothread, const char *holder)
> +{
> +    GList *link = g_list_find_custom(iothread->holders, holder,
> +                                     (GCompareFunc)iothread_holder_compare);
> +
> +    assert(link);
> +
> +    IoThreadHolder *h = (IoThreadHolder *)link->data;
> +    qapi_free_IoThreadHolder(h);
> +    iothread->holders = g_list_delete_link(iothread->holders, link);
> +}
> +
>  static void *iothread_run(void *opaque)
>  {
>      IOThread *iothread = opaque;
> @@ -108,6 +168,9 @@ static void iothread_instance_finalize(Object *obj)
>  
>      iothread_stop(iothread);
>  
> +    /* We don't support finalize without holders */
> +    assert(iothread->holders == NULL);

The comment seems to say "no holders is not supposed to happen".

The assertion says "holders is not supposed to happen".

Which one is wrong?

> +
>      /*
>       * Before glib2 2.33.10, there is a glib2 bug that GSource context
>       * pointer may not be cleared even if the context has already been
> @@ -356,6 +419,10 @@ char *iothread_get_id(IOThread *iothread)
>  
>  AioContext *iothread_get_aio_context(IOThread *iothread)
>  {
> +    /* Remove in next patch for build */
> +    iothread_ref(iothread, "tmp");
> +    iothread_unref(iothread, "tmp");
> +
>      return iothread->ctx;
>  }
>  
> diff --git a/qapi/misc.json b/qapi/misc.json
> index c71a5fe657..5fb7dcfcad 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -67,6 +67,54 @@
>  ##
>  { 'command': 'query-name', 'returns': 'NameInfo', 'allow-preconfig': true }
>  
> +
> +##
> +# @IoThreadHolderBlockNode:
> +#
> +# @data: Block node name.

Could we name this @node-name?

> +#
> +# Since: 11.1
> +#
> +##
> +{ 'struct': 'IoThreadHolderBlockNode',
> +  'data': { 'data': 'str' } }
> +
> +##
> +# @IoThreadHolderQomObject:
> +#
> +# @data: Absolute @qom-path.

Could we name this @qom-path?

> +#
> +# Since: 11.1
> +#
> +##
> +{ 'struct': 'IoThreadHolderQomObject',
> +  'data': { 'data': 'str' } }
> +
> +##
> +# @IoThreadHolderKind:
> +#
> +# @block-node: Block node name.
> +# @qom-object: Absolute @qom-path.

Let's use

   # @block-node: A block node
   #
   # @qom-object: A QOM Object

> +#
> +# Since: 11.1
> +##
> +{ 'enum': 'IoThreadHolderKind',
> +  'data': [ 'block-node', 'qom-object' ] }
> +
> +##
> +# @IoThreadHolder:
> +#
> +# @type: the kind of I/O thread holder.
> +#
> +# Since: 11.1
> +##
> +{ 'union': 'IoThreadHolder',
> +  'base': { 'type': 'IoThreadHolderKind' },
> +  'discriminator': 'type',
> +  'data': {
> +    'block-node': 'IoThreadHolderBlockNode',
> +    'qom-object': 'IoThreadHolderQomObject' } }
> +
>  ##
>  # @IOThreadInfo:
>  #