[libvirt] [PATCH 0/3] Introduce support for rx_queue_size
This series is implementing support for something, that is not merged yet to qemu. Therefore I won't push it until qemu side patch (which is in queue as qemu is preparing for 2.7 release) is in [1]. However, review is welcome! 1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02029.html Michal Privoznik (3): conf: Add support for virtio-net.rx_queue_size qemu_capabilities: Introduce virtio-net-*.rx_queue_size qemu: Implement virtio-net rx_queue_size docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 16 ++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 8 ++++++ src/qemu/qemu_domain.c | 7 ++++++ .../qemuxml2argv-net-virtio-rxqueuesize.args | 25 +++++++++++++++++++ .../qemuxml2argv-net-virtio-rxqueuesize.xml | 29 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 11 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml -- 2.8.4
https://bugzilla.redhat.com/show_bug.cgi?id=1366989 Qemu grew yet another virtio-net tunable [1]. It basically allows users to set the size of RX virtio ring. But because virtio-net uses two separate ring buffers to pass data from/to guest they named it explicitly rx_queue_size. We should expose it in our XML too. 1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02029.html Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 16 ++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 7 ++++++ .../qemuxml2argv-net-virtio-rxqueuesize.xml | 29 ++++++++++++++++++++++ 6 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bfbb0f2..6642dc8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4604,7 +4604,7 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rxqueuesize='128'> <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off' mrg_rxbuf='off'/> <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> </driver> @@ -4720,6 +4720,11 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">virtio-net since 1.0.6 (QEMU and KVM only)</span> <span class="since">vhost-user since 1.2.17 (QEMU and KVM only)</span> </dd> + <dt><code>rxqueuesize</code></dt> + <dd> + The optional <code>rxqueuesize</code> attribute controls + the size of virtio ring for each queue as described above. + </dd> </dl> <p> Offloading options for the host and guest can be configured using diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 052f28c..4b89a86 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2529,6 +2529,11 @@ </attribute> </optional> <optional> + <attribute name='rxqueuesize'> + <ref name='positiveInteger'/> + </attribute> + </optional> + <optional> <attribute name="txmode"> <choice> <value>iothread</value> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 14d4f7d..08cde19 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8911,6 +8911,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *ioeventfd = NULL; char *event_idx = NULL; char *queues = NULL; + char *rxqueuesize = NULL; char *str = NULL; char *filter = NULL; char *internal = NULL; @@ -9083,6 +9084,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, ioeventfd = virXMLPropString(cur, "ioeventfd"); event_idx = virXMLPropString(cur, "event_idx"); queues = virXMLPropString(cur, "queues"); + rxqueuesize = virXMLPropString(cur, "rxqueuesize"); } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) { if (filter) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -9466,6 +9468,17 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, if (q > 1) def->driver.virtio.queues = q; } + if (rxqueuesize) { + unsigned int q; + if (virStrToLong_uip(rxqueuesize, NULL, 10, &q) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("'rxqueuesize' attribute must be positive number: %s"), + rxqueuesize); + goto error; + } + if (q > 1) + def->driver.virtio.rxqueuesize = q; + } if ((str = virXPathString("string(./driver/host/@csum)", ctxt))) { if ((val = virTristateSwitchTypeFromString(str)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -9646,6 +9659,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(ioeventfd); VIR_FREE(event_idx); VIR_FREE(queues); + VIR_FREE(rxqueuesize); VIR_FREE(str); VIR_FREE(filter); VIR_FREE(type); @@ -20768,6 +20782,8 @@ virDomainVirtioNetDriverFormat(char **outstr, } if (def->driver.virtio.queues) virBufferAsprintf(&buf, "queues='%u' ", def->driver.virtio.queues); + if (def->driver.virtio.rxqueuesize) + virBufferAsprintf(&buf, "rxqueuesize='%u' ", def->driver.virtio.rxqueuesize); virBufferTrim(&buf, " ", -1); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8b26724..9197d70 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -906,6 +906,7 @@ struct _virDomainNetDef { virTristateSwitch ioeventfd; virTristateSwitch event_idx; unsigned int queues; /* Multiqueue virtio-net */ + unsigned int rxqueuesize; /* rx_queue_size */ struct { virTristateSwitch csum; virTristateSwitch gso; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6c0f261..ee24086 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2372,6 +2372,13 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, "not supported by QEMU")); goto cleanup; } + + if (STREQ_NULLABLE(net->model, "virtio") && + net->driver.virtio.rxqueuesize & (net->driver.virtio.rxqueuesize -1)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("rxqueuesize has to be a power of two")); + goto cleanup; + } } ret = 0; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml new file mode 100644 index 0000000..e23d3e3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <controller type='usb' index='0'/> + <interface type='user'> + <mac address='00:11:22:33:44:55'/> + <model type='virtio'/> + <driver rxqueuesize='128'/> + </interface> + <memballoon model='virtio'/> + </devices> +</domain> -- 2.8.4
On 08/19/2016 07:54 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1366989
Qemu grew yet another virtio-net tunable [1]. It basically allows
s/Qemu grew yet/QEMU added/
users to set the size of RX virtio ring. But because virtio-net uses two separate ring buffers to pass data from/to guest they named it explicitly rx_queue_size. We should expose it in our XML too.
1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02029.html
Has this been merged yet? I see Jason Wang has it queue for net-next (for 2.8), but I don't see the code in the mainline qemu I just updated to.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 16 ++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 7 ++++++ .../qemuxml2argv-net-virtio-rxqueuesize.xml | 29 ++++++++++++++++++++++ 6 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bfbb0f2..6642dc8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4604,7 +4604,7 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rxqueuesize='128'> <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off' mrg_rxbuf='off'/> <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> </driver> @@ -4720,6 +4720,11 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">virtio-net since 1.0.6 (QEMU and KVM only)</span> <span class="since">vhost-user since 1.2.17 (QEMU and KVM only)</span> </dd> + <dt><code>rxqueuesize</code></dt> + <dd> + The optional <code>rxqueuesize</code> attribute controls + the size of virtio ring for each queue as described above.
Need a <span class="since"> (I assume something similar to queues with the QEMU and KVM only condition) Also why not "rx_queue_size" - there's already event_idx or mrg_rxbuf, so adding the "_" at least mimics qemu's argument. Furthermore, the bz has quite a bit of discussion regarding an "appropriate value", so while I don't think it's something we want to provide (that is suggested values), perhaps we could create a pointer or at least a few hints. At the very least a this value needs to be a power of 2 value... If not provided, the QEMU default of 256 is used. A larger size gives you what advantage. In general some guidance on setting or where to look could be helpful.
+ </dd> </dl> <p> Offloading options for the host and guest can be configured using diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 052f28c..4b89a86 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2529,6 +2529,11 @@ </attribute> </optional> <optional> + <attribute name='rxqueuesize'> + <ref name='positiveInteger'/> + </attribute> + </optional> + <optional> <attribute name="txmode"> <choice> <value>iothread</value> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 14d4f7d..08cde19 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8911,6 +8911,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *ioeventfd = NULL; char *event_idx = NULL; char *queues = NULL; + char *rxqueuesize = NULL; char *str = NULL; char *filter = NULL; char *internal = NULL; @@ -9083,6 +9084,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, ioeventfd = virXMLPropString(cur, "ioeventfd"); event_idx = virXMLPropString(cur, "event_idx"); queues = virXMLPropString(cur, "queues"); + rxqueuesize = virXMLPropString(cur, "rxqueuesize"); } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) { if (filter) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -9466,6 +9468,17 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, if (q > 1) def->driver.virtio.queues = q; } + if (rxqueuesize) { + unsigned int q; + if (virStrToLong_uip(rxqueuesize, NULL, 10, &q) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("'rxqueuesize' attribute must be positive number: %s"), + rxqueuesize); + goto error; + } + if (q > 1) + def->driver.virtio.rxqueuesize = q; + } if ((str = virXPathString("string(./driver/host/@csum)", ctxt))) { if ((val = virTristateSwitchTypeFromString(str)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -9646,6 +9659,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(ioeventfd); VIR_FREE(event_idx); VIR_FREE(queues); + VIR_FREE(rxqueuesize); VIR_FREE(str); VIR_FREE(filter); VIR_FREE(type); @@ -20768,6 +20782,8 @@ virDomainVirtioNetDriverFormat(char **outstr, } if (def->driver.virtio.queues) virBufferAsprintf(&buf, "queues='%u' ", def->driver.virtio.queues); + if (def->driver.virtio.rxqueuesize) + virBufferAsprintf(&buf, "rxqueuesize='%u' ", def->driver.virtio.rxqueuesize);
virBufferTrim(&buf, " ", -1);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8b26724..9197d70 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -906,6 +906,7 @@ struct _virDomainNetDef { virTristateSwitch ioeventfd; virTristateSwitch event_idx; unsigned int queues; /* Multiqueue virtio-net */ + unsigned int rxqueuesize; /* rx_queue_size */ struct { virTristateSwitch csum; virTristateSwitch gso; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6c0f261..ee24086 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2372,6 +2372,13 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, "not supported by QEMU")); goto cleanup; } + + if (STREQ_NULLABLE(net->model, "virtio") && + net->driver.virtio.rxqueuesize & (net->driver.virtio.rxqueuesize -1)) {
- 1 ? e.g. extra space (although I do see the line is at 80 chars... could change the internal name to rxqsz ;-)).
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("rxqueuesize has to be a power of two"));
^^ hence why I think it should be documented. The bz also indicates there's a maximum of 1024. Should we check for that? Although if the maximum increases we'd have to follow suit. Naturally there isn't a way to get that maximum. If something larger than 1024 is passed, then qemu will fail. Ugh, the hazards of adding these 1-off things that have limits and rules, but we're not given all the details necessary.
+ goto cleanup; + } }
ret = 0; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml new file mode 100644 index 0000000..e23d3e3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <controller type='usb' index='0'/> + <interface type='user'> + <mac address='00:11:22:33:44:55'/> + <model type='virtio'/> + <driver rxqueuesize='128'/> + </interface> + <memballoon model='virtio'/> + </devices> +</domain>
Shouldn't qemuxml2xmltest.c be updated to add this new test? Should there be a "FAIL" test with an invalid value? I think by rule we have to wait for QEMU to merge this so we cannot push until then. In the meantime, we should probably find out if it's felt we should check a maximum of 1024 or not. I'd hate to see that value change and then we have problems, but the way it is now - qemu would fail with a 2048 value from libvirt. Conditional ACK once code is merged into QEMU and of course a few adjustments noted here. John
On 31.08.2016 14:48, John Ferlan wrote:
On 08/19/2016 07:54 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1366989
Qemu grew yet another virtio-net tunable [1]. It basically allows
s/Qemu grew yet/QEMU added/
users to set the size of RX virtio ring. But because virtio-net uses two separate ring buffers to pass data from/to guest they named it explicitly rx_queue_size. We should expose it in our XML too.
1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02029.html
Has this been merged yet? I see Jason Wang has it queue for net-next (for 2.8), but I don't see the code in the mainline qemu I just updated to.
No, I had to apply the patch locally and work over patched qemu. QEMU devels are currently in freeze for 2.7 release (not sure what their release date is planned on though).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 16 ++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 7 ++++++ .../qemuxml2argv-net-virtio-rxqueuesize.xml | 29 ++++++++++++++++++++++ 6 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index bfbb0f2..6642dc8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4604,7 +4604,7 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rxqueuesize='128'> <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off' mrg_rxbuf='off'/> <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> </driver> @@ -4720,6 +4720,11 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">virtio-net since 1.0.6 (QEMU and KVM only)</span> <span class="since">vhost-user since 1.2.17 (QEMU and KVM only)</span> </dd> + <dt><code>rxqueuesize</code></dt> + <dd> + The optional <code>rxqueuesize</code> attribute controls + the size of virtio ring for each queue as described above.
Need a <span class="since"> (I assume something similar to queues with the QEMU and KVM only condition)
Also why not "rx_queue_size" - there's already event_idx or mrg_rxbuf, so adding the "_" at least mimics qemu's argument.
That's what I wanted to prevent. Copying name from qemu. But I've struggled with the naming too (as can be seen - look how far I got :D). I don't have a strong opinion on either of those, so whatever we feel like I'll stick with that.
Furthermore, the bz has quite a bit of discussion regarding an "appropriate value", so while I don't think it's something we want to provide (that is suggested values), perhaps we could create a pointer or at least a few hints. At the very least a this value needs to be a power of 2 value... If not provided, the QEMU default of 256 is used. A larger size gives you what advantage. In general some guidance on setting or where to look could be helpful.
Well, as you point out later in the review too, qemu might change these limitation anytime and we would advice untrue. But if I'm careful with wording we might be safe.
+ </dd> </dl> <p> Offloading options for the host and guest can be configured using diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 052f28c..4b89a86 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2529,6 +2529,11 @@ </attribute> </optional> <optional> + <attribute name='rxqueuesize'> + <ref name='positiveInteger'/> + </attribute> + </optional> + <optional> <attribute name="txmode"> <choice> <value>iothread</value> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 14d4f7d..08cde19 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8911,6 +8911,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *ioeventfd = NULL; char *event_idx = NULL; char *queues = NULL; + char *rxqueuesize = NULL; char *str = NULL; char *filter = NULL; char *internal = NULL; @@ -9083,6 +9084,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, ioeventfd = virXMLPropString(cur, "ioeventfd"); event_idx = virXMLPropString(cur, "event_idx"); queues = virXMLPropString(cur, "queues"); + rxqueuesize = virXMLPropString(cur, "rxqueuesize"); } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) { if (filter) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -9466,6 +9468,17 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, if (q > 1) def->driver.virtio.queues = q; } + if (rxqueuesize) { + unsigned int q; + if (virStrToLong_uip(rxqueuesize, NULL, 10, &q) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("'rxqueuesize' attribute must be positive number: %s"), + rxqueuesize); + goto error; + } + if (q > 1) + def->driver.virtio.rxqueuesize = q; + } if ((str = virXPathString("string(./driver/host/@csum)", ctxt))) { if ((val = virTristateSwitchTypeFromString(str)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -9646,6 +9659,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(ioeventfd); VIR_FREE(event_idx); VIR_FREE(queues); + VIR_FREE(rxqueuesize); VIR_FREE(str); VIR_FREE(filter); VIR_FREE(type); @@ -20768,6 +20782,8 @@ virDomainVirtioNetDriverFormat(char **outstr, } if (def->driver.virtio.queues) virBufferAsprintf(&buf, "queues='%u' ", def->driver.virtio.queues); + if (def->driver.virtio.rxqueuesize) + virBufferAsprintf(&buf, "rxqueuesize='%u' ", def->driver.virtio.rxqueuesize);
virBufferTrim(&buf, " ", -1);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8b26724..9197d70 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -906,6 +906,7 @@ struct _virDomainNetDef { virTristateSwitch ioeventfd; virTristateSwitch event_idx; unsigned int queues; /* Multiqueue virtio-net */ + unsigned int rxqueuesize; /* rx_queue_size */ struct { virTristateSwitch csum; virTristateSwitch gso; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6c0f261..ee24086 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2372,6 +2372,13 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, "not supported by QEMU")); goto cleanup; } + + if (STREQ_NULLABLE(net->model, "virtio") && + net->driver.virtio.rxqueuesize & (net->driver.virtio.rxqueuesize -1)) {
- 1 ?
e.g. extra space (although I do see the line is at 80 chars... could change the internal name to rxqsz ;-)).
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("rxqueuesize has to be a power of two"));
^^ hence why I think it should be documented.
The bz also indicates there's a maximum of 1024. Should we check for that? Although if the maximum increases we'd have to follow suit. Naturally there isn't a way to get that maximum. If something larger than 1024 is passed, then qemu will fail. Ugh, the hazards of adding these 1-off things that have limits and rules, but we're not given all the details necessary.
Nope. Moreover, this feature is going to be used by expert users who know exactly what they are doing. So I'd say we're okay with just trying to start qemu and see if it fails or not. IOW too much work on our side not worth it.
+ goto cleanup; + } }
ret = 0; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml new file mode 100644 index 0000000..e23d3e3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <controller type='usb' index='0'/> + <interface type='user'> + <mac address='00:11:22:33:44:55'/> + <model type='virtio'/> + <driver rxqueuesize='128'/> + </interface> + <memballoon model='virtio'/> + </devices> +</domain>
Shouldn't qemuxml2xmltest.c be updated to add this new test?
Good catch.
Should there be a "FAIL" test with an invalid value?
Sure, I can introduce such test.
I think by rule we have to wait for QEMU to merge this so we cannot push until then. In the meantime, we should probably find out if it's felt we should check a maximum of 1024 or not. I'd hate to see that value change and then we have problems, but the way it is now - qemu would fail with a 2048 value from libvirt.
I don't think we need to. We should probably check whether it is a power of two (this looks like invariant requirement) = who would want a ring size of a non-power of two size? But the maximum/minimum size can change. Currently the limits are [256;1024]. Moreover, qemu fails with sensible message: error: internal error: process exited while connecting to monitor: 2016-08-31T14:23:16.583247Z qemu-system-x86_64: -device virtio-net-pci,mq=on,vectors=42,rx_queue_size=2048,netdev=hostnet1,id=net1,mac=52:54:00:6e:0e:72,bus=pci.2,addr=0x2: Invalid rx_queue_size (= 2048), must be a power of 2 between 256 and 1024. I'll post v2 which will is rebased onto current HEAD and address your findings. Thank you! Michal
On 08/31/2016 10:24 AM, Michal Privoznik wrote:
On 31.08.2016 14:48, John Ferlan wrote:
[...]
--- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4604,7 +4604,7 @@ qemu-kvm -net nic,model=? /dev/null <source network='default'/> <target dev='vnet1'/> <model type='virtio'/> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rxqueuesize='128'> <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off' mrg_rxbuf='off'/> <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> </driver> @@ -4720,6 +4720,11 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">virtio-net since 1.0.6 (QEMU and KVM only)</span> <span class="since">vhost-user since 1.2.17 (QEMU and KVM only)</span> </dd> + <dt><code>rxqueuesize</code></dt> + <dd> + The optional <code>rxqueuesize</code> attribute controls + the size of virtio ring for each queue as described above.
Need a <span class="since"> (I assume something similar to queues with the QEMU and KVM only condition)
Also why not "rx_queue_size" - there's already event_idx or mrg_rxbuf, so adding the "_" at least mimics qemu's argument.
That's what I wanted to prevent. Copying name from qemu. But I've struggled with the naming too (as can be seen - look how far I got :D). I don't have a strong opinion on either of those, so whatever we feel like I'll stick with that.
I guess I find it easier to be able to search qemu and libvirt code for the same strings "if possible". I think we've already nixed allowing "-" (dash), so those don't match up. I don't have a strong preference either.
Furthermore, the bz has quite a bit of discussion regarding an "appropriate value", so while I don't think it's something we want to provide (that is suggested values), perhaps we could create a pointer or at least a few hints. At the very least a this value needs to be a power of 2 value... If not provided, the QEMU default of 256 is used. A larger size gives you what advantage. In general some guidance on setting or where to look could be helpful.
Well, as you point out later in the review too, qemu might change these limitation anytime and we would advice untrue. But if I'm careful with wording we might be safe.
As you point out below - "This should be used by expert users" or as the disk device "ioeventfd" and "event_idx" descriptions indicate in bold letters" "In general you should leave this option alone, unless you are very certain you know what you are doing."
+ </dd> </dl> <p> Offloading options for the host and guest can be configured using
[...]
e.g. extra space (although I do see the line is at 80 chars... could change the internal name to rxqsz ;-)).
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("rxqueuesize has to be a power of two"));
^^ hence why I think it should be documented.
The bz also indicates there's a maximum of 1024. Should we check for that? Although if the maximum increases we'd have to follow suit. Naturally there isn't a way to get that maximum. If something larger than 1024 is passed, then qemu will fail. Ugh, the hazards of adding these 1-off things that have limits and rules, but we're not given all the details necessary.
Nope. Moreover, this feature is going to be used by expert users who know exactly what they are doing. So I'd say we're okay with just trying to start qemu and see if it fails or not. IOW too much work on our side not worth it.
+ goto cleanup; + } }
ret = 0; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml new file mode 100644 index 0000000..e23d3e3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <controller type='usb' index='0'/> + <interface type='user'> + <mac address='00:11:22:33:44:55'/> + <model type='virtio'/> + <driver rxqueuesize='128'/> + </interface> + <memballoon model='virtio'/> + </devices> +</domain>
Shouldn't qemuxml2xmltest.c be updated to add this new test?
Good catch.
As I'm going through your NVDIMM series - it has the same problem - so I hunted down what I did for luks-disks... See commit id '9bbf0d7e' - it adds a file link for the tests/qemuxml2xmloutdata/ to the ../qemuxml2argvdata/... I had just copied other examples. And I also modified qemuxml2xmltest
Should there be a "FAIL" test with an invalid value?
Sure, I can introduce such test.
I think by rule we have to wait for QEMU to merge this so we cannot push until then. In the meantime, we should probably find out if it's felt we should check a maximum of 1024 or not. I'd hate to see that value change and then we have problems, but the way it is now - qemu would fail with a 2048 value from libvirt.
I don't think we need to. We should probably check whether it is a power of two (this looks like invariant requirement) = who would want a ring size of a non-power of two size? But the maximum/minimum size can change. Currently the limits are [256;1024]. Moreover, qemu fails with sensible message:
error: internal error: process exited while connecting to monitor: 2016-08-31T14:23:16.583247Z qemu-system-x86_64: -device virtio-net-pci,mq=on,vectors=42,rx_queue_size=2048,netdev=hostnet1,id=net1,mac=52:54:00:6e:0e:72,bus=pci.2,addr=0x2: Invalid rx_queue_size (= 2048), must be a power of 2 between 256 and 1024.
I'll post v2 which will is rebased onto current HEAD and address your findings.
I'm fine with not adding the max comparison... As I think you could see left brain and right brain weren't in agreement either! John
Just like in the previous commit, teach qemu driver to detect whether qemu supports this configuration knob or not. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 3 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8efb8fb..373002d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -341,6 +341,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "intel-iommu", "smm", "virtio-pci-disable-legacy", + "virtio-net.rx_queue_size", ); @@ -1580,6 +1581,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioNet[] = { { "tx", QEMU_CAPS_VIRTIO_TX_ALG }, { "event_idx", QEMU_CAPS_VIRTIO_NET_EVENT_IDX }, + { "rx_queue_size", QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioSCSI[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 776a0f3..c8b60bf 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -374,6 +374,7 @@ typedef enum { QEMU_CAPS_DEVICE_INTEL_IOMMU, /* -device intel-iommu */ QEMU_CAPS_MACHINE_SMM_OPT, /* -machine xxx,smm=on/off/auto */ QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, /* virtio-*pci.disable-legacy */ + QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE, /* virtio-net-*.rx_queue_size */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.8.4
On 08/19/2016 07:54 AM, Michal Privoznik wrote:
Just like in the previous commit, teach qemu driver to detect whether qemu supports this configuration knob or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 3 insertions(+)
This will need a tweak since it conflicts with "query-hotpluggable-cpus" Looks like we'll need to add some tests/qemucapabilitiesdata/* files for 2.8 (caps_2.8.0.x86_64.{xml|replies}) Conditional ACK... John
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 8 +++++++ .../qemuxml2argv-net-virtio-rxqueuesize.args | 25 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 3 files changed, 35 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a6dea6a..0c9dff4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3553,6 +3553,14 @@ qemuBuildNicDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",mq=on,vectors=%zu", 2 * vhostfdSize + 2); } } + if (usingVirtio && net->driver.virtio.rxqueuesize) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("virtio rx_queue_size option is not supported with this QEMU binary")); + goto error; + } + virBufferAsprintf(&buf, ",rx_queue_size=%u", net->driver.virtio.rxqueuesize); + } if (vlan == -1) virBufferAsprintf(&buf, ",netdev=host%s", net->info.alias); else diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args new file mode 100644 index 0000000..6392889 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args @@ -0,0 +1,25 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-device virtio-net-pci,rx_queue_size=128,vlan=0,id=net0,mac=00:11:22:33:44:55,\ +bus=pci.0,addr=0x3 \ +-net user,vlan=0,name=hostnet0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e8b8cb4..fc94e30 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1043,6 +1043,8 @@ mymain(void) QEMU_CAPS_VIRTIO_S390); DO_TEST("net-virtio-ccw", QEMU_CAPS_VIRTIO_CCW, QEMU_CAPS_VIRTIO_S390); + DO_TEST("net-virtio-rxqueuesize", + QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE); DO_TEST("net-eth", NONE); DO_TEST("net-eth-ifname", NONE); DO_TEST("net-eth-names", NONE); -- 2.8.4
On 08/19/2016 07:54 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 8 +++++++ .../qemuxml2argv-net-virtio-rxqueuesize.args | 25 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ 3 files changed, 35 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args
Conditional ACK as well. John
participants (2)
-
John Ferlan -
Michal Privoznik