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(a)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