Re: [PATCH V8 02/15] 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 IOThreadHolder to a list.
> - iothread_unref(): Searches for the IOThreadHolder using a custom
>   string comparison (g_strcmp0), releases the associated memory
>   upon a successful match.
> - holders: A GList storing the IOThreadHolder 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                | 94 +++++++++++++++++++++++++++++++++++++++
>  qapi/misc.json            | 61 +++++++++++++++++++++++++
>  3 files changed, 160 insertions(+)
>
> diff --git a/include/system/iothread.h b/include/system/iothread.h
> index a1ef7696cb..b9207ad829 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.

You forgot monitors.

Suggest "which can either be a QOM object, a block node, or a monitor."

> +     */
> +    GList *holders;
>  
>      /* AioContext poll parameters */
>      int64_t poll_max_ns;
> diff --git a/iothread.c b/iothread.c
> index 3558535b40..3301b8d495 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -25,6 +25,92 @@
>  #include "qemu/rcu.h"
>  #include "qemu/main-loop.h"
>  
> +/*
> + * iothread_ref:
> + * @iothread: the iothread to track
> + * @holder: the IOThreadHolder object initialized by the caller
> + *
> + * Add the @holder to the iothread's tracking list.
> + */
> +static void iothread_ref(IOThread *iothread, const IOThreadHolder *holder)

Yes, this is the interface I want :)

> +{
> +    assert(holder);
> +    IOThreadHolder *h = g_new0(IOThreadHolder, 1);
> +
> +    h->type = holder->type;
> +    switch (holder->type) {
> +    case IO_THREAD_HOLDER_KIND_QOM_OBJECT:
> +        h->u.qom_object.qom_path = g_strdup(holder->u.qom_object.qom_path);
> +        break;
> +    case IO_THREAD_HOLDER_KIND_BLOCK_NODE:
> +        h->u.block_node.node_name =
> +            g_strdup(holder->u.block_node.node_name);
> +        break;
> +    case IO_THREAD_HOLDER_KIND_MONITOR_NAME:
> +        h->u.monitor_name.monitor_name =
> +            g_strdup(holder->u.monitor_name.monitor_name);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }

This makes @h a deep copy of @holder.  Try

    IOThreadHolder *h = QAPI_CLONE(IOThreadHolder, holder);

instead.

Instead of making a copy, the function could take ownership of @holder,
and require @holder's members to be dynamically allocated.  More
efficient, but complicates the callers somewhat.  Not worthwhile, I
guess.

> +
> +    iothread->holders = g_list_prepend(iothread->holders, h);
> +}
> +
> +static int iothread_holder_compare(gconstpointer a, gconstpointer b)
> +{
> +    const IOThreadHolder *holder_a = a;
> +    const IOThreadHolder *holder_b = b;
> +    const char *name_a, *name_b;
> +
> +    if (holder_a->type != holder_b->type) {
> +        return -1;

Please use

           return holder_b->type - holder_a->type;

Why?  By convention, an int-valued function fn() comparing A to B
returns less than 0 when A<B, 0 when A == B, and greater than 0 when
A>B.  Anything else needs a prominent comment.  Best to stick to the
convention whenever practical.

Since A < B iff B > A, we want fn(A, B) == -fn(B, A).

Your function does not satisfy that!  Harmless for now, because you only
ever pass it to g_list_find_custom(), which treats all non-zero values
the same.  But if anyone passes it to something like qsort() in the
future, they can look forward to some debugging "fun".

> +    }
> +
> +    switch (holder_a->type) {
> +    case IO_THREAD_HOLDER_KIND_QOM_OBJECT:
> +        name_a = holder_a->u.qom_object.qom_path;
> +        name_b = holder_b->u.qom_object.qom_path;
> +        break;
> +    case IO_THREAD_HOLDER_KIND_BLOCK_NODE:
> +        name_a = holder_a->u.block_node.node_name;
> +        name_b = holder_b->u.block_node.node_name;
> +        break;
> +    case IO_THREAD_HOLDER_KIND_MONITOR_NAME:
> +        name_a = holder_a->u.monitor_name.monitor_name;
> +        name_b = holder_b->u.monitor_name.monitor_name;
> +        break;
> +    default:
> +       /*
> +        * This should not happen. If it does, name_a/b remains
> +        * NULL and g_strcmp0 will handle it safely.
> +        */
> +        name_a = NULL;
> +        name_b = NULL;

Replace this by

           g_assert_not_reached();

Then neither @name_a nor @name_b can be null, because a non-optional
QAPI str is never null.  Simply use strcmp().

> +    }
> +
> +    return g_strcmp0(name_a, name_b);
> +}
> +
> +/*
> + * This function removes the @holder from the @iothread's tracking list.
> + * The @holder 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 IOThreadHolder *holder)
> +{
> +    assert(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);

Removing @link from the list before freeing link->data would be more
obviously correct, wouldn't it?

> +}
> +
>  static void *iothread_run(void *opaque)
>  {
>      IOThread *iothread = opaque;
> @@ -356,6 +442,14 @@ char *iothread_get_id(IOThread *iothread)
>  
>  AioContext *iothread_get_aio_context(IOThread *iothread)
>  {
> +    /* Remove in next patch for build */
> +    IOThreadHolder holder = {
> +        .type = IO_THREAD_HOLDER_KIND_QOM_OBJECT,
> +        .u.qom_object.qom_path = (char *)"tmp_path",
> +    };
> +    iothread_ref(iothread, &holder);
> +    iothread_unref(iothread, &holder);
> +

Is this really needed to make this patch work?

>      return iothread->ctx;
>  }
>  
> diff --git a/qapi/misc.json b/qapi/misc.json
> index c71a5fe657..d9f82f0922 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -67,6 +67,67 @@
>  ##
>  { 'command': 'query-name', 'returns': 'NameInfo', 'allow-preconfig': true }
>  
> +
> +##
> +# @IOThreadHolderBlockNode:
> +#
> +# @node-name: A block node.

Let's use

   # @node-name: Name of the block node

to make it more similar to node-name descriptions elsewhere.

> +#
> +# Since: 11.1
> +#
> +##
> +{ 'struct': 'IOThreadHolderBlockNode',
> +  'data': { 'node-name': 'str' } }
> +
> +##
> +# @IOThreadHolderQomObject:
> +#
> +# @qom-path: A QOM Object.

Likewise, let's use

  # @qom-path: Path to the object in the QOM tree

> +#
> +# Since: 11.1
> +#
> +##
> +{ 'struct': 'IOThreadHolderQomObject',
> +  'data': { 'qom-path': 'str' } }
> +
> +##
> +# @IOThreadHolderMonitor:
> +#

Let's use

   # @monitor-name: Name of the HMP/QMP monitor

> +# @monitor-name: A HMP/QMP monitor.
> +#
> +# Since: 11.1
> +#
> +##
> +{ 'struct': 'IOThreadHolderMonitor',
> +  'data': { 'monitor-name': 'str' } }
> +
> +##
> +# @IOThreadHolderKind:
> +#
> +# @block-node: A block node.
> +# @qom-object: A QOM Object.
> +# @monitor-name: A HMP/QMP monitor.

Rename to @monitor for consistency with the other two.

Remark, not a request to change anything in your patch: kind monitor
will become obsolete when monitors become QOM objects.  Doing that
before we release this interface is desirable.

> +#
> +# Since: 11.1
> +##
> +{ 'enum': 'IOThreadHolderKind',
> +  'data': [ 'block-node', 'qom-object', 'monitor-name' ] }
> +
> +##
> +# @IOThreadHolder:
> +#

Let's add a short description:

   # The block node, QOM object, or monitor holding the I/O thread.

> +# @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',
> +    'monitor-name': 'IOThreadHolderMonitor' } }
> +
>  ##
>  # @IOThreadInfo:
>  #