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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]