Re: [libvirt] [PATCH RFC] network: Bring netdevs online later

On 06/04/2014 05:55 PM, 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. > > This approach is an alternative to my previously proposed > 'network: Defer online of macvtap during qemu migration' > Laine/Wangrui, is this the sort of thing you had in mind? Yes and no. It is basically what I was thinking - *always* wait to bring up the devices right before turning on the CPU of the guest. However, it doesn't account for hotplug - In that case, the CPUs have already been started, and the single device that has just been hotplugged needs to be started. This patch doesn't do anything about that. And there are a few other problems I saw from reading through it as well (this is without compiling/testing it):

> > Previous thread: > https://www.redhat.com/archives/libvir-list/2014-May/msg00427.html > Associated BZ: > https://bugzilla.redhat.com/show_bug.cgi?id=1081461 > > Signed-off-by: Matthew Rosato <mjrosato(a)linux.vnet.ibm.com&gt; > --- > src/qemu/qemu_command.c | 45 +++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_command.h | 3 +++ > src/qemu/qemu_process.c | 3 +++ > src/util/virnetdevmacvlan.c | 5 ----- > src/util/virnetdevtap.c | 3 --- > 5 files changed, 51 insertions(+), 8 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index e6acced..c161d73 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -571,6 +571,51 @@ qemuNetworkPrepareDevices(virDomainDefPtr def) > return ret; > } > > +void > +qemuNetworkIfaceUp(virDomainNetDefPtr net) > +{ > + if (virNetDevSetOnline(net->ifname, true) < 0) { > + ignore_value(virNetDevTapDelete(net->ifname)); > + } > + return; > +} > + > +void > +qemuPhysIfaceUp(virDomainNetDefPtr net) > +{ > + if (virNetDevSetOnline(net->ifname, true) < 0) { > + ignore_value(virNetDevVPortProfileDisassociate(net->ifname, > + virDomainNetGetActualVirtPortProfile(net), > + &net->mac, > + virDomainNetGetActualDirectDev(net), > + -1, > + VIR_NETDEV_VPORT_PROFILE_OP_MIGRATE_IN_FINISH)); Is this always the proper vmop (MIGRATE_IN_FINISH) no matter what the situation is? (remember it could now be happening not as the result of a failure during migration, but also as the result of a failure during the initial start of a domain, or during a hotplug).

(I *really* dislike the way that this "vmop" (which is only used by low level macvtap functions) must be passed around through multiple layers of the callstack in higher level functions (existing problem), and wish/hope that there is a way to make it more localized, perhaps by storing the current state in the NetworkDef object as status somehow. But that's a separate issue, so for now we have to just continue passing it around.) > + ignore_value(virNetDevMacVLanDelete(net->ifname)); > + } > + return; > +} I think it would be better to have a single function that takes a virDomainNetPtr and contains the switch statement. This way 1) a call to it can easily be added in the proper place to support hotplug, and 2) that one function can later be moved to the same final location as what is currently called networkAllocateActualDevice() and those two functions can become part of an API that will allow non-privileged libvirt instances to use these network types.

> + > +void > +qemuNetworkInitializeDevices(virDomainDefPtr def) I think the verb "Start" would be better than "Initialize" in this case - that one is too easily confused with the already existing "Prepare".

Also, I think we should create a qemu_interface.c to contain all of these functions, similar to how we currently have a qemu_hostdev.c for functions related to <hostdev>.

> +{ > + size_t i; > + > + for (i = 0; i < def->nnets; i++) { > + virDomainNetDefPtr net = def->nets[i]; > + switch(virDomainNetGetActualType(net)) { > + case VIR_DOMAIN_NET_TYPE_BRIDGE: > + case VIR_DOMAIN_NET_TYPE_NETWORK: > + qemuNetworkIfaceUp(net); > + break; > + case VIR_DOMAIN_NET_TYPE_DIRECT: > + qemuPhysIfaceUp(net); > + break; > + } > + } > + > + return; > +} > + > static int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info, > const char *prefix) > { > diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h > index afbd6ff..4a44464 100644 > --- a/src/qemu/qemu_command.h > +++ b/src/qemu/qemu_command.h > @@ -206,6 +206,9 @@ int qemuOpenVhostNet(virDomainDefPtr def, > int *vhostfdSize); > > int qemuNetworkPrepareDevices(virDomainDefPtr def); > +void qemuNetworkIfaceUp(virDomainNetDefPtr net); > +void qemuPhysIfaceUp(virDomainNetDefPtr net); > +void qemuNetworkInitializeDevices(virDomainDefPtr def); > > /* > * NB: def->name can be NULL upon return and the caller > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index d719716..bbc11f3 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -2765,6 +2765,9 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, > qemuDomainObjPrivatePtr priv = vm->privateData; > virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); > > + /* Bring up netdevs before starting CPUs */ > + qemuNetworkInitializeDevices(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 cb85b74..3748527 100644 > --- a/src/util/virnetdevmacvlan.c > +++ b/src/util/virnetdevmacvlan.c > @@ -898,11 +898,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, > goto link_del_exit; > } > > - if (virNetDevSetOnline(cr_ifname, true) < 0) { > - rc = -1; > - goto disassociate_exit; > - } > - Here you've changed the semantics of virNetDevMacVLanCreateWithVPortProfile() without accounting for all the places that it is used. In particular, the LXC driver also calls this function, and assumes that the device will be online when the function returns, but once your patch is applied, that is no longer the case.

> if (withTap) { > if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10)) < 0) > goto disassociate_exit; > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c > index 0b444fa..09b9c12 100644 > --- a/src/util/virnetdevtap.c > +++ b/src/util/virnetdevtap.c > @@ -574,9 +574,6 @@ int virNetDevTapCreateInBridgePort(const char *brname, > goto error; > } > > - if (virNetDevSetOnline(*ifname, !!(flags & VIR_NETDEV_TAP_CREATE_IFUP)) < 0) > - goto error; > - And again you've changed the semantics, but in this case it's when the function is called with the flag "VIR_NETDEV_TAP_CREATE_IFUP" - it would be much safer to leave this code intact, and just remove that flag from the caller in the appropriate place.

> return 0; > > error: So the summary is: 1) we should create qemu_interface.c, and move/rename the qemuNetwork* functions to that file. 2) create qemuInterfaceStartDevice() and qemuInterfaceStartDevices() and call them from the appropriate places: * qemuInterfaceStartDevices() should be called from exactly where you are calling qemuNetworkInitializeDevices now. * hotplug needs to be changed to immediately call qemuInterfaceStartDevice() right after it calls networkAllocateActualDevice(). 3) the existing virNetDev* functions should not be changed, to be sure that we don't mess up LXC (or any other potential users) 4) we need to verify that we are providing the correct "vmop" to the disassociate function in the case that virNetDevSetOnlinevirNetDevSetOnline() fails for a macvtap device.