On 08/28/2014 09:56 AM, Martin Kletzander wrote:
> On Wed, Aug 27, 2014 at 10:34:14AM -0400, Matthew Rosato wrote:
>> Currently, MAC registration occurs during device creation, which is
>> early enough that, during live migration, you end up with duplicate
>> MAC addresses on still-running source and target devices, even though
>> the target device isn't actually being used yet.
>> This patch proposes to defer MAC registration until right before
>> the guest can actually use the device -- In other words, right
>> before starting guest CPUs.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxxxxxxx>
>> ---
>> src/Makefile.am | 3 +-
>> src/conf/domain_conf.h | 2 ++
>> src/lxc/lxc_process.c | 4 ++-
>> src/qemu/qemu_command.c | 6 +++-
>> src/qemu/qemu_hotplug.c | 7 ++++
>> src/qemu/qemu_interface.c | 78
>> +++++++++++++++++++++++++++++++++++++++++++
>> src/qemu/qemu_interface.h | 32 ++++++++++++++++++
>> src/qemu/qemu_process.c | 4 +++
>> src/util/virnetdevmacvlan.c | 8 +++--
>> src/util/virnetdevmacvlan.h | 2 ++
>> 10 files changed, 140 insertions(+), 6 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 46e411e..842573f 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES = \
>> qemu/qemu_monitor_text.h \
>> qemu/qemu_monitor_json.c \
>> qemu/qemu_monitor_json.h \
>> - qemu/qemu_driver.c qemu/qemu_driver.h
>> + qemu/qemu_driver.c qemu/qemu_driver.h \
>
> I don't know if this got mixed by the mail client or not, but I'm
> adjusting it (the backslash) to match the others.
>
>> + qemu/qemu_interface.c qemu/qemu_interface.h
>>
>
> I still don't see anything qemu-specific inthose files, but for now
> separate files are fine, we can move the code around any time later if
> it's needed.
I mentioned this in the cover letter comments, but you may have missed it:
" * I left the contents of qemu_interface as-is, rather than collapsing
them into non-qemu-specific functions, in order to keep Makefile linkage
consistent & happy (needs to be part of QEMU_DRIVER_SOURCES). Instead,
incorporated copyright suggestions from previous comments. "
Basically, you're right in that there is nothing inherently
qemu-specific in this code, just that it's currently only being written
with qemu in mind, and it sticks with the current Makefile convention
(everything in QEMU_DRIVER_SOURCES is part of src/qemu/qemu_*).
>
>> XENAPI_DRIVER_SOURCES = \
>> xenapi/xenapi_driver.c xenapi/xenapi_driver.h \
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index aead903..6268690 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -950,6 +950,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 ed30c37..b2256c0 100644
>> --- a/src/lxc/lxc_process.c
>> +++ b/src/lxc/lxc_process.c
>> @@ -300,6 +300,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
>> @@ -336,7 +337,8 @@ char
>> *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
>> &res_ifname,
>> VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
>> cfg->stateDir,
>> - virDomainNetGetActualBandwidth(net), 0) < 0)
>> + virDomainNetGetActualBandwidth(net),
>> + macvlan_create_flags) < 0)
>> goto cleanup;
>>
>> ret = res_ifname;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 1f71fa3..43a8b63 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
>> net->ifname = res_ifname;
>> }
>>
>> + /* Save vport profile op for later */
>> + net->vmOp = vmop;
>> +
>> virObjectUnref(cfg);
>> return rc;
>> }
>> @@ -286,7 +289,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);
>> @@ -346,6 +349,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>> goto cleanup;
>> }
>> } else {
>> + tap_create_flags |= VIR_NETDEV_TAP_CREATE_IFUP;
>
> But I don't quite understand why this is different for system and
> session daemon. Maybe it's my head just giving up on me.
I thought I had a good reason for doing this -- But I'm not finding it.
So, yeah, I think this should be done above so that it affects both paths.
>
>> 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 a364c52..633eda5 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -30,6 +30,7 @@
>> #include "qemu_domain.h"
>> #include "qemu_command.h"
>> #include "qemu_hostdev.h"
>> +#include "qemu_interface.h"
>> #include "domain_audit.h"
>> #include "domain_nwfilter.h"
>> #include "virlog.h"
>> @@ -878,6 +879,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) {
>> @@ -2069,6 +2073,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
>> goto cleanup;
>> }
>>
>> + /* Set device online immediately */
>> + qemuInterfaceStartDevice(newdev);
>> +
>> newType = virDomainNetGetActualType(newdev);
>>
>> if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
>
> But these two hunks don't make sense to me. Actually they would make
> sense under the (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) condition.
> Is there any reason why you are making these calls unconditionally?
>
Based on my comment above, I think we also need to be calling for
VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE then.
I guess my rationale was that we would always call the function, and the
switch in qemuInterfaceStartDevice() would decide whether or not work
needed to be done, minimizing the amount of effort needed to add new
devices later.
If you'd rather I only call qemuinterfaceStartDevice for the appropriate
values of newType, I can certainly do that.
>> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
>> new file mode 100644
>> index 0000000..dccfcc4
>> --- /dev/null
>> +++ b/src/qemu/qemu_interface.c
>> @@ -0,0 +1,78 @@
>> +/*
>> + * qemu_interface.c: QEMU interface management
>> + *
>> + * Copyright IBM Corp. 2014
>> + *
>> + * 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/>.
>> + *
>> + * Authors:
>> + * Matthew J. Rosato <mjrosato@xxxxxxxxxxxxxxxxxx>
>> + */
>> +
>> +#include <config.h>
>> +
>> +#include "qemu_interface.h"
>> +#include "virnetdev.h"
>> +#include "virnetdevtap.h"
>> +#include "virnetdevmacvlan.h"
>> +#include "virnetdevvportprofile.h"
>> +
>> +/**
>> + * qemuInterfaceStartDevice:
>> + * @net: net device to start
>> + *
>> + * Based upon the type of device provided, perform the appropriate
>> + * work to set the device online.
>> + */
>> +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;
>> + }
>> +}
>> +
>> +/**
>> + * qemuInterfaceStartDevices:
>> + * @def: domain definition
>> + *
>> + * Set all ifaces associated with this domain to the online state.
>> + */
>> +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..5810cda
>> --- /dev/null
>> +++ b/src/qemu/qemu_interface.h
>> @@ -0,0 +1,32 @@
>> +/*
>> + * qemu_interface.h: QEMU interface management
>> + *
>> + * Copyright IBM Corp. 2014
>> + *
>> + * 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/>.
>> + *
>> + * Authors:
>> + * Matthew J. Rosato <mjrosato@xxxxxxxxxxxxxxxxxx>
>> + */
>> +
>> +#ifndef __QEMU_INTERFACE_H__
>> +# define __QEMU_INTERFACE_H__
>> +
>> +# include "domain_conf.h"
>> +
>> +void qemuInterfaceStartDevice(virDomainNetDefPtr net);
>> +void qemuInterfaceStartDevices(virDomainDefPtr def);
>> +
>> +#endif /* __QEMU_INTERFACE_H__ */
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index f68dfbe..edb2b1f 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"
>> @@ -2854,6 +2855,9 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>
>> + /* Bring up netdevs before starting CPUs */
>> + qemuInterfaceStartDevices(vm->def);
>> +
>
> This seems to be in a wrong place, I guess. qemuProcessStartCPUs() is
> called from *many* places I'm not sure you need it (resume after dump,
> etc.).
>
Hmm, that's a fair point. I might be able to key off of the
'virDomainRunningReason reason' to decide whether or not the call is
necessary. Alternatively, I guess I could just call
qemuInterfaceStartDevices() prior to each appropriate
qemuProcessStartCPUs call.
> Other than these few things the patch looks fine, but I'd rather have
> another opinion in this code area.
Fair enough. Thanks for the comments.
Matt
>
>> 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 9ddeca4..4099090 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 e92da4a..43dc71f 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list