Re: [libvirt] [PATCH RFC] network: Bring netdevs online later
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
>> ---
>> 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 .
>
>
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);
>
Re: [libvirt] [PATCH RFC] network: Bring netdevs online later
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
> ---
> 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 .
> +{
> +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);
>
[libvirt] [PATCH RFC] network: Bring netdevs online later
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?
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
---
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));
+ignore_value(virNetDevMacVLanDelete(net->ifname));
+}
+return;
+}
+
+void
+qemuNetworkInitializeDevices(virDomainDefPtr def)
+{
+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;
-}
-
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;
-
return 0;
error:
--
1.7.9.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
