[libvirt] [PATCH v2 01/12] Introduce /domain/devices/interface/driver/@queues attribute
Laine Stump
laine at laine.org
Mon May 13 20:38:12 UTC 2013
On 05/13/2013 01:22 PM, Michal Privoznik wrote:
> This attribute is going to represent number of queues for
> multique vhost network interface. This commit implements XML
> extension part of the feature and add one test as well. For now,
> we can only do xml2xml test as qemu command line generation code
> is not adapted yet.
> ---
> docs/formatdomain.html.in | 11 ++++-
> docs/schemas/domaincommon.rng | 5 +++
> src/conf/domain_conf.c | 15 +++++++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_domain.c | 27 +++++++-----
> tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml | 2 +-
> .../qemuxml2argv-net-virtio-device.xml | 2 +-
> .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++
> tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 2 +-
> tests/qemuxml2xmltest.c | 1 +
> 10 files changed, 103 insertions(+), 14 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 9ade507..68cd3d4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3186,7 +3186,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'/></b>
> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/></b>
> </interface>
> </devices>
> ...</pre>
> @@ -3280,6 +3280,15 @@ qemu-kvm -net nic,model=? /dev/null
> <b>In general you should leave this option alone, unless you
> are very certain you know what you are doing.</b>
> </dd>
> + <dt><code>queues</code></dt>
> + <dd>
> + The optional <code>queues</code> attribute controls number of
> + queues for <a href="http://www.linux-kvm.org/page/Multiqueue">M
> + ultiqueue virtio-net</a> feature. Long story short, in case of a
"Long story short" is a bit too informal for documentation. Maybe
instead you could say something like:
If the interface has <model type='virtio'/>, multiple packet
processing queues can be created; each queue will potentially be handled
by a different processor, resulting in much higher throughput.
> + virtio net device, multiple queues can be created so each queue is
> + handled by different processor resulting in much higher throughput.
> + <span class="since">Since 1.0.5 (QEMU and KVM only)</span>
s/1.0.5/1.0.6/
> + </dd>
> </dl>
>
> <h5><a name="elementsNICSTargetOverride">Overriding the target element</a></h5>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 8004428..d671491 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1990,6 +1990,11 @@
> </attribute>
> </optional>
> <optional>
> + <attribute name='queues'>
> + <ref name="positiveInteger"/>
Should a lower limit be put on it in the RNG? (does qemu have a
documented limit?)
> + </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 862b997..429f0ed 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5935,6 +5935,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> char *txmode = NULL;
> char *ioeventfd = NULL;
> char *event_idx = NULL;
> + char *queues = NULL;
> char *filter = NULL;
> char *internal = NULL;
> char *devaddr = NULL;
> @@ -6046,6 +6047,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> txmode = virXMLPropString(cur, "txmode");
> ioeventfd = virXMLPropString(cur, "ioeventfd");
> event_idx = virXMLPropString(cur, "event_idx");
> + queues = virXMLPropString(cur, "queues");
> } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) {
> if (filter) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -6336,6 +6338,16 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
> def->driver.virtio.event_idx = idx;
> }
> + if (queues) {
> + unsigned int q;
> + if (virStrToLong_ui(queues, NULL, 10, &q) < 0) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("'queues' attribute must be unsigned integer: %s"),
How about "Invalid queues value %s, must be a positive number" or
"must be 1 - [some value]"?
> + queues);
> + goto error;
> + }
> + def->driver.virtio.queues = q;
> + }
> }
>
> def->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT;
> @@ -6389,6 +6401,7 @@ cleanup:
> VIR_FREE(txmode);
> VIR_FREE(ioeventfd);
> VIR_FREE(event_idx);
> + VIR_FREE(queues);
> VIR_FREE(filter);
> VIR_FREE(type);
> VIR_FREE(internal);
> @@ -14354,6 +14367,8 @@ virDomainNetDefFormat(virBufferPtr buf,
> virBufferAsprintf(buf, " event_idx='%s'",
> virDomainVirtioEventIdxTypeToString(def->driver.virtio.event_idx));
> }
> + if (def->driver.virtio.queues)
> + virBufferAsprintf(buf, " queues='%u'", def->driver.virtio.queues);
> virBufferAddLit(buf, "/>\n");
> }
> }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a9d3410..0a30406 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -923,6 +923,7 @@ struct _virDomainNetDef {
> enum virDomainNetVirtioTxModeType txmode;
> enum virDomainIoEventFd ioeventfd;
> enum virDomainVirtioEventIdx event_idx;
> + unsigned int queues; /* Multiqueue virtio-net */
> } virtio;
> } driver;
> union {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 97a8307..ce22afc 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -727,17 +727,24 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> virQEMUDriverPtr driver = opaque;
> virQEMUDriverConfigPtr cfg = NULL;
>
> - if (dev->type == VIR_DOMAIN_DEVICE_NET &&
> - dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> - !dev->data.net->model) {
> - if (def->os.arch == VIR_ARCH_S390 ||
> - def->os.arch == VIR_ARCH_S390X)
> - dev->data.net->model = strdup("virtio");
> - else
> - dev->data.net->model = strdup("rtl8139");
> + if (dev->type == VIR_DOMAIN_DEVICE_NET) {
> + if (dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> + !dev->data.net->model) {
> + if (def->os.arch == VIR_ARCH_S390 ||
> + def->os.arch == VIR_ARCH_S390X)
> + dev->data.net->model = strdup("virtio");
> + else
> + dev->data.net->model = strdup("rtl8139");
>
> - if (!dev->data.net->model)
> - goto no_memory;
> + if (!dev->data.net->model)
> + goto no_memory;
> + }
> +
> + if (STREQ(dev->data.net->model, "virtio") &&
> + dev->data.net->driver.virtio.queues == 0) {
> + /* default number of queues for multiqueue NIC */
> + dev->data.net->driver.virtio.queues = 1;
> + }
Rather than relying on some code in an out-of-the-way post-parse
callback to set this to 1, I think it would be better to just interpret
0 as 1 where the value is used. Aside from removing the mystery for
someone casually reading the code who doesn't know about the post-parse
function, it would also allow us to use "0" as a sentinel value which
means "don't emit" during formatting. This is one of those attributes
which I think *shouldn't* have its default value auto-filled (as long as
migrating from one machine to another with the number of queues changing
from source to dest doesn't create a problem, that is).
> }
>
> /* set default disk types and drivers */
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml
> index b3b7e89..44ce184 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.xml
> @@ -38,7 +38,7 @@
> <interface type='user'>
> <mac address='52:54:00:e5:48:58'/>
> <model type='virtio'/>
> - <driver name='vhost' event_idx='off'/>
> + <driver name='vhost' event_idx='off' queues='1'/>
... and you would then avoid all of these gratuitous changes to xml test
data.
> </interface>
> <serial type='pty'>
> <target port='0'/>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml
> index 1782831..04f3471 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.xml
> @@ -25,7 +25,7 @@
> <interface type='user'>
> <mac address='00:11:22:33:44:55'/>
> <model type='virtio'/>
> - <driver txmode='iothread'/>
> + <driver txmode='iothread' queues='1'/>
> </interface>
> <memballoon model='virtio'/>
> </devices>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml b/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
> new file mode 100644
> index 0000000..76f84f6
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
> @@ -0,0 +1,51 @@
> +<domain type='qemu'>
> + <name>test</name>
> + <uuid>bba65c0e-c049-934f-b6aa-4e2c0582acdf</uuid>
> + <memory unit='KiB'>1048576</memory>
> + <currentMemory unit='KiB'>1048576</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc-0.13'>hvm</type>
> + <boot dev='cdrom'/>
> + <boot dev='hd'/>
> + <bootmenu enable='yes'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>restart</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu</emulator>
> + <disk type='file' device='disk'>
> + <driver name='qemu' type='qcow2' event_idx='on'/>
> + <source file='/var/lib/libvirt/images/f14.img'/>
> + <target dev='vda' bus='virtio'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
> + </disk>
> + <disk type='file' device='cdrom'>
> + <driver name='qemu' type='raw'/>
> + <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/>
> + <target dev='hdc' bus='ide'/>
> + <readonly/>
> + <address type='drive' controller='0' bus='1' target='0' unit='0'/>
> + </disk>
> + <controller type='usb' index='0'/>
> + <controller type='virtio-serial' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>
> + </controller>
> + <controller type='ide' index='0'/>
> + <controller type='pci' index='0' model='pci-root'/>
> + <interface type='user'>
> + <mac address='52:54:00:e5:48:58'/>
> + <model type='virtio'/>
> + <driver name='vhost' queues='5'/>
> + </interface>
> + <serial type='pty'>
> + <target port='0'/>
> + </serial>
> + <console type='pty'>
> + <target type='serial' port='0'/>
> + </console>
> + <memballoon model='virtio'/>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml
> index 077ca92..2717439 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml
> @@ -37,7 +37,7 @@
> <interface type='user'>
> <mac address='52:54:00:e5:48:58'/>
> <model type='virtio'/>
> - <driver name='vhost' event_idx='off'/>
> + <driver name='vhost' event_idx='off' queues='1'/>
> </interface>
> <serial type='pty'>
> <target port='0'/>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 492ac60..c06c189 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -244,6 +244,7 @@ mymain(void)
> DO_TEST("smp");
> DO_TEST("lease");
> DO_TEST("event_idx");
> + DO_TEST("vhost_queues");
> DO_TEST("virtio-lun");
>
> DO_TEST("usb-redir");
More information about the libvir-list
mailing list