| Message ID | 20230918161604.1400051-2-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-blk: add iothread-vq-mapping parameter | expand |
Stefan Hajnoczi <stefanha@redhat.com> writes: > virtio-blk and virtio-scsi devices will need a way to specify the > mapping between IOThreads and virtqueues. At the moment all virtqueues > are assigned to a single IOThread or the main loop. This single thread > can be a CPU bottleneck, so it is necessary to allow finer-grained > assignment to spread the load. > > Introduce DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST() so devices can take a > parameter that maps virtqueues to IOThreads. The command-line syntax for > this new property is as follows: > > --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]}' > > IOThreads are specified by name and virtqueues are specified by 0-based > index. > > It will be common to simply assign virtqueues round-robin across a set > of IOThreads. A convenient syntax that does not require specifying > individual virtqueue indices is available: > > --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]}' > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > qapi/virtio.json | 30 ++++++++++++++++++ > include/hw/qdev-properties-system.h | 4 +++ > hw/core/qdev-properties-system.c | 47 +++++++++++++++++++++++++++++ > 3 files changed, 81 insertions(+) > > diff --git a/qapi/virtio.json b/qapi/virtio.json > index e6dcee7b83..cb341ae596 100644 > --- a/qapi/virtio.json > +++ b/qapi/virtio.json > @@ -928,3 +928,33 @@ > 'data': { 'path': 'str', 'queue': 'uint16', '*index': 'uint16' }, > 'returns': 'VirtioQueueElement', > 'features': [ 'unstable' ] } > + > +## > +# @IOThreadVirtQueueMapping: > +# > +# Describes the subset of virtqueues assigned to an IOThread. > +# > +# @iothread: the id of IOThread object > +# @vqs: an optional array of virtqueue indices that will be handled by this > +# IOThread. When absent, virtqueues are assigned round-robin across all > +# IOThreadVirtQueueMappings provided. Either all > +# IOThreadVirtQueueMappings must have @vqs or none of them must have it. > +# > +# Since: 8.2 > +# > +## Please format like ## # @IOThreadVirtQueueMapping: # # Describes the subset of virtqueues assigned to an IOThread. # # @iothread: the id of IOThread object # # @vqs: an optional array of virtqueue indices that will be handled by # this IOThread. When absent, virtqueues are assigned round-robin # across all IOThreadVirtQueueMappings provided. Either all # IOThreadVirtQueueMappings must have @vqs or none of them must # have it. # # Since: 8.2 ## to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments to conform to current conventions). > + > +{ 'struct': 'IOThreadVirtQueueMapping', > + 'data': { 'iothread': 'str', '*vqs': ['uint16'] } } > + > +## > +# @IOThreadVirtQueueMappings: > +# > +# IOThreadVirtQueueMapping list. This struct is not actually used but the > +# IOThreadVirtQueueMappingList type it generates is! Two spaces between sentences for consistency, please. Doc comments are QMP reference documentation for users. Does this paragraph belong there? > +# > +# Since: 8.2 > +## > + > +{ 'struct': 'IOThreadVirtQueueMappings', > + 'data': { 'mappings': ['IOThreadVirtQueueMapping'] } } > diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h > index 0ac327ae60..c526e502c8 100644 > --- a/include/hw/qdev-properties-system.h > +++ b/include/hw/qdev-properties-system.h > @@ -22,6 +22,7 @@ extern const PropertyInfo qdev_prop_audiodev; > extern const PropertyInfo qdev_prop_off_auto_pcibar; > extern const PropertyInfo qdev_prop_pcie_link_speed; > extern const PropertyInfo qdev_prop_pcie_link_width; > +extern const PropertyInfo qdev_prop_iothread_vq_mapping_list; > > #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d) \ > DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t) > @@ -73,5 +74,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width; > #define DEFINE_PROP_UUID_NODEFAULT(_name, _state, _field) \ > DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID) > > +#define DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST(_name, _state, _field) \ > + DEFINE_PROP(_name, _state, _field, qdev_prop_iothread_vq_mapping_list, \ > + IOThreadVirtQueueMappingList *) > > #endif > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index 6d5d43eda2..831796e106 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -18,6 +18,7 @@ > #include "qapi/qapi-types-block.h" > #include "qapi/qapi-types-machine.h" > #include "qapi/qapi-types-migration.h" > +#include "qapi/qapi-visit-virtio.h" > #include "qapi/qmp/qerror.h" > #include "qemu/ctype.h" > #include "qemu/cutils.h" > @@ -1147,3 +1148,49 @@ const PropertyInfo qdev_prop_uuid = { > .set = set_uuid, > .set_default_value = set_default_uuid_auto, > }; > + > +/* --- IOThreadVirtQueueMappingList --- */ > + > +static void get_iothread_vq_mapping_list(Object *obj, Visitor *v, > + const char *name, void *opaque, Error **errp) > +{ > + IOThreadVirtQueueMappingList **prop_ptr = > + object_field_prop_ptr(obj, opaque); > + IOThreadVirtQueueMappingList *list = *prop_ptr; > + > + visit_type_IOThreadVirtQueueMappingList(v, name, &list, errp); @list is a copy of the property value. Why do you need to pass a copy to visit_type_IOThreadVirtQueueMappingList()? > +} > + > +static void set_iothread_vq_mapping_list(Object *obj, Visitor *v, > + const char *name, void *opaque, Error **errp) > +{ > + IOThreadVirtQueueMappingList **prop_ptr = > + object_field_prop_ptr(obj, opaque); > + IOThreadVirtQueueMappingList *list; > + > + if (!visit_type_IOThreadVirtQueueMappingList(v, name, &list, errp)) { > + return; > + } > + > + qapi_free_IOThreadVirtQueueMappingList(*prop_ptr); > + *prop_ptr = list; > +} > + > +static void release_iothread_vq_mapping_list(Object *obj, > + const char *name, void *opaque) > +{ > + IOThreadVirtQueueMappingList **prop_ptr = > + object_field_prop_ptr(obj, opaque); > + > + qapi_free_IOThreadVirtQueueMappingList(*prop_ptr); > + *prop_ptr = NULL; > +} > + > +const PropertyInfo qdev_prop_iothread_vq_mapping_list = { > + .name = "IOThreadVirtQueueMappingList", > + .description = "IOThread virtqueue mapping list [{\"iothread\":\"<id>\", " > + "\"vqs\":[1,2,3,...]},...]", > + .get = get_iothread_vq_mapping_list, > + .set = set_iothread_vq_mapping_list, > + .release = release_iothread_vq_mapping_list, > +};
Am 18.09.2023 um 18:16 hat Stefan Hajnoczi geschrieben: > virtio-blk and virtio-scsi devices will need a way to specify the > mapping between IOThreads and virtqueues. At the moment all virtqueues > are assigned to a single IOThread or the main loop. This single thread > can be a CPU bottleneck, so it is necessary to allow finer-grained > assignment to spread the load. > > Introduce DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST() so devices can take a > parameter that maps virtqueues to IOThreads. The command-line syntax for > this new property is as follows: > > --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]}' > > IOThreads are specified by name and virtqueues are specified by 0-based > index. > > It will be common to simply assign virtqueues round-robin across a set > of IOThreads. A convenient syntax that does not require specifying > individual virtqueue indices is available: > > --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]}' > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> When testing this, Qing Wang noticed that "info qtree" crashes. This is because the string output visitor doesn't support structs. I suppose IOThreadVirtQueueMapping is the first struct type that is used in a qdev property type. So we'll probably have to add some kind of struct support to the string output visitor before we can apply this. Even if it's as stupid as just printing "<struct IOThreadVirtQueueMapping>" without actually displaying the value. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 18.09.2023 um 18:16 hat Stefan Hajnoczi geschrieben: >> virtio-blk and virtio-scsi devices will need a way to specify the >> mapping between IOThreads and virtqueues. At the moment all virtqueues >> are assigned to a single IOThread or the main loop. This single thread >> can be a CPU bottleneck, so it is necessary to allow finer-grained >> assignment to spread the load. >> >> Introduce DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST() so devices can take a >> parameter that maps virtqueues to IOThreads. The command-line syntax for >> this new property is as follows: >> >> --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]}' >> >> IOThreads are specified by name and virtqueues are specified by 0-based >> index. >> >> It will be common to simply assign virtqueues round-robin across a set >> of IOThreads. A convenient syntax that does not require specifying >> individual virtqueue indices is available: >> >> --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]}' >> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > When testing this, Qing Wang noticed that "info qtree" crashes. This is > because the string output visitor doesn't support structs. I suppose > IOThreadVirtQueueMapping is the first struct type that is used in a qdev > property type. > > So we'll probably have to add some kind of struct support to the string > output visitor before we can apply this. Even if it's as stupid as just > printing "<struct IOThreadVirtQueueMapping>" without actually displaying > the value. The string visitors have been nothing but trouble. For input, we can now use keyval_parse() and the QObject input visitor instead. Comes with restrictions, but I'd argue it's a more solid base than the string input visitor. Perhaps we can do something similar for output: create a suitable formatter for use it with the QObject output visitor, replacing the string output visitor.
On Mon, Dec 11, 2023 at 04:32:06PM +0100, Markus Armbruster wrote: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 18.09.2023 um 18:16 hat Stefan Hajnoczi geschrieben: > >> virtio-blk and virtio-scsi devices will need a way to specify the > >> mapping between IOThreads and virtqueues. At the moment all virtqueues > >> are assigned to a single IOThread or the main loop. This single thread > >> can be a CPU bottleneck, so it is necessary to allow finer-grained > >> assignment to spread the load. > >> > >> Introduce DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST() so devices can take a > >> parameter that maps virtqueues to IOThreads. The command-line syntax for > >> this new property is as follows: > >> > >> --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]}' > >> > >> IOThreads are specified by name and virtqueues are specified by 0-based > >> index. > >> > >> It will be common to simply assign virtqueues round-robin across a set > >> of IOThreads. A convenient syntax that does not require specifying > >> individual virtqueue indices is available: > >> > >> --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]}' > >> > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > When testing this, Qing Wang noticed that "info qtree" crashes. This is > > because the string output visitor doesn't support structs. I suppose > > IOThreadVirtQueueMapping is the first struct type that is used in a qdev > > property type. > > > > So we'll probably have to add some kind of struct support to the string > > output visitor before we can apply this. Even if it's as stupid as just > > printing "<struct IOThreadVirtQueueMapping>" without actually displaying > > the value. > > The string visitors have been nothing but trouble. > > For input, we can now use keyval_parse() and the QObject input visitor > instead. Comes with restrictions, but I'd argue it's a more solid base > than the string input visitor. > > Perhaps we can do something similar for output: create a suitable > formatter for use it with the QObject output visitor, replacing the > string output visitor. I sent an initial patch that just shows "<omitted>" but would like to work on a proper solution with your input. From what I've seen StringOutputVisitor is used in several places in QEMU. "info qtree" calls it through object_property_print() to print individual qdev properties. I don't understand the requirements of the other callers, but object_property_print() wants to return a single string without newlines. QObjectOutputVisitor produces a QObject. That could be formatted as JSON using qobject_to_json_pretty()? The pretty JSON would contain newlines and existing callers such as "info qtree" may need to scan the output and insert indentation. The goal would be to keep the output unchanged as far as possible and to emit JSON for structs and lists. What do you think about this approach? Stefan
Am 11.12.2023 um 16:32 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 18.09.2023 um 18:16 hat Stefan Hajnoczi geschrieben: > >> virtio-blk and virtio-scsi devices will need a way to specify the > >> mapping between IOThreads and virtqueues. At the moment all virtqueues > >> are assigned to a single IOThread or the main loop. This single thread > >> can be a CPU bottleneck, so it is necessary to allow finer-grained > >> assignment to spread the load. > >> > >> Introduce DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST() so devices can take a > >> parameter that maps virtqueues to IOThreads. The command-line syntax for > >> this new property is as follows: > >> > >> --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]}' > >> > >> IOThreads are specified by name and virtqueues are specified by 0-based > >> index. > >> > >> It will be common to simply assign virtqueues round-robin across a set > >> of IOThreads. A convenient syntax that does not require specifying > >> individual virtqueue indices is available: > >> > >> --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]}' > >> > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > When testing this, Qing Wang noticed that "info qtree" crashes. This is > > because the string output visitor doesn't support structs. I suppose > > IOThreadVirtQueueMapping is the first struct type that is used in a qdev > > property type. > > > > So we'll probably have to add some kind of struct support to the string > > output visitor before we can apply this. Even if it's as stupid as just > > printing "<struct IOThreadVirtQueueMapping>" without actually displaying > > the value. > > The string visitors have been nothing but trouble. > > For input, we can now use keyval_parse() and the QObject input visitor > instead. Comes with restrictions, but I'd argue it's a more solid base > than the string input visitor. > > Perhaps we can do something similar for output: create a suitable > formatter for use it with the QObject output visitor, replacing the > string output visitor. dump_qobject() frm block/qapi.c? But I wouldn't like to block this series for a major rework of "info qtree", so I think we need to make sure that the string input visitor doesn't crash at least. Kevin
Stefan Hajnoczi <stefanha@redhat.com> writes: > On Mon, Dec 11, 2023 at 04:32:06PM +0100, Markus Armbruster wrote: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 18.09.2023 um 18:16 hat Stefan Hajnoczi geschrieben: >> >> virtio-blk and virtio-scsi devices will need a way to specify the >> >> mapping between IOThreads and virtqueues. At the moment all virtqueues >> >> are assigned to a single IOThread or the main loop. This single thread >> >> can be a CPU bottleneck, so it is necessary to allow finer-grained >> >> assignment to spread the load. >> >> >> >> Introduce DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST() so devices can take a >> >> parameter that maps virtqueues to IOThreads. The command-line syntax for >> >> this new property is as follows: >> >> >> >> --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]}' >> >> >> >> IOThreads are specified by name and virtqueues are specified by 0-based >> >> index. >> >> >> >> It will be common to simply assign virtqueues round-robin across a set >> >> of IOThreads. A convenient syntax that does not require specifying >> >> individual virtqueue indices is available: >> >> >> >> --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]}' >> >> >> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >> > >> > When testing this, Qing Wang noticed that "info qtree" crashes. This is >> > because the string output visitor doesn't support structs. I suppose >> > IOThreadVirtQueueMapping is the first struct type that is used in a qdev >> > property type. >> > >> > So we'll probably have to add some kind of struct support to the string >> > output visitor before we can apply this. Even if it's as stupid as just >> > printing "<struct IOThreadVirtQueueMapping>" without actually displaying >> > the value. >> >> The string visitors have been nothing but trouble. >> >> For input, we can now use keyval_parse() and the QObject input visitor >> instead. Comes with restrictions, but I'd argue it's a more solid base >> than the string input visitor. >> >> Perhaps we can do something similar for output: create a suitable >> formatter for use it with the QObject output visitor, replacing the >> string output visitor. > > I sent an initial patch that just shows "<omitted>" but would like to > work on a proper solution with your input. > > From what I've seen StringOutputVisitor is used in several places in > QEMU. "info qtree" calls it through object_property_print() to print > individual qdev properties. I don't understand the requirements of the > other callers, but object_property_print() wants to return a single > string without newlines. string_output_visitor_new(): * hmp_info_migrate(): format a list of integers, then print it like monitor_printf(mon, "postcopy vcpu blocktime: %s\n", str); One element per vCPU; can produce a long line. * netfilter_print_info(): format the property values of a NetFilterState, then print each like monitor_printf(mon, ",%s=%s", prop->name, str); * object_property_print(): format a property value of an object, return the string. Function is misnamed. object_property_format() or object_property_to_string() would be better. Just one caller: qdev_print_props(), helper for hmp_info_qtree(). Prints the string like qdev_printf("%s = %s\n", props->name, *value ? value : "<null>"); where qdev_printf() is a macro wrapping monitor_printf(). This one passes human=true, unlike the others. More on that below. * hmp_info_memdev(): format a list of integers, then print it like monitor_printf(mon, " policy: %s\n", HostMemPolicy_str(m->value->policy)); One element per "host node", whatever that may be; might produce a long line. * Tests; not relevant here. hmp_info_migrate() and hmp_info_memdev() use the visitor as a (somewhat cumbersome) helper for printing uint32List and uint16List, respectively. Could do without. The other two display all properties in HMP. Both kind of assume the string visitor produces no newlines. I think we could instead use the QObject output visitor, then format the QObject in human-readable form. Might be less efficient, because we create a temporary QObject. Perhaps factor out a single helper first. string_input_visitor_new(), for good measure: * hmp_migrate_set_parameter(): parse an uint8_t, uint32, size_t, bool, str, or QAPI enum from a string. * object_property_parse(): parse a property value from a string, and assign it to the property. Calling this object_property_set_from_string() would be better. Callers: - object_apply_global_props(): applying compatibility properties (defined in C) and defauls set with -global (given by user). - object_set_propv(): helper for convenience functions to set multiple properties in C. - hmp_qom_set(): set the property value when not JSON (-j is off). - object_parse_property_opt(), for accelerator_set_property(), which processes the argument of -accel. - x86_cpu_apply_props() and x86_cpu_apply_version_props(): apply properties defined in C. The property settings defined in C could just as well use QObject input visitor instead. Might be less efficient, because we create a temporary QObject. The property settings that come from the user are ABI. Reminder: supports only scalars and lists of small integers. Supporting structured values containing strings would involve creating syntax that's basically reinventing JSON. I think qom-set demonstrates how to support structured values: just use JSON. > QObjectOutputVisitor produces a QObject. That could be formatted as > JSON using qobject_to_json_pretty()? > > The pretty JSON would contain newlines and existing callers such as > "info qtree" may need to scan the output and insert indentation. With pretty=false, the JSON formatter produces a string without newlines. With pretty=true, it adds newlines and indentation. Instead of scanning the formatted string to change its indentation, we could perhaps pass indentation instructions to the JSON formatter. > The goal would be to keep the output unchanged as far as possible and to > emit JSON for structs and lists. That's basically JSON, except we print 'str' values without quotes and escapes (resulting in ambiguity when they contain {[]}, and even more fun when they contain control characters), plus a few more largely gratuitous differences. With human=true (advertized as "a bit easier for humans to read"), we get strings with quotes (still no escapes), and a tweaked set of gratuitous differences. > What do you think about this approach? Format as JSON and call it a day? I figure the only thing of value we'd lose is displaying integers both decimal and hex. In some, but not all places.
On Sat, Oct 14, 2023 at 09:35:14AM +0200, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > > +## > > +# @IOThreadVirtQueueMappings: > > +# > > +# IOThreadVirtQueueMapping list. This struct is not actually used but the > > +# IOThreadVirtQueueMappingList type it generates is! > > Two spaces between sentences for consistency, please. > > Doc comments are QMP reference documentation for users. Does this > paragraph belong there? Someone might wonder why a type exists that is never used, so I think including this in the documentation is acceptable. My comment is mostly intended to stop someone from removing this "unused" type from the schema though. If there is a better way to do that I can do that instead. Stefan
Am 19.12.2023 um 16:13 hat Stefan Hajnoczi geschrieben: > On Sat, Oct 14, 2023 at 09:35:14AM +0200, Markus Armbruster wrote: > > Stefan Hajnoczi <stefanha@redhat.com> writes: > > > +## > > > +# @IOThreadVirtQueueMappings: > > > +# > > > +# IOThreadVirtQueueMapping list. This struct is not actually used but the > > > +# IOThreadVirtQueueMappingList type it generates is! > > > > Two spaces between sentences for consistency, please. > > > > Doc comments are QMP reference documentation for users. Does this > > paragraph belong there? > > Someone might wonder why a type exists that is never used, so I think > including this in the documentation is acceptable. I seem to remember that we had a similar remark elsewhere, but maybe it doesn't exist any more today? > My comment is mostly intended to stop someone from removing this > "unused" type from the schema though. If there is a better way to do > that I can do that instead. Won't the QAPI generator or the compiler stop them soon enough? Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 19.12.2023 um 16:13 hat Stefan Hajnoczi geschrieben: >> On Sat, Oct 14, 2023 at 09:35:14AM +0200, Markus Armbruster wrote: >> > Stefan Hajnoczi <stefanha@redhat.com> writes: >> > > +## >> > > +# @IOThreadVirtQueueMappings: >> > > +# >> > > +# IOThreadVirtQueueMapping list. This struct is not actually used but the >> > > +# IOThreadVirtQueueMappingList type it generates is! >> > >> > Two spaces between sentences for consistency, please. >> > >> > Doc comments are QMP reference documentation for users. Does this >> > paragraph belong there? >> >> Someone might wonder why a type exists that is never used, so I think >> including this in the documentation is acceptable. > > I seem to remember that we had a similar remark elsewhere, but maybe it > doesn't exist any more today? Working up more context... alright, now I see. When the QAPI schema defines a type 'T', the QAPI generator generates code and documentation for it whether it is used in the schema or not. We occasionally use this to generate types with QAPI goodies for purely internal use. In other words, the generator assumes there is a use of T in C code. However, the QAPI generator generates code for ['T'] only if there's a use of ['T'] in the schema. This is because it's actually needed only for about one in seven types. See commit 9f08c8ec738 (qapi: Lazy creation of array types). Once in a blue moon, ['T'] is only used in C code. The QAPI generator won't generate code for it then, and the C code won't compile. Then we have to add a dummy use to the schema to force the array into existence. Not exactly elegant, but it works. I can see two blue moons in the schema before this patch. In qapi/block-core.json: ## # @DummyBlockCoreForceArrays: # # Not used by QMP; hack to let us use BlockGraphInfoList internally # # Since: 8.0 ## { 'struct': 'DummyBlockCoreForceArrays', 'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } } It's called Dummy<NameOfModule>ForceArrays, it's at the end of the file, and it collects all the arrays being forced into existence. In qapi/machine.json: ## # @DummyForceArrays: # # Not used by QMP; hack to let us use X86CPUFeatureWordInfoList # internally # # Since: 2.5 ## { 'struct': 'DummyForceArrays', 'data': { 'unused': ['X86CPUFeatureWordInfo'] } } Less good: it has a clash-prone name, and it's not at the end of the file. It was the first use of this trick, and we still had a single schema file back then. I recommend you do yours just like the first one. >> My comment is mostly intended to stop someone from removing this >> "unused" type from the schema though. If there is a better way to do >> that I can do that instead. > > Won't the QAPI generator or the compiler stop them soon enough? The compiler would.
diff --git a/qapi/virtio.json b/qapi/virtio.json index e6dcee7b83..cb341ae596 100644 --- a/qapi/virtio.json +++ b/qapi/virtio.json @@ -928,3 +928,33 @@ 'data': { 'path': 'str', 'queue': 'uint16', '*index': 'uint16' }, 'returns': 'VirtioQueueElement', 'features': [ 'unstable' ] } + +## +# @IOThreadVirtQueueMapping: +# +# Describes the subset of virtqueues assigned to an IOThread. +# +# @iothread: the id of IOThread object +# @vqs: an optional array of virtqueue indices that will be handled by this +# IOThread. When absent, virtqueues are assigned round-robin across all +# IOThreadVirtQueueMappings provided. Either all +# IOThreadVirtQueueMappings must have @vqs or none of them must have it. +# +# Since: 8.2 +# +## + +{ 'struct': 'IOThreadVirtQueueMapping', + 'data': { 'iothread': 'str', '*vqs': ['uint16'] } } + +## +# @IOThreadVirtQueueMappings: +# +# IOThreadVirtQueueMapping list. This struct is not actually used but the +# IOThreadVirtQueueMappingList type it generates is! +# +# Since: 8.2 +## + +{ 'struct': 'IOThreadVirtQueueMappings', + 'data': { 'mappings': ['IOThreadVirtQueueMapping'] } } diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h index 0ac327ae60..c526e502c8 100644 --- a/include/hw/qdev-properties-system.h +++ b/include/hw/qdev-properties-system.h @@ -22,6 +22,7 @@ extern const PropertyInfo qdev_prop_audiodev; extern const PropertyInfo qdev_prop_off_auto_pcibar; extern const PropertyInfo qdev_prop_pcie_link_speed; extern const PropertyInfo qdev_prop_pcie_link_width; +extern const PropertyInfo qdev_prop_iothread_vq_mapping_list; #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d) \ DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t) @@ -73,5 +74,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width; #define DEFINE_PROP_UUID_NODEFAULT(_name, _state, _field) \ DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID) +#define DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST(_name, _state, _field) \ + DEFINE_PROP(_name, _state, _field, qdev_prop_iothread_vq_mapping_list, \ + IOThreadVirtQueueMappingList *) #endif diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 6d5d43eda2..831796e106 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -18,6 +18,7 @@ #include "qapi/qapi-types-block.h" #include "qapi/qapi-types-machine.h" #include "qapi/qapi-types-migration.h" +#include "qapi/qapi-visit-virtio.h" #include "qapi/qmp/qerror.h" #include "qemu/ctype.h" #include "qemu/cutils.h" @@ -1147,3 +1148,49 @@ const PropertyInfo qdev_prop_uuid = { .set = set_uuid, .set_default_value = set_default_uuid_auto, }; + +/* --- IOThreadVirtQueueMappingList --- */ + +static void get_iothread_vq_mapping_list(Object *obj, Visitor *v, + const char *name, void *opaque, Error **errp) +{ + IOThreadVirtQueueMappingList **prop_ptr = + object_field_prop_ptr(obj, opaque); + IOThreadVirtQueueMappingList *list = *prop_ptr; + + visit_type_IOThreadVirtQueueMappingList(v, name, &list, errp); +} + +static void set_iothread_vq_mapping_list(Object *obj, Visitor *v, + const char *name, void *opaque, Error **errp) +{ + IOThreadVirtQueueMappingList **prop_ptr = + object_field_prop_ptr(obj, opaque); + IOThreadVirtQueueMappingList *list; + + if (!visit_type_IOThreadVirtQueueMappingList(v, name, &list, errp)) { + return; + } + + qapi_free_IOThreadVirtQueueMappingList(*prop_ptr); + *prop_ptr = list; +} + +static void release_iothread_vq_mapping_list(Object *obj, + const char *name, void *opaque) +{ + IOThreadVirtQueueMappingList **prop_ptr = + object_field_prop_ptr(obj, opaque); + + qapi_free_IOThreadVirtQueueMappingList(*prop_ptr); + *prop_ptr = NULL; +} + +const PropertyInfo qdev_prop_iothread_vq_mapping_list = { + .name = "IOThreadVirtQueueMappingList", + .description = "IOThread virtqueue mapping list [{\"iothread\":\"<id>\", " + "\"vqs\":[1,2,3,...]},...]", + .get = get_iothread_vq_mapping_list, + .set = set_iothread_vq_mapping_list, + .release = release_iothread_vq_mapping_list, +};
virtio-blk and virtio-scsi devices will need a way to specify the mapping between IOThreads and virtqueues. At the moment all virtqueues are assigned to a single IOThread or the main loop. This single thread can be a CPU bottleneck, so it is necessary to allow finer-grained assignment to spread the load. Introduce DEFINE_PROP_IOTHREAD_VQ_MAPPING_LIST() so devices can take a parameter that maps virtqueues to IOThreads. The command-line syntax for this new property is as follows: --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0","vqs":[0,1,2]},...]}' IOThreads are specified by name and virtqueues are specified by 0-based index. It will be common to simply assign virtqueues round-robin across a set of IOThreads. A convenient syntax that does not require specifying individual virtqueue indices is available: --device '{"driver":"foo","iothread-vq-mapping":[{"iothread":"iothread0"},{"iothread":"iothread1"},...]}' Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- qapi/virtio.json | 30 ++++++++++++++++++ include/hw/qdev-properties-system.h | 4 +++ hw/core/qdev-properties-system.c | 47 +++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+)