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 | 12 ++++- docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 15 +++++++ src/conf/domain_conf.h | 1 + .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 5e89a92..217202d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3248,7 +3248,7 @@ qemu-kvm -net nic,model=? /dev/null &lt;source network='default'/&gt; &lt;target dev='vnet1'/&gt; &lt;model type='virtio'/&gt; - <b>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off'/&gt;</b> + <b>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'/&gt;</b> &lt;/interface&gt; &lt;/devices&gt; ...</pre> @@ -3342,6 +3342,16 @@ 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"> + Multiqueue virtio-net</a> feature. If the interface has <code>&lt;model + type='virtio'/&gt;</code>, multiple packet processing queues can be + created; each queue will potentially be handled by a different + processor, resulting in much higher throughput. + <span class="since">Since 1.0.6 (QEMU and KVM only)</span> + </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 361794e..500c43c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2008,6 +2008,11 @@ </attribute> </optional> <optional> + <attribute name='queues'> + <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 f0ca9d5..4a120f6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5950,6 +5950,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; @@ -6061,6 +6062,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", @@ -6351,6 +6353,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"), + queues); + goto error; + } + def->driver.virtio.queues = q; + } } def->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT; @@ -6404,6 +6416,7 @@ cleanup: VIR_FREE(txmode); VIR_FREE(ioeventfd); VIR_FREE(event_idx); + VIR_FREE(queues); VIR_FREE(filter); VIR_FREE(type); VIR_FREE(internal); @@ -14425,6 +14438,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 c176a4c..507dfd8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -932,6 +932,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/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/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 148a05a..96f3861 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -246,6 +246,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"); -- 1.8.2.1

Currently, we have one huge function to construct qemu command line. This is very ineffective esp. if there's a fault somewhere. --- src/qemu/qemu_command.c | 224 +++++++++++++++++++++++++----------------------- 1 file changed, 117 insertions(+), 107 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5b95c07..4c87d70 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6405,6 +6405,117 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, return 0; } +static int +qemuBuildInterfaceCommandLine(virCommandPtr cmd, + virQEMUDriverPtr driver, + virConnectPtr conn, + virDomainDefPtr def, + virDomainNetDefPtr net, + virQEMUCapsPtr qemuCaps, + int vlan, + int bootindex, + enum virNetDevVPortProfileOp vmop) +{ + int ret = -1; + int tapfd = -1; + char *nic = NULL, *host = NULL; + char *tapfdName = NULL; + char *vhostfdName = NULL; + int actualType = virDomainNetGetActualType(net); + + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + /* NET_TYPE_HOSTDEV devices are really hostdev devices, so + * their commandlines are constructed with other hostdevs. + */ + return 0; + } + + if (!bootindex) + bootindex = net->info.bootIndex; + + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { + tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps); + if (tapfd < 0) + goto cleanup; + } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop); + if (tapfd < 0) + goto cleanup; + } + + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + /* Attempt to use vhost-net mode for these types of + network device */ + int vhostfd; + + if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) + goto cleanup; + if (vhostfd >= 0) { + virCommandTransferFD(cmd, vhostfd); + if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) { + virReportOOMError(); + goto cleanup; + } + } + } + + if (tapfd >= 0) { + virCommandTransferFD(cmd, tapfd); + if (virAsprintf(&tapfdName, "%d", tapfd) < 0) { + virReportOOMError(); + goto cleanup; + } + } + + /* Possible combinations: + * + * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 + * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1 + * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 + * + * NB, no support for -netdev without use of -device + */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + if (!(host = qemuBuildHostNetStr(net, driver, + ',', vlan, tapfdName, + vhostfdName))) + goto cleanup; + virCommandAddArgList(cmd, "-netdev", host, NULL); + } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + if (!(nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps))) + goto cleanup; + virCommandAddArgList(cmd, "-device", nic, NULL); + } else { + if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) + goto cleanup; + virCommandAddArgList(cmd, "-net", nic, NULL); + } + if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { + if (!(host = qemuBuildHostNetStr(net, driver, + ',', vlan, tapfdName, + vhostfdName))) + goto cleanup; + virCommandAddArgList(cmd, "-net", host, NULL); + } + + ret = 0; +cleanup: + if (ret < 0) + virDomainConfNWFilterTeardown(net); + VIR_FREE(nic); + VIR_FREE(host); + VIR_FREE(tapfdName); + VIR_FREE(vhostfdName); + return ret; +} + /* * Constructs a argv suitable for launching qemu with config defined * for a given virtual machine. @@ -7327,118 +7438,17 @@ qemuBuildCommandLine(virConnectPtr conn, for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; - char *nic, *host; - char tapfd_name[50] = ""; - char vhostfd_name[50] = ""; - int vlan; - int bootindex = bootNet; - int actualType = virDomainNetGetActualType(net); - - if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { - /* NET_TYPE_HOSTDEV devices are really hostdev devices, so - * their commandlines are constructed with other hostdevs. - */ - continue; - } - - bootNet = 0; - if (!bootindex) - bootindex = net->info.bootIndex; - + int vlan = i; /* VLANs are not used with -netdev, so don't record them */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) vlan = -1; - else - vlan = i; - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { - int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, - qemuCaps); - if (tapfd < 0) - goto error; - - last_good_net = i; - virCommandTransferFD(cmd, tapfd); - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", - tapfd) >= sizeof(tapfd_name)) - goto no_memory; - } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - int tapfd = qemuPhysIfaceConnect(def, driver, net, - qemuCaps, vmop); - if (tapfd < 0) - goto error; - - last_good_net = i; - virCommandTransferFD(cmd, tapfd); - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", - tapfd) >= sizeof(tapfd_name)) - goto no_memory; - } - - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || - actualType == VIR_DOMAIN_NET_TYPE_ETHERNET || - actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - /* Attempt to use vhost-net mode for these types of - network device */ - int vhostfd; - - if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) - goto error; - if (vhostfd >= 0) { - virCommandTransferFD(cmd, vhostfd); - - if (snprintf(vhostfd_name, sizeof(vhostfd_name), "%d", - vhostfd) >= sizeof(vhostfd_name)) - goto no_memory; - } - } - /* Possible combinations: - * - * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 - * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1 - * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1 - * - * NB, no support for -netdev without use of -device - */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - virCommandAddArg(cmd, "-netdev"); - if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfd_name, - vhostfd_name))) - goto error; - virCommandAddArg(cmd, host); - VIR_FREE(host); - } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - virCommandAddArg(cmd, "-device"); - nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps); - if (!nic) - goto error; - virCommandAddArg(cmd, nic); - VIR_FREE(nic); - } else { - virCommandAddArg(cmd, "-net"); - if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) - goto error; - virCommandAddArg(cmd, nic); - VIR_FREE(nic); - } - if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { - virCommandAddArg(cmd, "-net"); - if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfd_name, - vhostfd_name))) - goto error; - virCommandAddArg(cmd, host); - VIR_FREE(host); - } + if (qemuBuildInterfaceCommandLine(cmd, driver, conn, def, net, + qemuCaps, vlan, bootNet, vmop) < 0) + goto error; + last_good_net = i; + bootNet = 0; } } -- 1.8.2.1

Currently, only one tapfd and one vhostfd could be passed. However, multiqueue network requires several FDs to be passed to qemu so we must adapt out monitor handling functions to cope with that. --- src/qemu/qemu_hotplug.c | 7 +++++-- src/qemu/qemu_monitor.c | 39 +++++++++++++++++++++++---------------- src/qemu/qemu_monitor.h | 4 ++-- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index d037c9d..b04f3bb 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -813,8 +813,11 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, qemuDomainObjEnterMonitor(driver, vm); if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuMonitorAddNetdev(priv->mon, netstr, tapfd, tapfd_name, - vhostfd, vhostfd_name) < 0) { + if (qemuMonitorAddNetdev(priv->mon, netstr, + &tapfd, &tapfd_name, + tapfd_name ? 1 : 0, + &vhostfd, &vhostfd_name, + vhostfd_name ? 1 : 0) < 0) { qemuDomainObjExitMonitor(driver, vm); virDomainAuditNet(vm, NULL, net, "attach", false); goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f6d8ef4..1d65dd8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2543,14 +2543,16 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon, int qemuMonitorAddNetdev(qemuMonitorPtr mon, const char *netdevstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name) + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize) { int ret = -1; - VIR_DEBUG("mon=%p netdevstr=%s tapfd=%d tapfd_name=%s " - "vhostfd=%d vhostfd_name=%s", - mon, netdevstr, tapfd, NULLSTR(tapfd_name), - vhostfd, NULLSTR(vhostfd_name)); + int i = 0, j = 0; + + VIR_DEBUG("mon=%p netdevstr=%s tapfd=%p tapfdName=%p tapfdSize=%d" + "vhostfd=%p vhostfdName=%p vhostfdSize=%d", + mon, netdevstr, tapfd, tapfdName, tapfdSize, + vhostfd, vhostfdName, tapfdSize); if (!mon) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2558,12 +2560,13 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon, return -1; } - if (tapfd >= 0 && qemuMonitorSendFileHandle(mon, tapfd_name, tapfd) < 0) - return -1; - if (vhostfd >= 0 && - qemuMonitorSendFileHandle(mon, vhostfd_name, vhostfd) < 0) { - vhostfd = -1; - goto cleanup; + for (i = 0; i < tapfdSize; i++) { + if (qemuMonitorSendFileHandle(mon, tapfdName[i], tapfd[i]) < 0) + goto cleanup; + } + for (j = 0; j < vhostfdSize; j++) { + if (qemuMonitorSendFileHandle(mon, vhostfdName[j], vhostfd[j]) < 0) + goto cleanup; } if (mon->json) @@ -2573,10 +2576,14 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon, cleanup: if (ret < 0) { - if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", tapfd_name); - if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", vhostfd_name); + while (i--) { + if (qemuMonitorCloseFileHandle(mon, tapfdName[i]) < 0) + VIR_WARN("failed to close device handle '%s'", tapfdName[i]); + } + while (j--) { + if (qemuMonitorCloseFileHandle(mon, vhostfdName[j]) < 0) + VIR_WARN("failed to close device handle '%s'", vhostfdName[j]); + } } return ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8f9c182..f08e468 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -510,8 +510,8 @@ int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon, int qemuMonitorAddNetdev(qemuMonitorPtr mon, const char *netdevstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name); + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize); int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, const char *alias); -- 1.8.2.1

Currently, only one tapfd and one vhostfd could be passed. However, multiqueue network requires several FDs to be passed to qemu so we must adapt out monitor handling functions to cope with that. --- src/qemu/qemu_hotplug.c | 7 +++++-- src/qemu/qemu_monitor.c | 39 +++++++++++++++++++++++---------------- src/qemu/qemu_monitor.h | 4 ++-- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b04f3bb..41431b3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -823,8 +823,11 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, goto cleanup; } } else { - if (qemuMonitorAddHostNetwork(priv->mon, netstr, tapfd, tapfd_name, - vhostfd, vhostfd_name) < 0) { + if (qemuMonitorAddHostNetwork(priv->mon, netstr, + &tapfd, &tapfd_name, + tapfd_name ? 1 : 0, + &vhostfd, &vhostfd_name, + vhostfd_name ? 1 : 0) < 0) { qemuDomainObjExitMonitor(driver, vm); virDomainAuditNet(vm, NULL, net, "attach", false); goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 1d65dd8..524eb9d 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2477,14 +2477,16 @@ cleanup: int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, const char *netstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name) + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize) { int ret = -1; - VIR_DEBUG("mon=%p netstr=%s tapfd=%d tapfd_name=%s " - "vhostfd=%d vhostfd_name=%s", - mon, netstr, tapfd, NULLSTR(tapfd_name), - vhostfd, NULLSTR(vhostfd_name)); + int i = 0, j = 0; + + VIR_DEBUG("mon=%p netstr=%s tapfd=%p tapfdName=%p tapfdSize=%d " + "vhostfd=%p vhostfdName=%p vhostfdSize=%d", + mon, netstr, tapfd, tapfdName, tapfdSize, + vhostfd, vhostfdName, vhostfdSize); if (!mon) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2492,12 +2494,13 @@ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, return -1; } - if (tapfd >= 0 && qemuMonitorSendFileHandle(mon, tapfd_name, tapfd) < 0) - return -1; - if (vhostfd >= 0 && - qemuMonitorSendFileHandle(mon, vhostfd_name, vhostfd) < 0) { - vhostfd = -1; - goto cleanup; + for (i = 0; i < tapfdSize; i++) { + if (qemuMonitorSendFileHandle(mon, tapfdName[i], tapfd[i]) < 0) + goto cleanup; + } + for (j = 0; j < vhostfdSize; j++) { + if (qemuMonitorSendFileHandle(mon, vhostfdName[j], vhostfd[j]) < 0) + goto cleanup; } if (mon->json) @@ -2508,10 +2511,14 @@ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, cleanup: if (ret < 0) { - if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", tapfd_name); - if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) - VIR_WARN("failed to close device handle '%s'", vhostfd_name); + while (i--) { + if (qemuMonitorCloseFileHandle(mon, tapfdName[i]) < 0) + VIR_WARN("failed to close device handle '%s'", tapfdName[i]); + } + while (j--) { + if (qemuMonitorCloseFileHandle(mon, vhostfdName[j]) < 0) + VIR_WARN("failed to close device handle '%s'", vhostfdName[j]); + } } return ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f08e468..a607712 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -501,8 +501,8 @@ int qemuMonitorRemoveFd(qemuMonitorPtr mon, int fdset, int fd); */ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, const char *netstr, - int tapfd, const char *tapfd_name, - int vhostfd, const char *vhostfd_name); + int *tapfd, char **tapfdName, int tapfdSize, + int *vhostfd, char **vhostfdName, int vhostfdSize); int qemuMonitorRemoveHostNetwork(qemuMonitorPtr mon, int vlan, -- 1.8.2.1

The qemuBuildHostNetStr() function which is responsible for generating command line for a network interface needs to be aware of multiqueue network interface as we are required to use: - fd=%d in case of one FD - fds=%d:%d:%d:...:%d in case of multiple FDs These two approaches can't be mixed. Same applies for vhost FDs. --- src/qemu/qemu_command.c | 46 +++++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_command.h | 6 ++++-- src/qemu/qemu_hotplug.c | 10 ++++++---- 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4c87d70..67baad3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4109,13 +4109,16 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, virQEMUDriverPtr driver, char type_sep, int vlan, - const char *tapfd, - const char *vhostfd) + char **tapfd, + int tapfdSize, + char **vhostfd, + int vhostfdSize) { bool is_tap = false; virBuffer buf = VIR_BUFFER_INITIALIZER; enum virDomainNetType netType = virDomainNetGetActualType(net); virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + int i; if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -4134,7 +4137,19 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_DIRECT: - virBufferAsprintf(&buf, "tap%cfd=%s", type_sep, tapfd); + virBufferAsprintf(&buf, "tap%c", type_sep); + /* for one tapfd 'fd=' shall be used, + * for more than one 'fds=' is the right choice */ + if (tapfdSize == 1) { + virBufferAsprintf(&buf, "fd=%s", tapfd[0]); + } else { + virBufferAddLit(&buf, "fds="); + for (i = 0; i < tapfdSize; i++) { + if (i) + virBufferAddChar(&buf, ':'); + virBufferAdd(&buf, tapfd[i], -1); + } + } type_sep = ','; is_tap = true; break; @@ -4194,8 +4209,19 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, } if (is_tap) { - if (vhostfd && *vhostfd) - virBufferAsprintf(&buf, ",vhost=on,vhostfd=%s", vhostfd); + if (vhostfdSize) { + virBufferAddLit(&buf, ",vhost=on,"); + if (vhostfdSize == 1) { + virBufferAsprintf(&buf, "vhostfd=%s", vhostfd[0]); + } else { + virBufferAddLit(&buf, "vhostfds="); + for (i = 0; i < vhostfdSize; i++) { + if (i) + virBufferAddChar(&buf, ':'); + virBufferAdd(&buf, vhostfd[i], -1); + } + } + } if (net->tune.sndbuf_specified) virBufferAsprintf(&buf, ",sndbuf=%lu", net->tune.sndbuf); } @@ -6482,8 +6508,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfdName, - vhostfdName))) + ',', vlan, + &tapfdName, tapfdName ? 1 : 0, + &vhostfdName, vhostfdName ? 1 : 0))) goto cleanup; virCommandAddArgList(cmd, "-netdev", host, NULL); } @@ -6499,8 +6526,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { if (!(host = qemuBuildHostNetStr(net, driver, - ',', vlan, tapfdName, - vhostfdName))) + ',', vlan, + &tapfdName, tapfdName ? 1 : 0, + &vhostfdName, vhostfdName ? 1 : 0))) goto cleanup; virCommandAddArgList(cmd, "-net", host, NULL); } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 36dfa6b..6765c3a 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -76,8 +76,10 @@ char * qemuBuildHostNetStr(virDomainNetDefPtr net, virQEMUDriverPtr driver, char type_sep, int vlan, - const char *tapfd, - const char *vhostfd); + char **tapfd, + int tapfdSize, + char **vhostfd, + int vhostfdSize); /* Legacy, pre device support */ char * qemuBuildNicStr(virDomainNetDefPtr net, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 41431b3..0a1845a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -800,13 +800,15 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (!(netstr = qemuBuildHostNetStr(net, driver, - ',', -1, tapfd_name, - vhostfd_name))) + ',', -1, + &tapfd_name, tapfd_name ? 1 : 0, + &vhostfd_name, vhostfd_name ? 1 : 0))) goto cleanup; } else { if (!(netstr = qemuBuildHostNetStr(net, driver, - ' ', vlan, tapfd_name, - vhostfd_name))) + ' ', vlan, + &tapfd_name, tapfd_name ? 1 : 0, + &vhostfd_name, vhostfd_name ? 1 : 0))) goto cleanup; } -- 1.8.2.1

Currently, the function knows how to open a TAP device for a single time. However, in case of multiqueue network we need to open it multiple times. Moreover, when doing TUNSETIFF ioctl, the IFF_MULTI_QUEUE flag shall be requested. This commit changes a behaviour slightly as well. Till now it was possible to pass NULL as an @fd address by which caller didn't care about returned FD. While this was used only in UML driver, the appropriate UML code snippets has been changed as well. --- src/network/bridge_driver.c | 2 +- src/qemu/qemu_command.c | 2 +- src/uml/uml_conf.c | 5 +- src/util/virnetdevtap.c | 113 ++++++++++++++++++++++++-------------------- src/util/virnetdevtap.h | 2 + 5 files changed, 71 insertions(+), 53 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6c6ce6d..ad4ab00 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2495,7 +2495,7 @@ networkStartNetworkVirtual(struct network_driver *driver, /* Keep tun fd open and interface up to allow for IPv6 DAD to happen */ if (virNetDevTapCreateInBridgePort(network->def->bridge, &macTapIfName, &network->def->mac, - NULL, &tapfd, NULL, NULL, + NULL, &tapfd, 1, NULL, NULL, VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE | VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 67baad3..a053d49 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -355,7 +355,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (cfg->privileged) err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, - def->uuid, &tapfd, + def->uuid, &tapfd, 1, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), tap_create_flags); diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 52b705c..3fda7e4 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -109,6 +109,7 @@ umlConnectTapDevice(virConnectPtr conn, const char *bridge) { bool template_ifname = false; + int tapfd; if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) || @@ -121,7 +122,7 @@ umlConnectTapDevice(virConnectPtr conn, } if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, &net->mac, - vm->uuid, NULL, + vm->uuid, &tapfd, 1, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), VIR_NETDEV_TAP_CREATE_IFUP | @@ -139,9 +140,11 @@ umlConnectTapDevice(virConnectPtr conn, } } + VIR_FORCE_CLOSE(tapfd); return 0; error: + VIR_FORCE_CLOSE(tapfd); return -1; } diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 75599db..62a1ba2 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -140,7 +140,8 @@ virNetDevProbeVnetHdr(int tapfd) /** * virNetDevTapCreate: * @ifname: the interface name - * @tapfd: file descriptor return value for the new tap device + * @tapfds: file descriptor return value for the new tap device + * @tapfdSize: size of @tapfd * @flags: OR of virNetDevTapCreateFlags. Only one flag is recognized: * * VIR_NETDEV_TAP_CREATE_VNET_HDR @@ -148,76 +149,85 @@ virNetDevProbeVnetHdr(int tapfd) * VIR_NETDEV_TAP_CREATE_PERSIST * - The device will persist after the file descriptor is closed * - * Creates a tap interface. - * If the @tapfd parameter is supplied, the open tap device file descriptor - * will be returned, otherwise the TAP device will be closed. The caller must - * use virNetDevTapDelete to remove a persistent TAP device when it is no - * longer needed. + * Creates a tap interface. The caller must use virNetDevTapDelete to + * remove a persistent TAP device when it is no longer needed. In case + * @tapfdSize is greater than one, multiqueue extension is requested + * from kernel. * * Returns 0 in case of success or -1 on failure. */ int virNetDevTapCreate(char **ifname, int *tapfd, + int tapfdSize, unsigned int flags) { - int fd; + int i; struct ifreq ifr; int ret = -1; - - if ((fd = open("/dev/net/tun", O_RDWR)) < 0) { - virReportSystemError(errno, "%s", - _("Unable to open /dev/net/tun, is tun module loaded?")); - return -1; - } + int fd; memset(&ifr, 0, sizeof(ifr)); + for (i = 0; i < tapfdSize; i++) { + if ((fd = open("/dev/net/tun", O_RDWR)) < 0) { + virReportSystemError(errno, "%s", + _("Unable to open /dev/net/tun, is tun module loaded?")); + goto cleanup; + } - ifr.ifr_flags = IFF_TAP|IFF_NO_PI; + memset(&ifr, 0, sizeof(ifr)); + + ifr.ifr_flags = IFF_TAP | IFF_NO_PI; + /* If tapfdSize is greater than one, request multiqueue */ + if (tapfdSize > 1) + ifr.ifr_flags |= IFF_MULTI_QUEUE; # ifdef IFF_VNET_HDR - if ((flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) && - virNetDevProbeVnetHdr(fd)) - ifr.ifr_flags |= IFF_VNET_HDR; + if ((flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) && + virNetDevProbeVnetHdr(fd)) + ifr.ifr_flags |= IFF_VNET_HDR; # endif - if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { - virReportSystemError(ERANGE, - _("Network interface name '%s' is too long"), - *ifname); - goto cleanup; + if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + *ifname); + goto cleanup; - } + } - if (ioctl(fd, TUNSETIFF, &ifr) < 0) { - virReportSystemError(errno, - _("Unable to create tap device %s"), - NULLSTR(*ifname)); - goto cleanup; - } + if (ioctl(fd, TUNSETIFF, &ifr) < 0) { + virReportSystemError(errno, + _("Unable to create tap device %s"), + NULLSTR(*ifname)); + goto cleanup; + } - if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) && - (errno = ioctl(fd, TUNSETPERSIST, 1))) { - virReportSystemError(errno, - _("Unable to set tap device %s to persistent"), - NULLSTR(*ifname)); - goto cleanup; - } + if (i == 0) { + /* In case we are looping more than once, set other + * TAPs to have the same name */ + VIR_FREE(*ifname); + if (ifr.ifr_name && VIR_STRDUP(*ifname, ifr.ifr_name) < 0) + goto cleanup; + } - VIR_FREE(*ifname); - if (!(*ifname = strdup(ifr.ifr_name))) { - virReportOOMError(); - goto cleanup; + if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) && + (errno = ioctl(fd, TUNSETPERSIST, 1))) { + virReportSystemError(errno, + _("Unable to set tap device %s to persistent"), + NULLSTR(*ifname)); + goto cleanup; + } + tapfd[i] = fd; } - if (tapfd) - *tapfd = fd; - else - VIR_FORCE_CLOSE(fd); ret = 0; cleanup: - if (ret < 0) + if (ret < 0) { VIR_FORCE_CLOSE(fd); + while (i--) + VIR_FORCE_CLOSE(tapfd[i]); + } return ret; } @@ -266,6 +276,7 @@ cleanup: #else /* ! TUNSETIFF */ int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED, int *tapfd ATTRIBUTE_UNUSED, + int tapfdSize ATTRIBUTE_UNUSED, unsigned int flags ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", @@ -286,7 +297,8 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) * @brname: the bridge name * @ifname: the interface name (or name template) * @macaddr: desired MAC address - * @tapfd: file descriptor return value for the new tap device + * @tapfd: array of file descriptor return value for the new tap device + * @tapfdSize: size of @tapfd * @virtPortProfile: bridge/port specific configuration * @flags: OR of virNetDevTapCreateFlags: @@ -314,6 +326,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, const virMacAddrPtr macaddr, const unsigned char *vmuuid, int *tapfd, + int tapfdSize, virNetDevVPortProfilePtr virtPortProfile, virNetDevVlanPtr virtVlan, unsigned int flags) @@ -321,7 +334,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, virMacAddr tapmac; char macaddrstr[VIR_MAC_STRING_BUFLEN]; - if (virNetDevTapCreate(ifname, tapfd, flags) < 0) + if (virNetDevTapCreate(ifname, tapfd, tapfdSize, flags) < 0) return -1; /* We need to set the interface MAC before adding it @@ -372,9 +385,9 @@ int virNetDevTapCreateInBridgePort(const char *brname, return 0; - error: - if (tapfd) - VIR_FORCE_CLOSE(*tapfd); +error: + while (tapfdSize) + VIR_FORCE_CLOSE(tapfd[--tapfdSize]); return -1; } diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index 6bfc80c..cb6c284 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -29,6 +29,7 @@ int virNetDevTapCreate(char **ifname, int *tapfd, + int tapfdSize, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; @@ -55,6 +56,7 @@ int virNetDevTapCreateInBridgePort(const char *brname, const virMacAddrPtr macaddr, const unsigned char *vmuuid, int *tapfd, + int tapfdSize, virNetDevVPortProfilePtr virtPortProfile, virNetDevVlanPtr virtVlan, unsigned int flags) -- 1.8.2.1

With multiqueue network feature, we are advised to pass multiple vhost-net FDs as well. The ratio should be 1:1. Therefore we must alter the qemuOpenVhostNet function to allow that. --- src/qemu/qemu_command.c | 60 ++++++++++++++++++++++++++++++++++++------------- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 12 ++++++---- 3 files changed, 54 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a053d49..ec66a33 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -405,17 +405,34 @@ cleanup: } +/** + * qemuOpenVhostNet: + * @def: domain definition + * @net: network definition + * @qemuCaps: qemu binary capabilities + * @vhostfd: array of opened vhost-net device + * @vhostfdSize: size of @vhostfd array + * + * Open vhost-net, multiple times - if requested. + * In case, no vhost-net is needed, @vhostfdSize is set to 0 + * and 0 is returned. + * + * Returns: 0 on success + * -1 on failure + */ int qemuOpenVhostNet(virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, - int *vhostfd) + int *vhostfd, + int *vhostfdSize) { - *vhostfd = -1; /* assume we won't use vhost */ + int i; /* If the config says explicitly to not use vhost, return now */ if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU) { - return 0; + *vhostfdSize = 0; + return 0; } /* If qemu doesn't support vhost-net mode (including the -netdev command @@ -430,6 +447,7 @@ qemuOpenVhostNet(virDomainDefPtr def, "this QEMU binary")); return -1; } + *vhostfdSize = 0; return 0; } @@ -441,23 +459,32 @@ qemuOpenVhostNet(virDomainDefPtr def, "virtio network interfaces")); return -1; } + *vhostfdSize = 0; return 0; } - *vhostfd = open("/dev/vhost-net", O_RDWR); - virDomainAuditNetDevice(def, net, "/dev/vhost-net", *vhostfd >= 0); + for (i = 0; i < *vhostfdSize; i++) { + vhostfd[i] = open("/dev/vhost-net", O_RDWR); + virDomainAuditNetDevice(def, net, "/dev/vhost-net", vhostfd[i] >= 0); - /* If the config says explicitly to use vhost and we couldn't open it, - * report an error. - */ - if ((*vhostfd < 0) && - (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("vhost-net was requested for an interface, " - "but is unavailable")); - return -1; + /* If the config says explicitly to use vhost and we couldn't open it, + * report an error. + */ + if (vhostfd[i] < 0 && + net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("vhost-net was requested for an interface, " + "but is unavailable")); + goto error; + } } return 0; + +error: + while (i--) + VIR_FORCE_CLOSE(vhostfd[i]); + + return -1; } int @@ -6477,10 +6504,11 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, /* Attempt to use vhost-net mode for these types of network device */ int vhostfd; + int vhostfdSize = 1; - if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0) + if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd, &vhostfdSize) < 0) goto cleanup; - if (vhostfd >= 0) { + if (vhostfdSize > 0) { virCommandTransferFD(cmd, vhostfd); if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) { virReportOOMError(); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 6765c3a..c87b754 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -170,7 +170,8 @@ int qemuPhysIfaceConnect(virDomainDefPtr def, int qemuOpenVhostNet(virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, - int *vhostfd); + int *vhostfd, + int *vhostfdSize); int qemuNetworkPrepareDevices(virDomainDefPtr def); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0a1845a..fdc4b24 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -689,6 +689,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, int tapfd = -1; char *vhostfd_name = NULL; int vhostfd = -1; + int vhostfdSize = 0; char *nicstr = NULL; char *netstr = NULL; virNetDevVPortProfilePtr vport = NULL; @@ -738,7 +739,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, priv->qemuCaps)) < 0) goto cleanup; iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + vhostfdSize = 1; + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net, @@ -746,10 +748,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) goto cleanup; iface_connected = true; - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + vhostfdSize = 1; + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0) + vhostfdSize = 1; + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd, &vhostfdSize) < 0) goto cleanup; } @@ -792,7 +796,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, goto no_memory; } - if (vhostfd != -1) { + if (vhostfdSize > 0) { if (virAsprintf(&vhostfd_name, "vhostfd-%s", net->info.alias) < 0) goto no_memory; } -- 1.8.2.1

On 05/16/2013 08:49 AM, Michal Privoznik wrote: I just realized while reviewing the next patch that we probably only want a single audit message here, rather than one for each individual fd.

On 05/16/2013 08:49 AM, Michal Privoznik wrote: since qemuCreateInBridgePortWithHelper() can't possibly produce more than a single fd, and the callers to qemuNetworkIfaceConnect have no way of knowing that, we need to make tapfdSize an int* and change it to 1 here. When creating the vhost-net file descriptors, you emit one audit message per fd, but in this case you're only emitting a single audit message, no matter how many fd's are created. Which is correct? (My guess is that it's better to just emit a single audit message, since there is no info specific to each fd anyway, and no extra access gained by the multiple fds.) (notice that this bit here would fail horribly if we were running non-privileged, multiple queues had been requested, and there was a failure somewhere in this function.)

On 05/16/2013 08:49 AM, Michal Privoznik wrote: I may have mentioned it in earlier patches, but tapfdSize needs to be passed as an int* so that qemuNetworkIfaceConnect() can modify it - otherwise qemuNetworkIfaceConnect could fail (or refuse to do multiple queues), and then this caller would think there was a different number of fds than reality, leading to erroneous cleanup. Doing the modification one layer at a time is good in some ways, but in other ways just creates extra transient code changes that need examination (not to mention needing to make sure that each step not only compiles but also runs properly). Truthfully, I wouldn't mind if all the changes to pass it all the way down/up the stack were in a single patch. But it's okay this way too, as long as it passes all the build and runtime tests at each step.

On 05/16/2013 08:49 AM, Michal Privoznik wrote: Again, tapfdSize needs to be an int*, but I already mentioned that in the last patch. macvtap doesn't support multiple queues? But macvtap is supposed to be the panacea of high performance... I am *really* losing track of which layer of which set of fds is getting the "multi" treatment in each patch. I think there are just too many steps in this patch series. It would be easier to follow if there was a patch to add the config parser/formatter/rng/docs, then just another single patch to wire that data all the way up and down the stack. I find myself spending time at each stage ttrying to decide if something is a bug, or just an interim thing that will be fixed in a later step. Okay, tapfdSize isn't set until/unless tapfd and tapfdName are both successfully allocated... Same here. (I mention this because I was worried about it initially :-) ACK, aside from making tapfdSize an int* (so it can be modified on failure/non-support in qemuIfaceNetworkConnect), and an answer about support for multiple queues when macvtap is used. Oh, actually another problem - in cases where multiple queues are requested and can't be honored due to interface type (or any other reason), I think we should log an error and fail, but it looks like you just ignore it. So I guess that also needs fixing (or an explanation).

--- src/qemu/qemu_command.c | 26 ++++++++++++++++++-------- src/qemu/qemu_hotplug.c | 27 ++++++++++++++++----------- 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5d64705..7661b13 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6499,12 +6499,16 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) { - if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(tapfdName) < 0) { + tapfdSize = net->driver.virtio.queues; + if (!tapfdSize) + tapfdSize = 1; + + if (VIR_ALLOC_N(tapfd, tapfdSize) < 0 || + VIR_ALLOC_N(tapfdName, tapfdSize) < 0) { virReportOOMError(); goto cleanup; } - tapfdSize = 1; if (qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps, tapfd, tapfdSize) < 0) goto cleanup; @@ -6526,11 +6530,15 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { /* Attempt to use vhost-net mode for these types of network device */ - if (VIR_ALLOC(vhostfd) < 0 || VIR_ALLOC(vhostfdName)) { + vhostfdSize = net->driver.virtio.queues; + if (!vhostfdSize) + vhostfdSize = 1; + + if (VIR_ALLOC_N(vhostfd, vhostfdSize) < 0 || + VIR_ALLOC_N(vhostfdName, vhostfdSize)) { virReportOOMError(); goto cleanup; } - vhostfdSize = 1; if (qemuOpenVhostNet(def, net, qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; @@ -6594,15 +6602,17 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, cleanup: if (ret < 0) virDomainConfNWFilterTeardown(net); - for (i = 0; i < tapfdSize; i++) { + for (i = 0; tapfd && i < tapfdSize; i++) { if (ret < 0) VIR_FORCE_CLOSE(tapfd[i]); - VIR_FREE(tapfdName[i]); + if (tapfdName) + VIR_FREE(tapfdName[i]); } - for (i = 0; i < vhostfdSize; i++) { + for (i = 0; vhostfd && i < vhostfdSize; i++) { if (ret < 0) VIR_FORCE_CLOSE(vhostfd[i]); - VIR_FREE(vhostfdName[i]); + if (vhostfdName) + VIR_FREE(vhostfdName[i]); } VIR_FREE(tapfd); VIR_FREE(vhostfd); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 695edc7..bdad4d8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -737,11 +737,14 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { - if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(vhostfd) < 0) { + tapfdSize = vhostfdSize = net->driver.virtio.queues; + if (!tapfdSize) + tapfdSize = vhostfdSize = 0; + if (VIR_ALLOC_N(tapfd, tapfdSize) < 0 || + VIR_ALLOC_N(vhostfd, vhostfdSize) < 0) { virReportOOMError(); goto cleanup; } - tapfdSize = vhostfdSize = 1; if (qemuNetworkIfaceConnect(vm->def, conn, driver, net, priv->qemuCaps, tapfd, tapfdSize) < 0) goto cleanup; @@ -749,11 +752,11 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + tapfdSize = vhostfdSize = 1; if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(vhostfd) < 0) { virReportOOMError(); goto cleanup; } - tapfdSize = vhostfdSize = 1; if ((tapfd[0] = qemuPhysIfaceConnect(vm->def, driver, net, priv->qemuCaps, VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) @@ -762,11 +765,11 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { - if (VIR_ALLOC(vhostfd) < 0) { - virReportOOMError(); - goto cleanup; - } vhostfdSize = 1; + if (VIR_ALLOC(vhostfd) < 0) { + virReportOOMError(); + goto cleanup; + } if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } @@ -956,15 +959,17 @@ cleanup: VIR_FREE(nicstr); VIR_FREE(netstr); - for (i = 0; i < tapfdSize; i++) { + for (i = 0; tapfd && i < tapfdSize; i++) { VIR_FORCE_CLOSE(tapfd[i]); - VIR_FREE(tapfdName[i]); + if (tapfdName) + VIR_FREE(tapfdName[i]); } VIR_FREE(tapfd); VIR_FREE(tapfdName); - for (i = 0; i < vhostfdSize; i++) { + for (i = 0; vhostfd && i < vhostfdSize; i++) { VIR_FORCE_CLOSE(vhostfd[i]); - VIR_FREE(vhostfdName[i]); + if (vhostfdName) + VIR_FREE(vhostfdName[i]); } VIR_FREE(vhostfd); VIR_FREE(vhostfdName); -- 1.8.2.1