[libvirt] [PATCH 2/2] network: Bring netdevs online later
Matthew Rosato
mjrosato at linux.vnet.ibm.com
Tue Jul 29 15:43:29 UTC 2014
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 at 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 at 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 at 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list