On 4/29/21 5:43 PM, Jonathon Jongsma wrote:
> On Thu, 2021-04-29 at 14:12 +0300, Gavi Teitz wrote:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1925363&data=04%7C01%7Cgavi%40nvidia.com%7C3a0a9dc902bd4ddb90fa08d90b1d3ac9%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637553042452946162%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=0xd9ZcmXQJPT%2FeqT%2Fs%2B0KOc5HnMN447pE6r7V6bc%2B98%3D&reserved=0
>>
>> Add support for setting the page-per-vq flag, which is important for
>> vdpa with vhost-user performance.
>>
>> Signed-off-by: Gavi Teitz <g...@nvidia.com>
>>
> [snip]
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 9d98f48..0350fde 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -10446,6 +10446,7 @@ virDomainNetDefParseXML(virDomainXMLOption
>> *xmlopt,
>> g_autofree char *queues = NULL;
>> g_autofree char *rx_queue_size = NULL;
>> g_autofree char *tx_queue_size = NULL;
>> + g_autofree char *page_per_vq = NULL;
>> g_autofree char *filter = NULL;
>> g_autofree char *internal = NULL;
>> g_autofree char *mode = NULL;
>> @@ -10615,6 +10616,7 @@ virDomainNetDefParseXML(virDomainXMLOption
>> *xmlopt,
>> queues = virXMLPropString(cur, "queues");
>> rx_queue_size = virXMLPropString(cur,
>> "rx_queue_size");
>> tx_queue_size = virXMLPropString(cur,
>> "tx_queue_size");
>> + page_per_vq = virXMLPropString(cur, "page_per_vq");
>>
>> if (virDomainVirtioOptionsParseXML(cur, &def-
>>> virtio) < 0)
>> goto error;
>> @@ -11041,6 +11043,15 @@ virDomainNetDefParseXML(virDomainXMLOption
>> *xmlopt,
>> }
>> def->driver.virtio.tx_queue_size = q;
>> }
>> + if (page_per_vq) {
>> + if ((val = virTristateSwitchTypeFromString(page_per_vq))
>> <= 0) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("unknown page_per_vq mode '%s'"),
>> + page_per_vq);
>> + goto error;
>> + }
>> + def->driver.virtio.page_per_vq = val;
>> + }
> Tim Wiederhake has been doing a bunch of refactoring work in this area
> changing code like this to use the newish virXMLProp* functions.
>
> So I think this whole chunk could instead be something like:
>
> if (virXMLPropTristateSwitch(cur, "page_per_vq", VIR_XML_PROP_NONE,
> &def->driver.virtio.page_per_vq) < 0)
> return -1;
>
>
> (untested)
>
> Jonathon
>
Assigning def->driver.virtio.page_per_vq in the initial parsing loop
with the suggested lines would prevent the assignment from being
conditioned upon virDomainNetIsVirtioModel() as it currently is.
Is it not necessary to enforce this condition, or should I instead do
the assignment as I currently do, after the parsing loop and under the
condition, and use virXPathNode("./driver", ctxt) to get the node?
