Zhang Chen <[email protected]> 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 <[email protected]>
> ---
> 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:
> #
