On Tue, Jul 01, 2014 at 02:00:56PM -0400, Matthew Rosato wrote:

Currently, there is one flag passed in during macvtap creation (withTap) -- Let's convert this field to an unsigned int flag field for future expansion.

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> --- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_command.c | 3 ++- src/util/virnetdevmacvlan.c | 24 +++++++++++++++--------- src/util/virnetdevmacvlan.h | 8 +++++++- 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index cb85b74..bfbecbf 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -790,6 +790,7 @@ 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'. + * @flags: OR of virNetDevMacVLanCreateFlags. * @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 @@ -797,14 +798,15 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * interface will be stored into if everything succeeded. It is up * to the caller to free the string. * - * Returns file descriptor of the tap device in case of success with @withTap, - * otherwise returns 0; returns -1 on error. + * Returns file descriptor of the tap device in case of success with + * flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; returns

s/flags/@flags/

+ * -1 on error. */ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, const virMacAddr *macaddress, const char *linkdev, virNetDevMacVLanMode mode, - bool withTap, + unsigned int flags, int vnet_hdr, const unsigned char *vmuuid, virNetDevVPortProfilePtr virtPortProfile,

Having @flags as some middle parameter feels itchy, could you move it to the end? [...]

@@ -813,9 +815,12 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, char *stateDir, virNetDevBandwidthPtr bandwidth) { - const char *type = withTap ? "macvtap" : "macvlan"; - const char *prefix = withTap ? MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; - const char *pattern = withTap ? MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; + const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + "macvtap" : "macvlan"; + const char *prefix = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; + const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN;

Since these are now multiline anyway, it would be nice to have them set in an if for more readability. Martin

On 07/23/2014 07:49 AM, Martin Kletzander wrote:

On Tue, Jul 01, 2014 at 02:00:57PM -0400, Matthew Rosato wrote:

...

Defer MAC registration until net devices are actually going to be used by the guest. This patch does so by setting the devices online just before starting guest CPUs.

Does this have some upside/downside? Are you trying to fix some problem? It would be nice to describe it in the commit message, so I know what to focus on or why it's needed. Depending on the answer there might be a way how to unit-test it.

...

+++ b/src/qemu/qemu_interface.c @@ -0,0 +1,65 @@ +/* + * qemu_interface.c: QEMU interface management + * + * Copyright (C) 2014 Red Hat, Inc. + * Copyright IBM Corp. 2014 + *

I don't understand this double copyright here, copy-paste mistake?

If this file is copied from a pre-existing file with double copyright, and substantially borrows from that content rather than being fresh material, then keeping double copyright is ideal. If this is something you mostly wrote yourself, and then just copied in boilerplate, then it's simpler to just put one copyright line for yourself (in this case, IBM); then later, if other contributors make contributions, a second line can be added then (I tend to add Red Hat copyright whenever I touch files, since I'm doing my work on Red Hat time). I don't see it as a show-stopper either way, because these days, git log is more reliable for telling who contributed what portions of a file. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 07/23/2014 09:49 AM, Martin Kletzander wrote:

On Tue, Jul 01, 2014 at 02:00:57PM -0400, Matthew Rosato wrote:

...

Defer MAC registration until net devices are actually going to be used by the guest. This patch does so by setting the devices online just before starting guest CPUs.

Does this have some upside/downside? Are you trying to fix some problem? It would be nice to describe it in the commit message, so I know what to focus on or why it's needed. Depending on the answer there might be a way how to unit-test it.

(sorry for the late reply, just returning from vacation) Well, it's attempting to fix the problem linked in the cover letter: https://bugzilla.redhat.com/show_bug.cgi?id=1081461 as well as a problem Wangrui (on CC) reported separately: https://www.redhat.com/archives/libvir-list/2014-March/msg01054.html In short, MAC registration currently occurs during target device creation, early enough that you end up with a duplicate MAC address on the still-running source and the target device -- But the target isn't really even using the device yet (not running yet). The longer the live migration takes, the wider the window where this disparity exists, and could cause packets intended for the still-running source to get routed to the not-yet-running target. The patch proposes to shrink this window by not "upping" those netdevs until right before CPUs are started (ie, immediately before the guest will actually start using them). I will try to make the problem statement a little clearer in the commit message for the next version.

...

Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> --- src/Makefile.am | 1 + src/conf/domain_conf.h | 2 ++ src/lxc/lxc_process.c | 3 +- src/qemu/qemu_command.c | 6 +++- src/qemu/qemu_hotplug.c | 7 +++++ src/qemu/qemu_interface.c | 65 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_interface.h | 33 ++++++++++++++++++++++ src/qemu/qemu_process.c | 4 +++ src/util/virnetdevmacvlan.c | 8 ++++-- src/util/virnetdevmacvlan.h | 2 ++ 10 files changed, 126 insertions(+), 5 deletions(-) create mode 100644 src/qemu/qemu_interface.c create mode 100644 src/qemu/qemu_interface.h

diff --git a/src/Makefile.am b/src/Makefile.am index 35720be..e3d2e36 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -687,6 +687,7 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_domain.c qemu/qemu_domain.h \ qemu/qemu_cgroup.c qemu/qemu_cgroup.h \ qemu/qemu_hostdev.c qemu/qemu_hostdev.h \ + qemu/qemu_interface.c qemu/qemu_interface.h \ qemu/qemu_hotplug.c qemu/qemu_hotplug.h \ qemu/qemu_hotplugpriv.h \ qemu/qemu_conf.c qemu/qemu_conf.h \ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1122eb2..8375106 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -921,6 +921,8 @@ struct _virDomainNetDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int linkstate; + /* vmOp value saved if deferring interface start */ + virNetDevVPortProfileOp vmOp; };

/* Used for prefix of ifname of any network name generated dynamically diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index ab0c5d0..3083ed3 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -302,6 +302,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, virNetDevBandwidthPtr bw; virNetDevVPortProfilePtr prof; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); + unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;

/* XXX how todo bandwidth controls ? * Since the 'net-ifname' is about to be moved to a different @@ -333,7 +334,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), virDomainNetGetActualDirectMode(net), - 0, false, def->uuid, + macvlan_create_flags, false, def->uuid, virDomainNetGetActualVirtPortProfile(net), &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 314d8a3..a5662f5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -196,6 +196,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname = res_ifname; }

+ /* Save vport profile op for later */ + net->vmOp = vmop; + virObjectUnref(cfg); return rc;

@@ -294,7 +297,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, { char *brname = NULL; int ret = -1; - unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; + unsigned int tap_create_flags = 0; bool template_ifname = false; int actualType = virDomainNetGetActualType(net); virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -354,6 +357,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, goto cleanup; } } else { + tap_create_flags |= VIR_NETDEV_TAP_CREATE_IFUP; if (qemuCreateInBridgePortWithHelper(cfg, brname, &net->ifname, tapfd, tap_create_flags) < 0) { diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5e8aa4e..98e7764 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -49,6 +49,7 @@ #include "virstoragefile.h" #include "virstring.h" #include "virtime.h" +#include "qemu_interface.h"

#define VIR_FROM_THIS VIR_FROM_QEMU

@@ -854,6 +855,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (networkAllocateActualDevice(vm->def, net) < 0) goto cleanup;

+ /* Set device online immediately */ + qemuInterfaceStartDevice(net); + actualType = virDomainNetGetActualType(net);

if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { @@ -2030,6 +2034,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }

+ /* Set device online immediately */ + qemuInterfaceStartDevice(newdev); + newType = virDomainNetGetActualType(newdev);

if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c new file mode 100644 index 0000000..b70b6eb --- /dev/null +++ b/src/qemu/qemu_interface.c @@ -0,0 +1,65 @@ +/* + * qemu_interface.c: QEMU interface management + * + * Copyright (C) 2014 Red Hat, Inc. + * Copyright IBM Corp. 2014 + *

I don't understand this double copyright here, copy-paste mistake?

Well, I used a different file as a boilerplate that was copyright Red Hat, so it seemed like the right thing to do. But based on your comment and Eric's follow-up, I'll strike the Red Hat line.

...

+ * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#include <config.h> + +#include "qemu_interface.h" +#include "virnetdev.h" +#include "virnetdevtap.h" +#include "virnetdevmacvlan.h" +#include "virnetdevvportprofile.h" + +void +qemuInterfaceStartDevice(virDomainNetDefPtr net) +{ + switch (virDomainNetGetActualType(net)) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevTapDelete(net->ifname)); + } + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevVPortProfileDisassociate(net->ifname, + virDomainNetGetActualVirtPortProfile(net), + &net->mac, + virDomainNetGetActualDirectDev(net), + -1, + net->vmOp)); + } + break; + } +} + +void +qemuInterfaceStartDevices(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->nnets; i++) { + qemuInterfaceStartDevice(def->nets[i]); + } + + return; +} diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h new file mode 100644 index 0000000..dd14556 --- /dev/null +++ b/src/qemu/qemu_interface.h @@ -0,0 +1,33 @@ +/* + * qemu_interface.h: QEMU interface management + * + * Copyright (C) 2014 Red Hat, Inc. + * Copyright IBM Corp. 2014 + *

The same as in 'qemu_interface.c'.

See above.

...

+ * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#ifndef __QEMU_INTERFACE_H__ +#define __QEMU_INTERFACE_H__

Improper indentation.

Thanks.

...

+ +//# include "qemu_conf.h"

Possible leftover?

Oops. Yep. Thanks.

...

+# include "domain_conf.h" + +void qemuInterfaceStartDevice(virDomainNetDefPtr net); +void qemuInterfaceStartDevices(virDomainDefPtr def); + +#endif /* __QEMU_INTERFACE_H__ */

Why are these qemu_ when they have nothing to do with qemu in particular?

Well, they're only being proposed for used by qemu currently. But, I guess there's no reason they couldn't be used by a different hypervisor. This name was based on Laine's suggestion, did you have something else in mind? Something in network/ or util/? Actually, maybe they can just find a home in util/virnetdev.c as, say, virNetDevStartDevice and virNetDevStartDevices, and we can strike these new files. Laine, do you have any input on this one?

...

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5b598be..26bd494 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -42,6 +42,7 @@ #include "qemu_hostdev.h" #include "qemu_hotplug.h" #include "qemu_migration.h" +#include "qemu_interface.h"

#include "cpu/cpu.h" #include "datatypes.h" @@ -2775,6 +2776,9 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);

+ /* Bring up netdevs before starting CPUs */ + qemuInterfaceStartDevices(vm->def); + VIR_DEBUG("Using lock state '%s'", NULLSTR(priv->lockState)); if (virDomainLockProcessResume(driver->lockManager, cfg->uri, vm, priv->lockState) < 0) { diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index bfbecbf..efb31aa 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -903,9 +903,11 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto link_del_exit; }

- if (virNetDevSetOnline(cr_ifname, true) < 0) { - rc = -1; - goto disassociate_exit; + if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP) { + if (virNetDevSetOnline(cr_ifname, true) < 0) { + rc = -1; + goto disassociate_exit; + } }

if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index f7895b5..d43446f7 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -44,6 +44,8 @@ typedef enum { VIR_NETDEV_MACVLAN_CREATE_NONE = 0, /* Create with a tap device */ VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, + /* Bring the interface up */ + VIR_NETDEV_MACVLAN_CREATE_IFUP = 1 << 1, } virNetDevMacVLanCreateFlags;

int virNetDevMacVLanCreate(const char *ifname, -- 1.7.9.5

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list