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) > +# > # @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,
