Re: [PATCH V11 03/15] iothread: track users with holder name

Zhang Chen <[email protected]> writes:

> Introduce iothread_ref_and_get_aio_context() with a holder argument
> and its counterpart iothread_put_aio_context().
>
> Previously, users of an IOThread AioContext did not explicitly record
> their identity, making it difficult to debug which devices or
> subsystems were pinning an IOThread.
>
> Registering a holder takes an IOThread object reference so that the
> IOThread and its AioContext stay alive until the matching put
> operation. Document the ownership and BQL requirements.
>
> Signed-off-by: Zhang Chen <[email protected]>
> ---
>  include/system/iothread.h | 21 +++++++++++++++++++++
>  iothread.c                | 23 ++++++++++++++++++++++-
>  2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/include/system/iothread.h b/include/system/iothread.h
> index b8aeb32b0e..b6664e7a38 100644
> --- a/include/system/iothread.h
> +++ b/include/system/iothread.h
> @@ -67,6 +67,27 @@ 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);
> +
> +/*
> + * Register @holder and return @iothread's AioContext.  The holder is copied,
> + * and a reference is taken on @iothread so that both the IOThread and its
> + * AioContext remain alive.
> + *
> + * The caller must eventually call iothread_put_aio_context() with an
> + * equivalent holder.  This function is not thread-safe and must be called
> + * under the Big QEMU Lock (BQL).
> + */
> +AioContext *iothread_ref_and_get_aio_context(IOThread *iothread,
> +                                             const IOThreadHolder *holder);
> +
> +/*
> + * Unregister @holder and release the corresponding reference on @iothread.
> + * Calling this function without a matching
> + * iothread_ref_and_get_aio_context() call is a programming error.
> + *
> + * This function is not thread-safe and must be called under the BQL.
> + */
> +void iothread_put_aio_context(IOThread *iothread, const IOThreadHolder *holder);
>  GMainContext *iothread_get_g_main_context(IOThread *iothread);
>  
>  /*
> diff --git a/iothread.c b/iothread.c
> index 2a4c92e08b..0cc8344ee6 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -33,6 +33,11 @@ void iothread_ref(IOThread *iothread, const IOThreadHolder *holder)
>      assert(holder);
>  
>      QAPI_LIST_PREPEND(iothread->holders, QAPI_CLONE(IOThreadHolder, holder));
> +    /*
> +     * This guarantees that the IOThread and its AioContext remain alive
> +     * as long as there is a holder.
> +     */
> +    object_ref(OBJECT(iothread));
>  }
>  
>  static int iothread_holder_compare(const IOThreadHolder *holder_a,
> @@ -78,6 +83,7 @@ void iothread_unref(IOThread *iothread, const IOThreadHolder *holder)
>              *prev = curr->next;
>              curr->next = NULL;
>              qapi_free_IOThreadHolderList(curr);
> +            object_unref(OBJECT(iothread));
>              return;
>          }
>          prev = &curr->next;
> @@ -199,7 +205,7 @@ 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));
> +    source = aio_get_g_source(iothread->ctx);
>      g_source_set_name(source, name);
>      g_source_attach(source, iothread->worker_context);
>      g_source_unref(source);
> @@ -421,6 +427,21 @@ AioContext *iothread_get_aio_context(IOThread *iothread)
>      return iothread->ctx;
>  }
>  
> +AioContext *iothread_ref_and_get_aio_context(IOThread *iothread,
> +                                             const IOThreadHolder *holder)
> +{
> +    /* Add IOThreadHolder to the list */
> +    iothread_ref(iothread, holder);
> +
> +    return iothread->ctx;
> +}
> +
> +void iothread_put_aio_context(IOThread *iothread, const IOThreadHolder *holder)

Should we name this "unref" rather than "put"?  Perhaps Stefan has an
opinion.

> +{
> +    /* Delete IOThreadHolder from the list */
> +    iothread_unref(iothread, holder);
> +}
> +
>  static int query_one_iothread(Object *object, void *opaque)
>  {
>      IOThreadInfoList ***tail = opaque;