On 05/16/2013 08:49 AM, 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 | 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 <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> @@ -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">

"the number of queues to be used for the <a href..."

+ Multiqueue virtio-net</a> feature. If the interface has <code><model + type='virtio'/></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) {

Shouldn't this be <= 0 ?

+ virReportError(VIR_ERR_XML_DETAIL, + _("'queues' attribute must be unsigned integer: %s"),

I still like "positive number" better, both because it is less "programmer-ese", and because "unsigned" implies that 0 is okay, but in this case it isn't.

+ 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");

ACK with those issues fixed.

On 05/16/2013 08:49 AM, Michal Privoznik wrote:

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);

Wait. So it turned out you didn't even need the virQEMUDriverConfigPtr at all? Nice!

+ + 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; + } + }

I didn't notice this before - it's nice that you you moved this common clause out of the BRIDGE and DIRECT cases, but why is it down below the vhostnet handling, instead of right up with the code that created the tapfd?

+ + /* 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; } }

My previous ACK stands, but I would like it even better if you made the code movement I indicated above.

On 05/16/2013 08:49 AM, Michal Privoznik wrote:

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(-)

The fact that this patch and the previous patch have *exactly* the same diffstats has to mean something... :-)

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,

ACK.

On 05/16/2013 08:49 AM, Michal Privoznik wrote:

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; }

ACK.

On 05/16/2013 08:49 AM, Michal Privoznik wrote:

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

"array of file descriptors ..."

+ * @tapfdSize: size of @tapfd

"number of file descriptors in @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

"number of file descriptors in @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)

ACK with comment fixes.

On 05/16/2013 08:49 AM, Michal Privoznik wrote:

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

"number of file descriptors in ..." (since "size" might be mistaken as "size in bytes". I agree that wouldn't make sense to me, but...)

+ * + * 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; }

ACK.

On 20.05.2013 19:15, Laine Stump wrote:

On 05/16/2013 08:49 AM, Michal Privoznik wrote:

...

--- src/qemu/qemu_command.c | 82 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 58 insertions(+), 24 deletions(-)

...

goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop); - if (tapfd < 0) + if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(tapfdName) < 0) { + virReportOOMError(); + goto cleanup; + } + tapfdSize = 1; + tapfd[0] = qemuPhysIfaceConnect(def, driver, net, + qemuCaps, vmop);

macvtap doesn't support multiple queues? But macvtap is supposed to be the panacea of high performance...

Unfortunately not yet. Macvtap is still under development in question of MQ.

...

+ if (tapfd[0] < 0) goto cleanup; }

@@ -6510,28 +6526,34 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { /* Attempt to use vhost-net mode for these types of network device */ - int vhostfd; - int vhostfdSize = 1; + if (VIR_ALLOC(vhostfd) < 0 || VIR_ALLOC(vhostfdName)) { + virReportOOMError(); + goto cleanup; + } + vhostfdSize = 1;

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.

Well, I keep modifying patches as you review them. I think I gonna post one more version - just in the form as you requested.

...

+ + if (qemuOpenVhostNet(def, net, qemuCaps, vhostfd, &vhostfdSize) < 0) + goto cleanup; + }

- if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd, &vhostfdSize) < 0) + for (i = 0; i < tapfdSize; i++) { + virCommandTransferFD(cmd, tapfd[i]); + if (virAsprintf(&tapfdName[i], "%d", tapfd[i]) < 0) { + virReportOOMError(); goto cleanup; - if (vhostfdSize > 0) { - virCommandTransferFD(cmd, vhostfd); - if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) { + } + } + + for (i = 0; i < vhostfdSize; i++) { + if (vhostfd[i] >= 0) { + virCommandTransferFD(cmd, vhostfd[i]); + if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 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 @@ -6544,8 +6566,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { if (!(host = qemuBuildHostNetStr(net, driver, ',', vlan, - &tapfdName, tapfdName ? 1 : 0, - &vhostfdName, vhostfdName ? 1 : 0))) + tapfdName, tapfdSize, + vhostfdName, vhostfdSize))) goto cleanup; virCommandAddArgList(cmd, "-netdev", host, NULL); } @@ -6562,8 +6584,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { if (!(host = qemuBuildHostNetStr(net, driver, ',', vlan, - &tapfdName, tapfdName ? 1 : 0, - &vhostfdName, vhostfdName ? 1 : 0))) + tapfdName, tapfdSize, + vhostfdName, vhostfdSize))) goto cleanup; virCommandAddArgList(cmd, "-net", host, NULL); } @@ -6572,6 +6594,18 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, cleanup: if (ret < 0) virDomainConfNWFilterTeardown(net); + for (i = 0; i < tapfdSize; i++) { + if (ret < 0) + VIR_FORCE_CLOSE(tapfd[i]); + VIR_FREE(tapfdName[i]);

Okay, tapfdSize isn't set until/unless tapfd and tapfdName are both successfully allocated...

...

+ } + for (i = 0; i < vhostfdSize; i++) { + if (ret < 0) + VIR_FORCE_CLOSE(vhostfd[i]); + VIR_FREE(vhostfdName[i]);

Same here. (I mention this because I was worried about it initially :-)

...

+ } + VIR_FREE(tapfd); + VIR_FREE(vhostfd); VIR_FREE(nic); VIR_FREE(host); VIR_FREE(tapfdName);

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).

Yeah, I was thinking about it too. Michal

On 05/16/2013 08:49 AM, Michal Privoznik wrote:

--- 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;

Hmm. So in the case of macvtap, you have a single macvtap device fd and multiple vhost-net fds. Is this correct/complete?

+ 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;

I think you wanted to say ".... = 1;" here, didn't you?

+ 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;

But in this case, for macvtap you hard code to 1 tapfd and 1 vhost-net, which isn't the same as what you do for the initial startup commandline (1 tapfd, n vhost-nets).

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);

The discrepency between startup and hotplug vhostfdSize needs to be fixed (and also I'm still curious if macvtap doesn't support multiple queues, since users of macvtap are likely already the ones who are most performance-conscious, so most likely to want multiple queues). Also you need to set to 1 instead of 0 in the hotplug case when the config says nothing.