[libvirt] [PATCH RFC] network: Bring netdevs online later
Matthew Rosato
mjrosato at linux.vnet.ibm.com
Tue Jun 10 13:55:29 UTC 2014
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 at linux.vnet.ibm.com>
>> ---
>> 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
More information about the libvir-list
mailing list