On 12/04/2015 07:30 AM, Michal Privoznik wrote: Any reason for the alignment adjustment? If not, it's just fodder for creating merge conflicts later. Aside from that, ACK.

There are few outdated things. Firstly, we don't need to undergo the torture of fopen, fscanf and fclose when we have nice wrapper over that: virFileReadAll. Secondly, we can use dynamically allocated buffer for the interface index. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virnetdevmacvlan.c | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 9384b9f..c1a5f0f 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -237,40 +237,28 @@ int virNetDevMacVLanTapOpen(const char *ifname, int retries) { int ret = -1; - FILE *file = NULL; char *path; int ifindex; - char tapname[50]; + char *tapname = NULL; + char *buf = NULL; + char *c; int tapfd; if (virNetDevSysfsFile(&path, ifname, "ifindex") < 0) return -1; - file = fopen(path, "r"); - - if (!file) { - virReportSystemError(errno, - _("cannot open macvtap file %s to determine " - "interface index"), path); + if (virFileReadAll(path, sizeof(buf), &buf) < 0) goto cleanup; - } - if (fscanf(file, "%d", &ifindex) != 1) { + if (virStrToLong_i(buf, &c, 10, &ifindex) < 0 || *c != '\n') { virReportSystemError(errno, "%s", _("cannot determine macvtap's tap device " - "interface index")); + "interface index")); goto cleanup; } - VIR_FORCE_FCLOSE(file); - - if (snprintf(tapname, sizeof(tapname), - "/dev/tap%d", ifindex) >= sizeof(tapname)) { - virReportSystemError(errno, - "%s", - _("internal buffer for tap device is too small")); + if (virAsprintf(&tapname, "/dev/tap%d", ifindex) < 0) goto cleanup; - } while (1) { /* may need to wait for udev to be done */ @@ -292,7 +280,8 @@ int virNetDevMacVLanTapOpen(const char *ifname, ret = tapfd; cleanup: VIR_FREE(path); - VIR_FORCE_FCLOSE(file); + VIR_FREE(tapname); + VIR_FREE(buf); return ret; } -- 2.4.10

On 12/04/2015 07:30 AM, Michal Privoznik wrote: s/So, for the multiqueue/For multiqueue support/ Although libvirt very commonly uses goto essentially for "exception handling in C", and that does make the code quite a bit cleaner, I still have an aversion to goto created by the graders in my very first Pascal class eons ago - if you had a goto, it wasn't accepted unless you explained how you would have written the code without it, and then they *still* took half a mark from your grade. This is why when I see the above, I would prefer to write something like this: for (i = 0; i < tapfdSize; i++) { int fd = -1; while (fd < 0) { if ((fd = open(tapname, O_RDWR)) >= 0) { tapfd[i] = fd; } else if (retries-- > 0) { usleep(20000); } else { virReportSystemError(errno, _("cannot open macvtap tap device %s"), tapname); goto cleanup; } } } Note that this has the effect of allowing some retries to count when opening the first fd, and other retries to count when opening the others (similar to your loop, btw). Nothing wrong with this, since the retries are there to wait for udev to complete creation of the tap device (so once it's done, we'll never need to wait for it again), just pointing it out. ACK. It's up to you whether you want to use the loop with an extra label or the one without; I'm not going to insist on it (but my grader from [redacted to avoid shocking anyone] years ago would frown at me in my sleep if I didn't mention it :-))

On 12/04/2015 07:31 AM, Michal Privoznik wrote: Either merge 3 & 4 together, or change the log message (I'm okay with multiple functions at the same level being changed in the same patch). Comparing this to the VNET_HDR stuff for tap devices, I see that in that case lack of support for VNET_HDR isn't fatal - it simply isn't set. My guess is that VNET_HDR support was probably added prior to macvtap, so that it's always there if we're doing macvtap, but I don't know for sure. (I'm just wondering if maybe there's some way code could be shared between the two to reduce maintenance). The other difference is that IFF_MULTI_QUEUE is set on each fd for a tap device. Is this also needed for macvtap?

So, for the multiqueue on macvtaps we are going to need to open the device multiple times. Currently, this is not supported. Rework the function, so that upper layers can be reworked too. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/lxc/lxc_process.c | 1 + src/qemu/qemu_command.c | 21 ++++++++++++--------- src/util/virnetdevmacvlan.c | 18 +++++++++++++----- src/util/virnetdevmacvlan.h | 4 +++- 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0ada6e4..f7e2b81 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -349,6 +349,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, + NULL, 0, macvlan_create_flags) < 0) goto cleanup; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 55809e9..d1ef4ab 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -230,15 +230,18 @@ qemuPhysIfaceConnect(virDomainDefPtr def, if (net->model && STREQ(net->model, "virtio")) macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR; - rc = virNetDevMacVLanCreateWithVPortProfile( - net->ifname, &net->mac, - virDomainNetGetActualDirectDev(net), - virDomainNetGetActualDirectMode(net), - def->uuid, - virDomainNetGetActualVirtPortProfile(net), - &res_ifname, - vmop, cfg->stateDir, - macvlan_create_flags); + if (virNetDevMacVLanCreateWithVPortProfile(net->ifname, + &net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + def->uuid, + virDomainNetGetActualVirtPortProfile(net), + &res_ifname, + vmop, cfg->stateDir, + &rc, 1, + macvlan_create_flags) < 0) + return -1; + if (rc >= 0) { virDomainAuditNetDevice(def, net, res_ifname, true); VIR_FREE(net->ifname); diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index f7a7d72..244e01a 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -739,11 +739,15 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * @res_ifname: Pointer to a string pointer where the actual name of the * interface will be stored into if everything succeeded. It is up * to the caller to free the string. + * @tapfd: array of file descriptor return value for the new tap device + * @tapfdSize: number of file descriptors in @tapfd * @flags: OR of virNetDevMacVLanCreateFlags. * - * Returns file descriptor of the tap device in case of success with - * @flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; returns - * -1 on error. + * Creates a macvlan device. Optionally, if flags & + * VIR_NETDEV_MACVLAN_CREATE_WITH_TAP is set, @tapfd is populated with FDs of + * tap devices up to @tapfdSize. + * + * Return 0 on success, -1 on error. */ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, const virMacAddr *macaddress, @@ -754,6 +758,8 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, char **res_ifname, virNetDevVPortProfileOp vmOp, char *stateDir, + int *tapfd, + size_t tapfdSize, unsigned int flags) { const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? @@ -865,10 +871,10 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, } if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { - if (virNetDevMacVLanTapOpen(cr_ifname, &rc, 1, 10) < 0) + if (virNetDevMacVLanTapOpen(cr_ifname, tapfd, tapfdSize, 10) < 0) goto disassociate_exit; - if (virNetDevMacVLanTapSetup(&rc, 1, vnet_hdr) < 0) { + if (virNetDevMacVLanTapSetup(tapfd, tapfdSize, vnet_hdr) < 0) { VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ goto disassociate_exit; } @@ -1028,6 +1034,8 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname ATTRIBUTE_UNUSED, char **res_ifname ATTRIBUTE_UNUSED, virNetDevVPortProfileOp vmop ATTRIBUTE_UNUSED, char *stateDir ATTRIBUTE_UNUSED, + int *tapfd ATTRIBUTE_UNUSED, + size_t tapfdSize ATTRIBUTE_UNUSED, unsigned int unused_flags ATTRIBUTE_UNUSED) { virReportSystemError(ENOSYS, "%s", diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index b04fc8c..08ccdd2 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -71,9 +71,11 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname, char **res_ifname, virNetDevVPortProfileOp vmop, char *stateDir, + int *tapfd, + size_t tapfdSize, unsigned int flags) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(6) - ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(10) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(12) ATTRIBUTE_RETURN_CHECK; int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, const virMacAddr *macaddress, -- 2.4.10

https://bugzilla.redhat.com/show_bug.cgi?id=1240439 Ta-da! Now that we know how to open a macvtap device multiple times, we can finally enable the multiqueue feature. Everything else is already prepared (e.g. command line generation) from the previous iteration where the feature was implemented for TUN/TAP devices. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_command.c | 41 ++++++++++++++++++++++++++--------------- src/qemu/qemu_command.h | 2 ++ src/qemu/qemu_hotplug.c | 16 +++++++++------- 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d1ef4ab..821204e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -212,17 +212,21 @@ qemuVirCommandGetDevSet(virCommandPtr cmd, int fd) * @def: the definition of the VM (needed by 802.1Qbh and audit) * @driver: pointer to the driver instance * @net: pointer to the VM's interface description with direct device type + * @tapfd: array of file descriptor return value for the new device + * @tapfdSize: number of file descriptors in @tapfd * @vmop: VM operation type * - * Returns a filedescriptor on success or -1 in case of error. + * Returns 0 on success or -1 in case of error. */ int qemuPhysIfaceConnect(virDomainDefPtr def, virQEMUDriverPtr driver, virDomainNetDefPtr net, + int *tapfd, + size_t tapfdSize, virNetDevVPortProfileOp vmop) { - int rc; + int ret = -1; char *res_ifname = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP; @@ -238,18 +242,18 @@ qemuPhysIfaceConnect(virDomainDefPtr def, virDomainNetGetActualVirtPortProfile(net), &res_ifname, vmop, cfg->stateDir, - &rc, 1, + tapfd, tapfdSize, macvlan_create_flags) < 0) - return -1; + goto cleanup; - if (rc >= 0) { - virDomainAuditNetDevice(def, net, res_ifname, true); - VIR_FREE(net->ifname); - net->ifname = res_ifname; - } + virDomainAuditNetDevice(def, net, res_ifname, true); + VIR_FREE(net->ifname); + net->ifname = res_ifname; + ret = 0; + cleanup: virObjectUnref(cfg); - return rc; + return ret; } @@ -8746,7 +8750,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, /* Currently nothing besides TAP devices supports multiqueue. */ if (net->driver.virtio.queues > 0 && !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE)) { + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_DIRECT)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Multiqueue network is not supported for: %s"), virDomainNetTypeToString(actualType)); @@ -8791,11 +8796,17 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, tapfd, &tapfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { - 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) goto cleanup; - tapfdSize = 1; - tapfd[0] = qemuPhysIfaceConnect(def, driver, net, vmop); - if (tapfd[0] < 0) + + memset(tapfd, -1, tapfdSize * sizeof(tapfd[0])); + + if (qemuPhysIfaceConnect(def, driver, net, tapfd, tapfdSize, vmop) < 0) goto cleanup; } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index bebdd27..f0d6900 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -232,6 +232,8 @@ int qemuNetworkIfaceConnect(virDomainDefPtr def, int qemuPhysIfaceConnect(virDomainDefPtr def, virQEMUDriverPtr driver, virDomainNetDefPtr net, + int *tapfd, + size_t tapfdSize, virNetDevVPortProfileOp vmop); int qemuOpenVhostNet(virDomainDefPtr def, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8804d3d..1a3b278 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -947,15 +947,17 @@ 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) + tapfdSize = vhostfdSize = net->driver.virtio.queues; + if (!tapfdSize) + tapfdSize = vhostfdSize = 1; + if (VIR_ALLOC_N(tapfd, tapfdSize) < 0) goto cleanup; - *tapfd = -1; - if (VIR_ALLOC(vhostfd) < 0) + memset(tapfd, -1, sizeof(*tapfd) * tapfdSize); + if (VIR_ALLOC_N(vhostfd, vhostfdSize) < 0) goto cleanup; - *vhostfd = -1; - if ((tapfd[0] = qemuPhysIfaceConnect(vm->def, driver, net, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) + memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize); + if (qemuPhysIfaceConnect(vm->def, driver, net, tapfd, tapfdSize, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE) < 0) goto cleanup; iface_connected = true; if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) -- 2.4.10