[PATCH V5 03/13] iothread: tracking iothread users with holder name

Zhang Chen posted 13 patches 1 month, 1 week ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony PERARD <anthony@xenproject.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, John Levon <john.levon@nutanix.com>, Thanos Makatos <thanos.makatos@nutanix.com>, "Cédric Le Goater" <clg@redhat.com>, David Hildenbrand <david@kernel.org>, "Dr. David Alan Gilbert" <dave@treblig.org>, Markus Armbruster <armbru@redhat.com>, Zhang Chen <zhangckid@gmail.com>, Li Zhijian <lizhijian@fujitsu.com>, Jason Wang <jasowang@redhat.com>, Eric Blake <eblake@redhat.com>
There is a newer version of this series
[PATCH V5 03/13] iothread: tracking iothread users with holder name
Posted by Zhang Chen 1 month, 1 week ago
Introduce iothread_get_aio_context() with a 'holder' argument and its
counterpart iothread_put_aio_context().

Previously, users of an IOThread's AioContext did not explicitly
record their identity, making it difficult to debug which devices or
subsystems were pinning an IOThread.

This patch enhances the reference counting mechanism by:
1. Automatically incrementing the object reference count when a context
   is retrieved.
2. Tracking holders by name using iothread_ref() and iothread_unref().

In iothread_instance_finalize(), we now retrieve the source name from
the GMainContext to correctly unref the initial internal holder.

Signed-off-by: Zhang Chen <zhangckid@gmail.com>
---
 include/system/iothread.h |  3 ++-
 iothread.c                | 28 ++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/include/system/iothread.h b/include/system/iothread.h
index 21a76bd70d..595abeefbe 100644
--- a/include/system/iothread.h
+++ b/include/system/iothread.h
@@ -47,7 +47,8 @@ DECLARE_INSTANCE_CHECKER(IOThread, IOTHREAD,
 
 char *iothread_get_id(IOThread *iothread);
 IOThread *iothread_by_id(const char *id);
-AioContext *iothread_get_aio_context(IOThread *iothread);
+AioContext *iothread_get_aio_context(IOThread *iothread, const char *holder);
+void iothread_put_aio_context(IOThread *iothread, const char *holder);
 GMainContext *iothread_get_g_main_context(IOThread *iothread);
 
 /*
diff --git a/iothread.c b/iothread.c
index 80a8cf4b32..da98fbb9ad 100644
--- a/iothread.c
+++ b/iothread.c
@@ -172,7 +172,8 @@ static void iothread_init_gcontext(IOThread *iothread, const char *thread_name)
     g_autofree char *name = g_strdup_printf("%s aio-context", thread_name);
 
     iothread->worker_context = g_main_context_new();
-    source = aio_get_g_source(iothread_get_aio_context(iothread));
+    /* No need setup itself as the init holder */
+    source = aio_get_g_source(iothread_get_aio_context(iothread, NULL));
     g_source_set_name(source, name);
     g_source_attach(source, iothread->worker_context);
     g_source_unref(source);
@@ -362,11 +363,34 @@ char *iothread_get_id(IOThread *iothread)
     return g_strdup(object_get_canonical_path_component(OBJECT(iothread)));
 }
 
-AioContext *iothread_get_aio_context(IOThread *iothread)
+AioContext *iothread_get_aio_context(IOThread *iothread, const char *holder)
 {
+    /*
+     * In some cases, iothread user need the ctx to clearup other resource.
+     * When holder is empty, back to the legacy way.
+     */
+    if (holder) {
+        /*
+         * This guarantees that the IOThread and its AioContext remain alive
+         * as long as there is a holder.
+         */
+        object_ref(OBJECT(iothread));
+
+        /* Add holder device path to the list */
+        iothread_ref(iothread, holder);
+    }
+
     return iothread->ctx;
 }
 
+void iothread_put_aio_context(IOThread *iothread, const char *holder)
+{
+    object_unref(OBJECT(iothread));
+
+    /* Delete holder device path from the list */
+    iothread_unref(iothread, holder);
+}
+
 static int query_one_iothread(Object *object, void *opaque)
 {
     IOThreadInfoList ***tail = opaque;
-- 
2.49.0
Re: [PATCH V5 03/13] iothread: tracking iothread users with holder name
Posted by Stefan Hajnoczi 1 month ago
On Thu, Mar 05, 2026 at 10:24:49PM +0800, Zhang Chen wrote:
> Introduce iothread_get_aio_context() with a 'holder' argument and its
> counterpart iothread_put_aio_context().
> 
> Previously, users of an IOThread's AioContext did not explicitly
> record their identity, making it difficult to debug which devices or
> subsystems were pinning an IOThread.
> 
> This patch enhances the reference counting mechanism by:
> 1. Automatically incrementing the object reference count when a context
>    is retrieved.
> 2. Tracking holders by name using iothread_ref() and iothread_unref().
> 
> In iothread_instance_finalize(), we now retrieve the source name from
> the GMainContext to correctly unref the initial internal holder.
> 
> Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> ---
>  include/system/iothread.h |  3 ++-
>  iothread.c                | 28 ++++++++++++++++++++++++++--
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/include/system/iothread.h b/include/system/iothread.h
> index 21a76bd70d..595abeefbe 100644
> --- a/include/system/iothread.h
> +++ b/include/system/iothread.h
> @@ -47,7 +47,8 @@ DECLARE_INSTANCE_CHECKER(IOThread, IOTHREAD,
>  
>  char *iothread_get_id(IOThread *iothread);
>  IOThread *iothread_by_id(const char *id);
> -AioContext *iothread_get_aio_context(IOThread *iothread);
> +AioContext *iothread_get_aio_context(IOThread *iothread, const char *holder);

This commit breaks all iothread_get_aio_context() callers because they
are not yet converted. The build will fail here.

Every commit must build and pass the tests. This is necessary so that
git-bisect(1) works and it also makes backporting patches much easier
when there are no implicit dependencies between commits.

Please structure this patch series in a way that keeps the build and
tests passing. One way to do this is to give the new API a different
name like:

  AioContext *iothread_get_aio_context(IOThread *iothread);
+ AioContext *iothread_ref_and_get_aio_context(IOThread *iothread, const char *holder);

That name also makes clear that a reference will be taken on the
IOThread. You could then require callers to call iothread_unref()
directly instead of iothread_put_aio_context().

One of the last commits in the patch series could remove the old
iothread_get_aio_context() after everything has been converted.
Re: [PATCH V5 03/13] iothread: tracking iothread users with holder name
Posted by Zhang Chen 1 month ago
On Mon, Mar 9, 2026 at 4:33 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Mar 05, 2026 at 10:24:49PM +0800, Zhang Chen wrote:
> > Introduce iothread_get_aio_context() with a 'holder' argument and its
> > counterpart iothread_put_aio_context().
> >
> > Previously, users of an IOThread's AioContext did not explicitly
> > record their identity, making it difficult to debug which devices or
> > subsystems were pinning an IOThread.
> >
> > This patch enhances the reference counting mechanism by:
> > 1. Automatically incrementing the object reference count when a context
> >    is retrieved.
> > 2. Tracking holders by name using iothread_ref() and iothread_unref().
> >
> > In iothread_instance_finalize(), we now retrieve the source name from
> > the GMainContext to correctly unref the initial internal holder.
> >
> > Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> > ---
> >  include/system/iothread.h |  3 ++-
> >  iothread.c                | 28 ++++++++++++++++++++++++++--
> >  2 files changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/system/iothread.h b/include/system/iothread.h
> > index 21a76bd70d..595abeefbe 100644
> > --- a/include/system/iothread.h
> > +++ b/include/system/iothread.h
> > @@ -47,7 +47,8 @@ DECLARE_INSTANCE_CHECKER(IOThread, IOTHREAD,
> >
> >  char *iothread_get_id(IOThread *iothread);
> >  IOThread *iothread_by_id(const char *id);
> > -AioContext *iothread_get_aio_context(IOThread *iothread);
> > +AioContext *iothread_get_aio_context(IOThread *iothread, const char *holder);
>
> This commit breaks all iothread_get_aio_context() callers because they
> are not yet converted. The build will fail here.
>
> Every commit must build and pass the tests. This is necessary so that
> git-bisect(1) works and it also makes backporting patches much easier
> when there are no implicit dependencies between commits.
>
> Please structure this patch series in a way that keeps the build and
> tests passing. One way to do this is to give the new API a different
> name like:
>
>   AioContext *iothread_get_aio_context(IOThread *iothread);
> + AioContext *iothread_ref_and_get_aio_context(IOThread *iothread, const char *holder);
>
> That name also makes clear that a reference will be taken on the
> IOThread. You could then require callers to call iothread_unref()
> directly instead of iothread_put_aio_context().
>
> One of the last commits in the patch series could remove the old
> iothread_get_aio_context() after everything has been converted.

Make sense, thanks for detailed instructions. I even considered combining
the later parts into a single patch to solve this problem, but that would be
too difficult for the individual modules to review.

Thanks

Chen
Re: [PATCH V5 03/13] iothread: tracking iothread users with holder name
Posted by Stefan Hajnoczi 1 month ago
On Thu, Mar 05, 2026 at 10:24:49PM +0800, Zhang Chen wrote:
> Introduce iothread_get_aio_context() with a 'holder' argument and its
> counterpart iothread_put_aio_context().
> 
> Previously, users of an IOThread's AioContext did not explicitly
> record their identity, making it difficult to debug which devices or
> subsystems were pinning an IOThread.
> 
> This patch enhances the reference counting mechanism by:
> 1. Automatically incrementing the object reference count when a context
>    is retrieved.
> 2. Tracking holders by name using iothread_ref() and iothread_unref().
> 
> In iothread_instance_finalize(), we now retrieve the source name from
> the GMainContext to correctly unref the initial internal holder.
> 
> Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> ---
>  include/system/iothread.h |  3 ++-
>  iothread.c                | 28 ++++++++++++++++++++++++++--
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/include/system/iothread.h b/include/system/iothread.h
> index 21a76bd70d..595abeefbe 100644
> --- a/include/system/iothread.h
> +++ b/include/system/iothread.h
> @@ -47,7 +47,8 @@ DECLARE_INSTANCE_CHECKER(IOThread, IOTHREAD,
>  
>  char *iothread_get_id(IOThread *iothread);
>  IOThread *iothread_by_id(const char *id);
> -AioContext *iothread_get_aio_context(IOThread *iothread);
> +AioContext *iothread_get_aio_context(IOThread *iothread, const char *holder);
> +void iothread_put_aio_context(IOThread *iothread, const char *holder);
>  GMainContext *iothread_get_g_main_context(IOThread *iothread);
>  
>  /*
> diff --git a/iothread.c b/iothread.c
> index 80a8cf4b32..da98fbb9ad 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -172,7 +172,8 @@ static void iothread_init_gcontext(IOThread *iothread, const char *thread_name)
>      g_autofree char *name = g_strdup_printf("%s aio-context", thread_name);
>  
>      iothread->worker_context = g_main_context_new();
> -    source = aio_get_g_source(iothread_get_aio_context(iothread));
> +    /* No need setup itself as the init holder */
> +    source = aio_get_g_source(iothread_get_aio_context(iothread, NULL));

This is cleaner:

  source = aio_get_g_source(iothread->ctx);

That way iothread_get_aio_context() doesn't need a special case for
NULL.

>      g_source_set_name(source, name);
>      g_source_attach(source, iothread->worker_context);
>      g_source_unref(source);
> @@ -362,11 +363,34 @@ char *iothread_get_id(IOThread *iothread)
>      return g_strdup(object_get_canonical_path_component(OBJECT(iothread)));
>  }
>  
> -AioContext *iothread_get_aio_context(IOThread *iothread)
> +AioContext *iothread_get_aio_context(IOThread *iothread, const char *holder)
>  {
> +    /*
> +     * In some cases, iothread user need the ctx to clearup other resource.
> +     * When holder is empty, back to the legacy way.
> +     */
> +    if (holder) {
> +        /*
> +         * This guarantees that the IOThread and its AioContext remain alive
> +         * as long as there is a holder.
> +         */
> +        object_ref(OBJECT(iothread));

Can this be done in iothread_ref()/iothread_unref()? That would ensure
that callers always increment the QOM reference count regardless of
whether the iothread_ref() or iothread_get_aio_context() API is used.

> +
> +        /* Add holder device path to the list */
> +        iothread_ref(iothread, holder);
> +    }
> +
>      return iothread->ctx;
>  }
>  
> +void iothread_put_aio_context(IOThread *iothread, const char *holder)
> +{
> +    object_unref(OBJECT(iothread));
> +
> +    /* Delete holder device path from the list */
> +    iothread_unref(iothread, holder);
> +}
> +
>  static int query_one_iothread(Object *object, void *opaque)
>  {
>      IOThreadInfoList ***tail = opaque;
> -- 
> 2.49.0
> 
Re: [PATCH V5 03/13] iothread: tracking iothread users with holder name
Posted by Zhang Chen 1 month ago
On Mon, Mar 9, 2026 at 4:02 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Thu, Mar 05, 2026 at 10:24:49PM +0800, Zhang Chen wrote:
> > Introduce iothread_get_aio_context() with a 'holder' argument and its
> > counterpart iothread_put_aio_context().
> >
> > Previously, users of an IOThread's AioContext did not explicitly
> > record their identity, making it difficult to debug which devices or
> > subsystems were pinning an IOThread.
> >
> > This patch enhances the reference counting mechanism by:
> > 1. Automatically incrementing the object reference count when a context
> >    is retrieved.
> > 2. Tracking holders by name using iothread_ref() and iothread_unref().
> >
> > In iothread_instance_finalize(), we now retrieve the source name from
> > the GMainContext to correctly unref the initial internal holder.
> >
> > Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> > ---
> >  include/system/iothread.h |  3 ++-
> >  iothread.c                | 28 ++++++++++++++++++++++++++--
> >  2 files changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/system/iothread.h b/include/system/iothread.h
> > index 21a76bd70d..595abeefbe 100644
> > --- a/include/system/iothread.h
> > +++ b/include/system/iothread.h
> > @@ -47,7 +47,8 @@ DECLARE_INSTANCE_CHECKER(IOThread, IOTHREAD,
> >
> >  char *iothread_get_id(IOThread *iothread);
> >  IOThread *iothread_by_id(const char *id);
> > -AioContext *iothread_get_aio_context(IOThread *iothread);
> > +AioContext *iothread_get_aio_context(IOThread *iothread, const char *holder);
> > +void iothread_put_aio_context(IOThread *iothread, const char *holder);
> >  GMainContext *iothread_get_g_main_context(IOThread *iothread);
> >
> >  /*
> > diff --git a/iothread.c b/iothread.c
> > index 80a8cf4b32..da98fbb9ad 100644
> > --- a/iothread.c
> > +++ b/iothread.c
> > @@ -172,7 +172,8 @@ static void iothread_init_gcontext(IOThread *iothread, const char *thread_name)
> >      g_autofree char *name = g_strdup_printf("%s aio-context", thread_name);
> >
> >      iothread->worker_context = g_main_context_new();
> > -    source = aio_get_g_source(iothread_get_aio_context(iothread));
> > +    /* No need setup itself as the init holder */
> > +    source = aio_get_g_source(iothread_get_aio_context(iothread, NULL));
>
> This is cleaner:
>
>   source = aio_get_g_source(iothread->ctx);
>
> That way iothread_get_aio_context() doesn't need a special case for
> NULL.

OK for here. But according to the following patch we try to use the
NULL to handle
problems that cannot be easily solved using ref/unref or get/put.
For example the patch 7 and 9.  We can discuss the details in that mail threads.

>
> >      g_source_set_name(source, name);
> >      g_source_attach(source, iothread->worker_context);
> >      g_source_unref(source);
> > @@ -362,11 +363,34 @@ char *iothread_get_id(IOThread *iothread)
> >      return g_strdup(object_get_canonical_path_component(OBJECT(iothread)));
> >  }
> >
> > -AioContext *iothread_get_aio_context(IOThread *iothread)
> > +AioContext *iothread_get_aio_context(IOThread *iothread, const char *holder)
> >  {
> > +    /*
> > +     * In some cases, iothread user need the ctx to clearup other resource.
> > +     * When holder is empty, back to the legacy way.
> > +     */
> > +    if (holder) {
> > +        /*
> > +         * This guarantees that the IOThread and its AioContext remain alive
> > +         * as long as there is a holder.
> > +         */
> > +        object_ref(OBJECT(iothread));
>
> Can this be done in iothread_ref()/iothread_unref()? That would ensure
> that callers always increment the QOM reference count regardless of
> whether the iothread_ref() or iothread_get_aio_context() API is used.

Sure, will move it into iothread_ref()/iothread_unref().

Thanks

Chen

>
> > +
> > +        /* Add holder device path to the list */
> > +        iothread_ref(iothread, holder);
> > +    }
> > +
> >      return iothread->ctx;
> >  }
> >
> > +void iothread_put_aio_context(IOThread *iothread, const char *holder)
> > +{
> > +    object_unref(OBJECT(iothread));
> > +
> > +    /* Delete holder device path from the list */
> > +    iothread_unref(iothread, holder);
> > +}
> > +
> >  static int query_one_iothread(Object *object, void *opaque)
> >  {
> >      IOThreadInfoList ***tail = opaque;
> > --
> > 2.49.0
> >