On 06/09/2014 08:55 AM, Laine Stump wrote: > 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): > Thanks very much for your detailed comments, this is exactly what I was looking for and why I marked as RFC -- Wanted to make sure I was even in the right ballpark with this. >> >> 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@xxxxxxxxxxxxxxxxxx> >> --- >> 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). > D'oh. Good catch, I forgot this was even being passed in (as you probably guessed, it was copied directly from my migration-specific patch). > (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. > OK, sure. >> + >> +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". > Yeah, I went back-and-forth on the naming, "Start" sounds fine. > 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>. > > OK. >> +{ >> + 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. > Good point. For this, I suppose I could add another bool operand that specifies whether or not to bring the device up (like bool withTap). Or, since we already have withTap, I could add another patch that introduces a 'flags' field (same idea as virNetDevTapCreateFlags), add a new flag like VIR_NETDEV_MACVLAN_CREATE_IFUP, and also pull in the withTap flag as VIR_NETDEV_MACVLAN_CREATE_WITH_TAP. >> 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. > > Agreed. >> 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. > I'll roll these comments into a v2. Thanks, Matt -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list