[libvirt] [PATCH RFC] network: Bring netdevs online later
Laine Stump
laine at laine.org
Mon Jun 9 12:55:03 UTC 2014
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 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).
(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.
More information about the libvir-list
mailing list