Re: [RESEND PATCH V10 02/15] iothread: introduce iothread_ref/unref to track attached devices

On Tue, Jul 21, 2026 at 8:20 PM Markus Armbruster <[email protected]> wrote:
>
> Zhang Chen <[email protected]> writes:
>
> > Currently, IOThreads do not maintain a record of which devices are
> > associated with them.
>
> It's not just devices, it's QOM objects and block exports, as far as I
> can tell.
>
> >                       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
>
> Search what?
>
> >   string comparison (strcmp), releases the associated memory
> >   upon a successful match.
>
> More implementation detail than I'd use here.
>
> > - holders: A IOThreadHolderList storing the IOThreadHolder
> >   of attached devices for runtime introspection.
>
> Is this the list mentioned for iothread_ref()?  If yes, I'd put it
> first, then refer to it, perhaps like this:
>
>   - holders: a list describing what is "holding" the I/O thread
>   - iothread_ref(): add a "holder" to this list
>   - iothread_unref(): remove a "holder" from this list
>
> And maybe
>
>   A "holder" can be a QOM object or a block export.
>
> > A later commit will add QMP commands to let management applications
> > query the attachment status of IOThreads.
> >
> > Signed-off-by: Zhang Chen <[email protected]>
> > Reviewed-by: Stefan Hajnoczi <[email protected]>
> > ---
> >  include/system/iothread.h | 11 ++++++
> >  iothread.c                | 70 +++++++++++++++++++++++++++++++++++++++
> >  qapi/misc.json            | 59 +++++++++++++++++++++++++++++++++
> >  3 files changed, 140 insertions(+)
> >
> > diff --git a/include/system/iothread.h b/include/system/iothread.h
> > index a1ef7696cb..ef0b2f9648 100644
> > --- a/include/system/iothread.h
> > +++ b/include/system/iothread.h
> > @@ -38,6 +38,9 @@
> >  #define IOTHREAD_POLL_WEIGHT_DEFAULT 0ULL
> >  #endif
> >
> > +typedef struct IOThreadHolder IOThreadHolder;
> > +typedef struct IOThreadHolderList IOThreadHolderList;
>
> These are is already defined in generated qapi/qapi-types-misc.h.  Why
> not include it instead?
>
> > +
> >  struct IOThread {
> >      EventLoopBase parent_obj;
> >
> > @@ -50,6 +53,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 export name.
> > +     */
>
> Back when holders was a GList, explaining the element type in a comment
> was useful.  Now it's not.
>
> > +    IOThreadHolderList *holders;
> >
> >      /* AioContext poll parameters */
> >      int64_t poll_max_ns;
> > @@ -82,4 +90,7 @@ void iothread_destroy(IOThread *iothread);
> >   */
> >  bool qemu_in_iothread(void);
> >
> > +void iothread_ref(IOThread *iothread, const IOThreadHolder *holder);
> > +void iothread_unref(IOThread *iothread, const IOThreadHolder *holder);
> > +
> >  #endif /* IOTHREAD_H */
> > diff --git a/iothread.c b/iothread.c
> > index 3558535b40..38f273c0e9 100644
> > --- a/iothread.c
> > +++ b/iothread.c
> > @@ -21,10 +21,78 @@
> >  #include "system/iothread.h"
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-commands-misc.h"
> > +#include "qapi/clone-visitor.h"
> > +#include "qapi/qapi-visit-misc.h"
> >  #include "qemu/error-report.h"
> >  #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.
> > + */
>
> This is confusing.  What's tracking what?
>
> Here's my attempt:
>
>    /*
>     * Add a deep copy of @holder to @iothread's list of holders.
>     */
>
> If you really want to use "tracking list", you should define the term,
> say with a comment next to @holder in struct IOThread.
>
> > +void iothread_ref(IOThread *iothread, const IOThreadHolder *holder)
> > +{
> > +    assert(holder);
> > +
> > +    QAPI_LIST_PREPEND(iothread->holders, QAPI_CLONE(IOThreadHolder, holder));
> > +}
> > +
> > +static int iothread_holder_compare(const IOThreadHolder *holder_a,
> > +                                   const IOThreadHolder *holder_b)
> > +{
> > +    const char *name_a, *name_b;
> > +
> > +    if (holder_a->type != holder_b->type) {
> > +        return holder_b->type - holder_a->type;
> > +    }
> > +
> > +    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_EXPORT:
> > +        name_a = holder_a->u.block_export.export_name;
> > +        name_b = holder_b->u.block_export.export_name;
> > +        break;
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> > +
> > +    return strcmp(name_a, name_b);
> > +}
> > +
> > +/*
> > + * This function removes the @holder from the @iothread's tracking list.
>
> Imperative mood, please:
>
>     * Remove @holder from @iothread's list of holders.
>
> > + * The @holder must match the one used previously in iothread_ref().
>
> I believe the next sentence makes this one redundant.
>
> > + * It is a programming error to call this with a @holder that is not
> > + * currently associated with the @iothread.
> > + */
> > +void iothread_unref(IOThread *iothread, const IOThreadHolder *holder)
> > +{
> > +    IOThreadHolderList **prev = &iothread->holders;
> > +    IOThreadHolderList *curr;
> > +
> > +    assert(holder);
> > +
> > +    while (*prev) {
> > +        curr = *prev;
> > +        if (iothread_holder_compare(curr->value, holder) == 0) {
> > +            *prev = curr->next;
> > +            curr->next = NULL;
> > +            qapi_free_IOThreadHolderList(curr);
> > +            return;
> > +        }
> > +        prev = &curr->next;
> > +    }
> > +
> > +    g_assert_not_reached();
> > +}
> > +
> >  static void *iothread_run(void *opaque)
> >  {
> >      IOThread *iothread = opaque;
> > @@ -129,6 +197,7 @@ static void iothread_instance_finalize(Object *obj)
> >          iothread->main_loop = NULL;
> >      }
> >      qemu_sem_destroy(&iothread->init_done_sem);
> > +    qapi_free_IOThreadHolderList(iothread->holders);
> >  }
> >
> >  static void iothread_init_gcontext(IOThread *iothread, const char *thread_name)
> > @@ -373,6 +442,7 @@ static int query_one_iothread(Object *object, void *opaque)
> >      info = g_new0(IOThreadInfo, 1);
> >      info->id = iothread_get_id(iothread);
> >      info->thread_id = iothread->thread_id;
> > +    info->holders = QAPI_CLONE(IOThreadHolderList, iothread->holders);
> >      info->poll_max_ns = iothread->poll_max_ns;
> >      info->poll_grow = iothread->poll_grow;
> >      info->poll_shrink = iothread->poll_shrink;
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index c71a5fe657..096e418b7a 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -67,6 +67,56 @@
> >  ##
> >  { 'command': 'query-name', 'returns': 'NameInfo', 'allow-preconfig': true }
> >
> > +
> > +##
> > +# @IOThreadHolderBlockExport:
> > +#
> > +# @export-name: Name of the block export.
>
> What's a block export name?
>
> Is it BlockExportOptions member @id?
>
> Is it BlockExportOptions member @node-name?
>
> Something else?
>
> > +#
> > +# Since: 11.1
>
> By now 11.2.
>
> > +#
> > +##
> > +{ 'struct': 'IOThreadHolderBlockExport',
> > +  'data': { 'export-name': 'str' } }
> > +
> > +##
> > +# @IOThreadHolderQomObject:
> > +#
> > +# @qom-path: Path to the object in the QOM tree.
> > +#
> > +# Since: 11.1
> > +#
> > +##
> > +{ 'struct': 'IOThreadHolderQomObject',
> > +  'data': { 'qom-path': 'str' } }
> > +
> > +##
> > +# @IOThreadHolderKind:
> > +#
> > +# @block-export: A block export.
> > +# @qom-object: A QOM Object.
> > +#
> > +# Since: 11.1
> > +##
> > +{ 'enum': 'IOThreadHolderKind',
> > +  'data': [ 'block-export', 'qom-object' ] }
> > +
> > +##
> > +# @IOThreadHolder:
> > +#
> > +# The block export or QOM object holding the I/O thread.
> > +#
> > +# @type: the kind of I/O thread holder.
>
> We should use "iothread" for consistency (even though I/O thread is
> nicer).
>
> > +#
> > +# Since: 11.1
> > +##
> > +{ 'union': 'IOThreadHolder',
> > +  'base': { 'type': 'IOThreadHolderKind' },
> > +  'discriminator': 'type',
> > +  'data': {
> > +    'block-export': 'IOThreadHolderBlockExport',
> > +    'qom-object': 'IOThreadHolderQomObject' } }
> > +
> >  ##
> >  # @IOThreadInfo:
> >  #
> > @@ -76,6 +126,11 @@
> >  #
> >  # @thread-id: ID of the underlying host thread
> >  #
> > +# @holders: the QOM objects or block nodes currently
> > +#     associated with this iothread.  When an associated component is
> > +#     detached or destroyed, it is removed from this list.
> > +#     (Since 11.1)
>
> Wrap lines more nicely, please:
>
>    # @holders: the QOM objects or block nodes currently associated with
>    #     this iothread.  When an associated component is detached or
>    #     destroyed, it is removed from this list.  (Since 11.1)
>
>

Thanks. I will update here and the commit message to describe holders
and remove the implementation details.

The current block-export holder value is actually the block node name.
I will change it to BlockExportOptions.id and expose it as "export-id",
so the holder identifies the block export rather than its backing node.

And rethink for the patch 14, I will drop it and keep the original name
"iothread_ref_and_get_aio_context" to avoid misunderstanding.

Thanks

Chen

> > +#
> >  # @poll-max-ns: maximum polling time in ns, 0 means polling is
> >  #     disabled (since 2.9)
> >  #
> > @@ -98,6 +153,7 @@
> >  { 'struct': 'IOThreadInfo',
> >    'data': {'id': 'str',
> >             'thread-id': 'int',
> > +           'holders': ['IOThreadHolder'],
> >             'poll-max-ns': 'int',
> >             'poll-grow': 'int',
> >             'poll-shrink': 'int',
> > @@ -124,6 +180,8 @@
> >  #              {
> >  #                 "id":"iothread0",
> >  #                 "thread-id":3134,
> > +#                 "holders":[{"qom-path": "/machine/peripheral/blk1/virtio-backend", "type": "qom-object"},
> > +#                            {"qom-path": "/machine/peripheral/blk2/virtio-backend", "type": "qom-object"}],
> >  #                 "poll-max-ns":32768,
> >  #                 "poll-grow":0,
> >  #                 "poll-shrink":0,
> > @@ -132,6 +190,7 @@
> >  #              {
> >  #                 "id":"iothread1",
> >  #                 "thread-id":3135,
> > +#                 "holders":[{"export-name": "fmt_qcow2", "type": "block-export"}],
> >  #                 "poll-max-ns":32768,
> >  #                 "poll-grow":0,
> >  #                 "poll-shrink":0,
>