[libvirt] [PATCH 1/3] conf: Add support for virtio-net.rx_queue_size
John Ferlan
jferlan at redhat.com
Wed Aug 31 12:48:11 UTC 2016
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 at 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
More information about the libvir-list
mailing list