[libvirt] [PATCH v2 0/7] Add multiqueue support for macvtaps
Patches 1, 2, 3, 6, 7 have been ACKed in previous round. However, I did slightly change them to reflect Laine's review suggestions. Patch 4 has not been ACKed yet, patch 5 is new. Michal Privoznik (7): virNetDevMacVLanCreateWithVPortProfile: Turn vnet_hdr into flag virNetDevMacVLanTapOpen: Slightly rework virNetDevMacVLanTapOpen: Rework to support multiple FDs virNetDevMacVLanTapSetup: Rework to support multiple FDs virNetDevMacVLanTapSetup: Allow enabling of IFF_MULTI_QUEUE virNetDevMacVLanCreateWithVPortProfile: Rework to support multiple FDs qemu: Enable multiqueue for macvtaps src/lxc/lxc_process.c | 3 +- src/qemu/qemu_command.c | 65 ++++++++++------ src/qemu/qemu_command.h | 2 + src/qemu/qemu_hotplug.c | 16 ++-- src/util/virnetdevmacvlan.c | 185 ++++++++++++++++++++++---------------------- src/util/virnetdevmacvlan.h | 7 +- 6 files changed, 153 insertions(+), 125 deletions(-) -- 2.4.10
So yet again one of integer arguments that we use as a boolean. Since the argument count of the function is unbearably long enough, lets turn those booleans into flags. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_command.c | 5 ++--- src/util/virnetdevmacvlan.c | 4 +--- src/util/virnetdevmacvlan.h | 3 ++- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 57e3880..0ada6e4 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -344,7 +344,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, net->ifname, &net->mac, linkdev, virDomainNetGetActualDirectMode(net), - false, def->uuid, + def->uuid, prof, &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ad0a22d..b1febed 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -224,18 +224,17 @@ qemuPhysIfaceConnect(virDomainDefPtr def, { int rc; char *res_ifname = NULL; - int vnet_hdr = 0; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP; if (net->model && STREQ(net->model, "virtio")) - vnet_hdr = 1; + macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR; rc = virNetDevMacVLanCreateWithVPortProfile( net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), virDomainNetGetActualDirectMode(net), - vnet_hdr, def->uuid, + def->uuid, virDomainNetGetActualVirtPortProfile(net), &res_ifname, vmop, cfg->stateDir, diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index de345e6..9384b9f 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -727,7 +727,6 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * @macaddress: The MAC address for the macvtap device * @linkdev: The interface name of the NIC to connect to the external bridge * @mode: int describing the mode for 'bridge', 'vepa', 'private' or 'passthru'. - * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it * @vmuuid: The UUID of the VM the macvtap belongs to * @virtPortProfile: pointer to object holding the virtual port profile data * @res_ifname: Pointer to a string pointer where the actual name of the @@ -743,7 +742,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, const virMacAddr *macaddress, const char *linkdev, virNetDevMacVLanMode mode, - int vnet_hdr, const unsigned char *vmuuid, virNetDevVPortProfilePtr virtPortProfile, char **res_ifname, @@ -764,6 +762,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, const char *cr_ifname = NULL; int ret; int vf = -1; + bool vnet_hdr = flags & VIR_NETDEV_MACVLAN_VNET_HDR; macvtapMode = modeMap[mode]; @@ -1017,7 +1016,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname ATTRIBUTE_UNUSED, const virMacAddr *macaddress ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, virNetDevMacVLanMode mode ATTRIBUTE_UNUSED, - int vnet_hdr ATTRIBUTE_UNUSED, const unsigned char *vmuuid ATTRIBUTE_UNUSED, virNetDevVPortProfilePtr virtPortProfile ATTRIBUTE_UNUSED, char **res_ifname ATTRIBUTE_UNUSED, diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 298e522..0613f4d 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -46,6 +46,8 @@ typedef enum { VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, /* Bring the interface up */ VIR_NETDEV_MACVLAN_CREATE_IFUP = 1 << 1, + /* Enable VNET_HDR */ + VIR_NETDEV_MACVLAN_VNET_HDR = 1 << 2, } virNetDevMacVLanCreateFlags; int virNetDevMacVLanCreate(const char *ifname, @@ -64,7 +66,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname, const virMacAddr *macaddress, const char *linkdev, virNetDevMacVLanMode mode, - int vnet_hdr, const unsigned char *vmuuid, virNetDevVPortProfilePtr virtPortProfile, char **res_ifname, -- 2.4.10
There are few outdated things. Firstly, we don't need to undergo the torture of fopen, fscanf and fclose just to get the interface index when we have nice wrapper over that: virNetDevGetIndex. Secondly, we don't need to have statically allocated buffer for the path we are opening. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevmacvlan.c | 34 ++++------------------------------ 1 file changed, 4 insertions(+), 30 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 9384b9f..4657ff2 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -237,40 +237,15 @@ int virNetDevMacVLanTapOpen(const char *ifname, int retries) { int ret = -1; - FILE *file = NULL; - char *path; int ifindex; - char tapname[50]; + char *tapname = NULL; int tapfd; - if (virNetDevSysfsFile(&path, ifname, "ifindex") < 0) + if (virNetDevGetIndex(ifname, &ifindex) < 0) return -1; - file = fopen(path, "r"); - - if (!file) { - virReportSystemError(errno, - _("cannot open macvtap file %s to determine " - "interface index"), path); + if (virAsprintf(&tapname, "/dev/tap%d", ifindex) < 0) goto cleanup; - } - - if (fscanf(file, "%d", &ifindex) != 1) { - virReportSystemError(errno, - "%s", _("cannot determine macvtap's tap device " - "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")); - goto cleanup; - } while (1) { /* may need to wait for udev to be done */ @@ -291,8 +266,7 @@ int virNetDevMacVLanTapOpen(const char *ifname, } ret = tapfd; cleanup: - VIR_FREE(path); - VIR_FORCE_FCLOSE(file); + VIR_FREE(tapname); return ret; } -- 2.4.10
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@redhat.com> --- src/util/virnetdevmacvlan.c | 57 ++++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 4657ff2..c2b60eb 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -226,20 +226,26 @@ int virNetDevMacVLanDelete(const char *ifname) /** * virNetDevMacVLanTapOpen: - * Open the macvtap's tap device. * @ifname: Name of the macvtap interface + * @tapfd: array of file descriptor return value for the new macvtap device + * @tapfdSize: number of file descriptors in @tapfd * @retries : Number of retries in case udev for example may need to be * waited for to create the tap chardev - * Returns negative value in case of error, the file descriptor otherwise. + * + * Open the macvtap's tap device, possibly multiple times if @tapfdSize > 1. + * + * Returns 0 on success, -1 otherwise. */ -static -int virNetDevMacVLanTapOpen(const char *ifname, - int retries) +static int +virNetDevMacVLanTapOpen(const char *ifname, + int *tapfd, + size_t tapfdSize, + int retries) { int ret = -1; int ifindex; char *tapname = NULL; - int tapfd; + size_t i = 0; if (virNetDevGetIndex(ifname, &ifindex) < 0) return -1; @@ -247,25 +253,32 @@ int virNetDevMacVLanTapOpen(const char *ifname, if (virAsprintf(&tapname, "/dev/tap%d", ifindex) < 0) goto cleanup; - while (1) { - /* may need to wait for udev to be done */ - tapfd = open(tapname, O_RDWR); - if (tapfd < 0 && retries > 0) { - retries--; - usleep(20000); - continue; + 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) { + /* may need to wait for udev to be done */ + usleep(20000); + } else { + /* However, if haven't succeeded, quit. */ + virReportSystemError(errno, + _("cannot open macvtap tap device %s"), + tapname); + goto cleanup; + } } - break; } - if (tapfd < 0) { - virReportSystemError(errno, - _("cannot open macvtap tap device %s"), - tapname); - goto cleanup; - } - ret = tapfd; + ret = 0; + cleanup: + if (ret < 0) { + while (i--) + VIR_FORCE_CLOSE(tapfd[i]); + } VIR_FREE(tapname); return ret; } @@ -832,7 +845,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, } if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { - if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10)) < 0) + if (virNetDevMacVLanTapOpen(cr_ifname, &rc, 1, 10) < 0) goto disassociate_exit; if (virNetDevMacVLanTapSetup(rc, vnet_hdr) < 0) { -- 2.4.10
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@redhat.com> --- src/util/virnetdevmacvlan.c | 66 ++++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index c2b60eb..c4d0d53 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -286,10 +286,9 @@ virNetDevMacVLanTapOpen(const char *ifname, /** * virNetDevMacVLanTapSetup: - * @tapfd: file descriptor of the macvtap tap - * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it - * - * Returns 0 on success, -1 in case of fatal error, error code otherwise. + * @tapfd: array of file descriptors of the macvtap tap + * @tapfdSize: number of file descriptors in @tapfd + * @vnet_hdr: whether to enable or disable IFF_VNET_HDR * * Turn the IFF_VNET_HDR flag, if requested and available, make sure * it's off in the other cases. @@ -297,47 +296,52 @@ virNetDevMacVLanTapOpen(const char *ifname, * be turned off for some reason. This is reported with -1. Other fatal * error is not being able to read the interface flags. In that case the * macvtap device should not be used. + * + * Returns 0 on success, -1 in case of fatal error, error code otherwise. */ static int -virNetDevMacVLanTapSetup(int tapfd, int vnet_hdr) +virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr) { unsigned int features; struct ifreq ifreq; short new_flags = 0; int rc_on_fail = 0; const char *errmsg = NULL; + size_t i; - memset(&ifreq, 0, sizeof(ifreq)); + for (i = 0; i < tapfdSize; i++) { + memset(&ifreq, 0, sizeof(ifreq)); - if (ioctl(tapfd, TUNGETIFF, &ifreq) < 0) { - virReportSystemError(errno, "%s", - _("cannot get interface flags on macvtap tap")); - return -1; - } - - new_flags = ifreq.ifr_flags; - - if ((ifreq.ifr_flags & IFF_VNET_HDR) && !vnet_hdr) { - new_flags = ifreq.ifr_flags & ~IFF_VNET_HDR; - rc_on_fail = -1; - errmsg = _("cannot clean IFF_VNET_HDR flag on macvtap tap"); - } else if ((ifreq.ifr_flags & IFF_VNET_HDR) == 0 && vnet_hdr) { - if (ioctl(tapfd, TUNGETFEATURES, &features) < 0) { + if (ioctl(tapfd[i], TUNGETIFF, &ifreq) < 0) { virReportSystemError(errno, "%s", - _("cannot get feature flags on macvtap tap")); + _("cannot get interface flags on macvtap tap")); return -1; } - if ((features & IFF_VNET_HDR)) { - new_flags = ifreq.ifr_flags | IFF_VNET_HDR; - errmsg = _("cannot set IFF_VNET_HDR flag on macvtap tap"); + + new_flags = ifreq.ifr_flags; + + if ((ifreq.ifr_flags & IFF_VNET_HDR) && !vnet_hdr) { + new_flags = ifreq.ifr_flags & ~IFF_VNET_HDR; + rc_on_fail = -1; + errmsg = _("cannot clean IFF_VNET_HDR flag on macvtap tap"); + } else if ((ifreq.ifr_flags & IFF_VNET_HDR) == 0 && vnet_hdr) { + if (ioctl(tapfd[i], TUNGETFEATURES, &features) < 0) { + virReportSystemError(errno, "%s", + _("cannot get feature flags on macvtap tap")); + return -1; + } + if ((features & IFF_VNET_HDR)) { + new_flags = ifreq.ifr_flags | IFF_VNET_HDR; + errmsg = _("cannot set IFF_VNET_HDR flag on macvtap tap"); + } } - } - if (new_flags != ifreq.ifr_flags) { - ifreq.ifr_flags = new_flags; - if (ioctl(tapfd, TUNSETIFF, &ifreq) < 0) { - virReportSystemError(errno, "%s", errmsg); - return rc_on_fail; + if (new_flags != ifreq.ifr_flags) { + ifreq.ifr_flags = new_flags; + if (ioctl(tapfd[i], TUNSETIFF, &ifreq) < 0) { + virReportSystemError(errno, "%s", errmsg); + return rc_on_fail; + } } } @@ -848,7 +852,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, if (virNetDevMacVLanTapOpen(cr_ifname, &rc, 1, 10) < 0) goto disassociate_exit; - if (virNetDevMacVLanTapSetup(rc, vnet_hdr) < 0) { + if (virNetDevMacVLanTapSetup(&rc, 1, vnet_hdr) < 0) { VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ goto disassociate_exit; } -- 2.4.10
Like we are doing for TUN/TAP devices, we should do the same for macvtaps. Although, it's not as critical as in that case, we should do it for the consistency. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevmacvlan.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index c4d0d53..76fd542 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -289,24 +289,26 @@ virNetDevMacVLanTapOpen(const char *ifname, * @tapfd: array of file descriptors of the macvtap tap * @tapfdSize: number of file descriptors in @tapfd * @vnet_hdr: whether to enable or disable IFF_VNET_HDR + * @multiqueue: whether to enable or disable IFF_MULTI_QUEUE + * + * Turn the IFF_VNET_HDR flag, if requested and available, make sure it's + * off in the other cases. Similarly, IFF_MULTI_QUEUE is enabled if + * requested. However, if requested and failed to set, it is considered a + * fatal error (as opposed to @vnet_hdr). * - * Turn the IFF_VNET_HDR flag, if requested and available, make sure - * it's off in the other cases. * A fatal error is defined as the VNET_HDR flag being set but it cannot * be turned off for some reason. This is reported with -1. Other fatal * error is not being able to read the interface flags. In that case the * macvtap device should not be used. * - * Returns 0 on success, -1 in case of fatal error, error code otherwise. + * Returns 0 on success, -1 in case of fatal error. */ static int -virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr) +virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr, bool multiqueue) { unsigned int features; struct ifreq ifreq; short new_flags = 0; - int rc_on_fail = 0; - const char *errmsg = NULL; size_t i; for (i = 0; i < tapfdSize; i++) { @@ -320,27 +322,29 @@ virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr) new_flags = ifreq.ifr_flags; - if ((ifreq.ifr_flags & IFF_VNET_HDR) && !vnet_hdr) { - new_flags = ifreq.ifr_flags & ~IFF_VNET_HDR; - rc_on_fail = -1; - errmsg = _("cannot clean IFF_VNET_HDR flag on macvtap tap"); - } else if ((ifreq.ifr_flags & IFF_VNET_HDR) == 0 && vnet_hdr) { + if (vnet_hdr) { if (ioctl(tapfd[i], TUNGETFEATURES, &features) < 0) { virReportSystemError(errno, "%s", _("cannot get feature flags on macvtap tap")); return -1; } - if ((features & IFF_VNET_HDR)) { - new_flags = ifreq.ifr_flags | IFF_VNET_HDR; - errmsg = _("cannot set IFF_VNET_HDR flag on macvtap tap"); - } + if (features & IFF_VNET_HDR) + new_flags |= IFF_VNET_HDR; + } else { + new_flags &= ~IFF_VNET_HDR; } + if (multiqueue) + new_flags |= IFF_MULTI_QUEUE; + else + new_flags &= ~IFF_MULTI_QUEUE; + if (new_flags != ifreq.ifr_flags) { ifreq.ifr_flags = new_flags; if (ioctl(tapfd[i], TUNSETIFF, &ifreq) < 0) { - virReportSystemError(errno, "%s", errmsg); - return rc_on_fail; + virReportSystemError(errno, "%s", + _("unable to set vnet or multiqueue flags on macvtap")); + return -1; } } } @@ -852,7 +856,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, if (virNetDevMacVLanTapOpen(cr_ifname, &rc, 1, 10) < 0) goto disassociate_exit; - if (virNetDevMacVLanTapSetup(&rc, 1, vnet_hdr) < 0) { + if (virNetDevMacVLanTapSetup(&rc, 1, vnet_hdr, false) < 0) { VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ goto disassociate_exit; } -- 2.4.10
Hello, On Thu, 2015-12-10 at 08:38 +0100, Michal Privoznik wrote:
Like we are doing for TUN/TAP devices, we should do the same for macvtaps. Although, it's not as critical as in that case, we should do it for the consistency.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This has triggered a build failure on amd64+i386+armhf within the Xen automated test framework (which uses Debian Wheezy as the build environment), I doubt it is in any way Xen specific though: util/virnetdevmacvlan.c: In function 'virNetDevMacVLanTapSetup': util/virnetdevmacvlan.c:338:26: error: 'IFF_MULTI_QUEUE' undeclared (first use in this function) util/virnetdevmacvlan.c:338:26: note: each undeclared identifier is reported only once for each function it appears in I'm not sure where that definition is supposed to come from, so I can't tell if it is a missing #include in this code or an out of date header on the Debian system. Full logs are at http://logs.test-lab.xenproject.org/osstest/logs/65756/ http://logs.test-lab.xenproject.org/osstest/logs/65756/build-amd64-libvirt/5... http://lists.xen.org/archives/html/xen-devel/2015-12/msg01470.html But TBH there isn't much more of use than the above. Cheers, Ian.
--- src/util/virnetdevmacvlan.c | 40 ++++++++++++++++++++++----------------- - 1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index c4d0d53..76fd542 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -289,24 +289,26 @@ virNetDevMacVLanTapOpen(const char *ifname, * @tapfd: array of file descriptors of the macvtap tap * @tapfdSize: number of file descriptors in @tapfd * @vnet_hdr: whether to enable or disable IFF_VNET_HDR + * @multiqueue: whether to enable or disable IFF_MULTI_QUEUE + * + * Turn the IFF_VNET_HDR flag, if requested and available, make sure it's + * off in the other cases. Similarly, IFF_MULTI_QUEUE is enabled if + * requested. However, if requested and failed to set, it is considered a + * fatal error (as opposed to @vnet_hdr). * - * Turn the IFF_VNET_HDR flag, if requested and available, make sure - * it's off in the other cases. * A fatal error is defined as the VNET_HDR flag being set but it cannot * be turned off for some reason. This is reported with -1. Other fatal * error is not being able to read the interface flags. In that case the * macvtap device should not be used. * - * Returns 0 on success, -1 in case of fatal error, error code otherwise. + * Returns 0 on success, -1 in case of fatal error. */ static int -virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr) +virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr, bool multiqueue) { unsigned int features; struct ifreq ifreq; short new_flags = 0; - int rc_on_fail = 0; - const char *errmsg = NULL; size_t i; for (i = 0; i < tapfdSize; i++) { @@ -320,27 +322,29 @@ virNetDevMacVLanTapSetup(int *tapfd, size_t tapfdSize, bool vnet_hdr) new_flags = ifreq.ifr_flags; - if ((ifreq.ifr_flags & IFF_VNET_HDR) && !vnet_hdr) { - new_flags = ifreq.ifr_flags & ~IFF_VNET_HDR; - rc_on_fail = -1; - errmsg = _("cannot clean IFF_VNET_HDR flag on macvtap tap"); - } else if ((ifreq.ifr_flags & IFF_VNET_HDR) == 0 && vnet_hdr) { + if (vnet_hdr) { if (ioctl(tapfd[i], TUNGETFEATURES, &features) < 0) { virReportSystemError(errno, "%s", _("cannot get feature flags on macvtap tap")); return -1; } - if ((features & IFF_VNET_HDR)) { - new_flags = ifreq.ifr_flags | IFF_VNET_HDR; - errmsg = _("cannot set IFF_VNET_HDR flag on macvtap tap"); - } + if (features & IFF_VNET_HDR) + new_flags |= IFF_VNET_HDR; + } else { + new_flags &= ~IFF_VNET_HDR; } + if (multiqueue) + new_flags |= IFF_MULTI_QUEUE; + else + new_flags &= ~IFF_MULTI_QUEUE; + if (new_flags != ifreq.ifr_flags) { ifreq.ifr_flags = new_flags; if (ioctl(tapfd[i], TUNSETIFF, &ifreq) < 0) { - virReportSystemError(errno, "%s", errmsg); - return rc_on_fail; + virReportSystemError(errno, "%s", + _("unable to set vnet or multiqueue flags on macvtap")); + return -1; } } } @@ -852,7 +856,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, if (virNetDevMacVLanTapOpen(cr_ifname, &rc, 1, 10) < 0) goto disassociate_exit; - if (virNetDevMacVLanTapSetup(&rc, 1, vnet_hdr) < 0) { + if (virNetDevMacVLanTapSetup(&rc, 1, vnet_hdr, false) < 0) { VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ goto disassociate_exit; }
On 14.12.2015 11:23, Ian Campbell wrote:
Hello,
On Thu, 2015-12-10 at 08:38 +0100, Michal Privoznik wrote:
Like we are doing for TUN/TAP devices, we should do the same for macvtaps. Although, it's not as critical as in that case, we should do it for the consistency.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This has triggered a build failure on amd64+i386+armhf within the Xen automated test framework (which uses Debian Wheezy as the build environment), I doubt it is in any way Xen specific though:
util/virnetdevmacvlan.c: In function 'virNetDevMacVLanTapSetup': util/virnetdevmacvlan.c:338:26: error: 'IFF_MULTI_QUEUE' undeclared (first use in this function) util/virnetdevmacvlan.c:338:26: note: each undeclared identifier is reported only once for each function it appears in
this is supposed to be fixed by: commit ec93cc25ecdad100a535cb52c08f7eaa3004b960 Author: Michal Privoznik <mprivozn@redhat.com> AuthorDate: Sat Dec 12 08:05:17 2015 +0100 Commit: Michal Privoznik <mprivozn@redhat.com> CommitDate: Sun Dec 13 08:35:46 2015 +0100 virNetDevMacVLanTapSetup: Work around older systems Some older systems, e.g. RHEL-6 do not have IFF_MULTI_QUEUE flag which we use to enable multiqueue feature. Therefore one gets the following compile error there: CC util/libvirt_util_la-virnetdevmacvlan.lo util/virnetdevmacvlan.c: In function 'virNetDevMacVLanTapSetup': util/virnetdevmacvlan.c:338: error: 'IFF_MULTI_QUEUE' undeclared (first use in this function) util/virnetdevmacvlan.c:338: error: (Each undeclared identifier is reported only once util/virnetdevmacvlan.c:338: error: for each function it appears in.) make[3]: *** [util/libvirt_util_la-virnetdevmacvlan.lo] Error 1 So, whenever user wants us to enable the feature on such systems, we will just throw a runtime error instead. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Mon, 2015-12-14 at 12:35 +0100, Michal Privoznik wrote:
On 14.12.2015 11:23, Ian Campbell wrote:
Hello,
On Thu, 2015-12-10 at 08:38 +0100, Michal Privoznik wrote:
Like we are doing for TUN/TAP devices, we should do the same for macvtaps. Although, it's not as critical as in that case, we should do it for the consistency.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This has triggered a build failure on amd64+i386+armhf within the Xen automated test framework (which uses Debian Wheezy as the build environment), I doubt it is in any way Xen specific though:
util/virnetdevmacvlan.c: In function 'virNetDevMacVLanTapSetup': util/virnetdevmacvlan.c:338:26: error: 'IFF_MULTI_QUEUE' undeclared (first use in this function) util/virnetdevmacvlan.c:338:26: note: each undeclared identifier is reported only once for each function it appears in
this is supposed to be fixed by:
Ah, I somehow missed that commit in the logs, sorry. The test run in question had picked up afe73ed46856 which was before the fixup, the next one will pickup the newer version and be fine. Thanks and sorry for the noise. Ian.
commit ec93cc25ecdad100a535cb52c08f7eaa3004b960 Author: Michal Privoznik <mprivozn@redhat.com> AuthorDate: Sat Dec 12 08:05:17 2015 +0100 Commit: Michal Privoznik <mprivozn@redhat.com> CommitDate: Sun Dec 13 08:35:46 2015 +0100
virNetDevMacVLanTapSetup: Work around older systems Some older systems, e.g. RHEL-6 do not have IFF_MULTI_QUEUE flag which we use to enable multiqueue feature. Therefore one gets the following compile error there: CC util/libvirt_util_la-virnetdevmacvlan.lo util/virnetdevmacvlan.c: In function 'virNetDevMacVLanTapSetup': util/virnetdevmacvlan.c:338: error: 'IFF_MULTI_QUEUE' undeclared (first use in this function) util/virnetdevmacvlan.c:338: error: (Each undeclared identifier is reported only once util/virnetdevmacvlan.c:338: error: for each function it appears in.) make[3]: *** [util/libvirt_util_la-virnetdevmacvlan.lo] Error 1 So, whenever user wants us to enable the feature on such systems, we will just throw a runtime error instead. Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Michal
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@redhat.com> --- src/lxc/lxc_process.c | 1 + src/qemu/qemu_command.c | 21 ++++++++++++--------- src/util/virnetdevmacvlan.c | 20 +++++++++++++++----- src/util/virnetdevmacvlan.h | 4 +++- 4 files changed, 31 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 b1febed..d856377 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 76fd542..e62cbda 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -727,11 +727,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, @@ -742,6 +746,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) ? @@ -853,10 +859,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, false) < 0) { + if (virNetDevMacVLanTapSetup(tapfd, tapfdSize, vnet_hdr, tapfdSize > 0) < 0) { VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ goto disassociate_exit; } @@ -892,6 +898,8 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, linkdev, vf, vmOp)); + while (tapfdSize--) + VIR_FORCE_CLOSE(tapfd[tapfdSize]); link_del_exit: ignore_value(virNetDevMacVLanDelete(cr_ifname)); @@ -1016,6 +1024,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 0613f4d..2844a44 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@redhat.com> --- src/qemu/qemu_command.c | 45 ++++++++++++++++++++++++++++++--------------- src/qemu/qemu_command.h | 2 ++ src/qemu/qemu_hotplug.c | 16 +++++++++------- 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d856377..d2f37e4 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,22 @@ 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: + if (ret < 0) { + while (tapfdSize--) + VIR_FORCE_CLOSE(tapfd[tapfdSize]); } - virObjectUnref(cfg); - return rc; + return ret; } @@ -8746,7 +8754,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 +8800,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 1a61701..a88c2f2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -951,15 +951,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
On 12/10/2015 02:38 AM, Michal Privoznik wrote:
Patches 1, 2, 3, 6, 7 have been ACKed in previous round. However, I did slightly change them to reflect Laine's review suggestions. Patch 4 has not been ACKed yet, patch 5 is new.
Michal Privoznik (7): virNetDevMacVLanCreateWithVPortProfile: Turn vnet_hdr into flag virNetDevMacVLanTapOpen: Slightly rework virNetDevMacVLanTapOpen: Rework to support multiple FDs virNetDevMacVLanTapSetup: Rework to support multiple FDs virNetDevMacVLanTapSetup: Allow enabling of IFF_MULTI_QUEUE virNetDevMacVLanCreateWithVPortProfile: Rework to support multiple FDs qemu: Enable multiqueue for macvtaps
src/lxc/lxc_process.c | 3 +- src/qemu/qemu_command.c | 65 ++++++++++------ src/qemu/qemu_command.h | 2 + src/qemu/qemu_hotplug.c | 16 ++-- src/util/virnetdevmacvlan.c | 185 ++++++++++++++++++++++---------------------- src/util/virnetdevmacvlan.h | 7 +- 6 files changed, 153 insertions(+), 125 deletions(-)
For some reason 5/7 and 6/7 didn't get delivered to me (although they are in the list archive). I have a suggestion for slight rewording of the function description in 5/7, which I'll list here, but otherwise ACK for the series.
* Turn the IFF_VNET_HDR flag, if requested and available, make sure it's * off in the other cases. Similarly, IFF_MULTI_QUEUE is enabled if * requested. However, if requested and failed to set, it is considered a * fatal error (as opposed to @vnet_hdr).
Instead maybe: "Turn on the IFF_VNET_HDR flag if requested and available, but make sure it's off otherwise. Similarly, turn on IFF_MULTI_QUEUE if requested, but if it can't be set, consider it a fatal error (rather than ignoring as with @vnet_hdr)."
On 10.12.2015 17:52, Laine Stump wrote:
On 12/10/2015 02:38 AM, Michal Privoznik wrote:
Patches 1, 2, 3, 6, 7 have been ACKed in previous round. However, I did slightly change them to reflect Laine's review suggestions. Patch 4 has not been ACKed yet, patch 5 is new.
Michal Privoznik (7): virNetDevMacVLanCreateWithVPortProfile: Turn vnet_hdr into flag virNetDevMacVLanTapOpen: Slightly rework virNetDevMacVLanTapOpen: Rework to support multiple FDs virNetDevMacVLanTapSetup: Rework to support multiple FDs virNetDevMacVLanTapSetup: Allow enabling of IFF_MULTI_QUEUE virNetDevMacVLanCreateWithVPortProfile: Rework to support multiple FDs qemu: Enable multiqueue for macvtaps
src/lxc/lxc_process.c | 3 +- src/qemu/qemu_command.c | 65 ++++++++++------ src/qemu/qemu_command.h | 2 + src/qemu/qemu_hotplug.c | 16 ++-- src/util/virnetdevmacvlan.c | 185 ++++++++++++++++++++++---------------------- src/util/virnetdevmacvlan.h | 7 +- 6 files changed, 153 insertions(+), 125 deletions(-)
For some reason 5/7 and 6/7 didn't get delivered to me (although they are in the list archive).
Weird. Same here ....
I have a suggestion for slight rewording of the function description in 5/7, which I'll list here, but otherwise ACK for the series.
* Turn the IFF_VNET_HDR flag, if requested and available, make sure it's * off in the other cases. Similarly, IFF_MULTI_QUEUE is enabled if * requested. However, if requested and failed to set, it is considered a * fatal error (as opposed to @vnet_hdr).
Instead maybe:
"Turn on the IFF_VNET_HDR flag if requested and available, but make sure it's off otherwise. Similarly, turn on IFF_MULTI_QUEUE if requested, but if it can't be set, consider it a fatal error (rather than ignoring as with @vnet_hdr)."
Thanks, fixed and pushed. Michal
participants (3)
-
Ian Campbell -
Laine Stump -
Michal Privoznik