[libvirt] [PATCH v3 22/36] network: remove the virDomainNetBandwidthChangeAllowed callback
Laine Stump
laine at laine.org
Wed Apr 3 01:22:38 UTC 2019
On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> The current qemu driver code for changing bandwidth on a NIC first asks
> the network driver if the change is supported, then changes the
> bandwidth on the VIF, and then tells the network driver to update the
> bandwidth on the bridge.
>
> This is potentially racing if a parallel API call causes the network
> driver to allocate bandwidth on the bridge between the check and the
> update phases.
>
> Change the code to just try to apply the network bridge update
> immediately and rollback at the end if something failed.
>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
> src/conf/domain_conf.c | 15 --------
> src/conf/domain_conf.h | 10 -----
> src/libvirt_private.syms | 1 -
> src/network/bridge_driver.c | 75 ++++++++-----------------------------
> src/qemu/qemu_driver.c | 8 ++--
> 5 files changed, 20 insertions(+), 89 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ee4d586d77..1c0efc018c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -30436,7 +30436,6 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom,
> static virDomainNetAllocateActualDeviceImpl netAllocate;
> static virDomainNetNotifyActualDeviceImpl netNotify;
> static virDomainNetReleaseActualDeviceImpl netRelease;
> -static virDomainNetBandwidthChangeAllowedImpl netBandwidthChangeAllowed;
> static virDomainNetBandwidthUpdateImpl netBandwidthUpdate;
>
>
> @@ -30444,13 +30443,11 @@ void
> virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
> virDomainNetNotifyActualDeviceImpl notify,
> virDomainNetReleaseActualDeviceImpl release,
> - virDomainNetBandwidthChangeAllowedImpl bandwidthChangeAllowed,
> virDomainNetBandwidthUpdateImpl bandwidthUpdate)
> {
> netAllocate = allocate;
> netNotify = notify;
> netRelease = release;
> - netBandwidthChangeAllowed = bandwidthChangeAllowed;
> netBandwidthUpdate = bandwidthUpdate;
> }
>
> @@ -30558,18 +30555,6 @@ virDomainNetReleaseActualDevice(virConnectPtr conn,
> return ret;
> }
>
> -bool
> -virDomainNetBandwidthChangeAllowed(virDomainNetDefPtr iface,
> - virNetDevBandwidthPtr newBandwidth)
> -{
> - if (!netBandwidthChangeAllowed) {
> - virReportError(VIR_ERR_NO_SUPPORT, "%s",
> - _("Virtual networking driver is not available"));
> - return -1;
> - }
> -
> - return netBandwidthChangeAllowed(iface, newBandwidth);
> -}
>
> int
> virDomainNetBandwidthUpdate(virDomainNetDefPtr iface,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 546ee181b1..4c0b3da47d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3542,10 +3542,6 @@ typedef int
> virDomainDefPtr dom,
> virDomainNetDefPtr iface);
>
> -typedef bool
> -(*virDomainNetBandwidthChangeAllowedImpl)(virDomainNetDefPtr iface,
> - virNetDevBandwidthPtr newBandwidth);
> -
> typedef int
> (*virDomainNetBandwidthUpdateImpl)(virDomainNetDefPtr iface,
> virNetDevBandwidthPtr newBandwidth);
> @@ -3555,7 +3551,6 @@ void
> virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
> virDomainNetNotifyActualDeviceImpl notify,
> virDomainNetReleaseActualDeviceImpl release,
> - virDomainNetBandwidthChangeAllowedImpl bandwidthChangeAllowed,
> virDomainNetBandwidthUpdateImpl bandwidthUpdate);
>
> int
> @@ -3576,11 +3571,6 @@ virDomainNetReleaseActualDevice(virConnectPtr conn,
> virDomainNetDefPtr iface)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> -bool
> -virDomainNetBandwidthChangeAllowed(virDomainNetDefPtr iface,
> - virNetDevBandwidthPtr newBandwidth)
> - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> -
> int
> virDomainNetBandwidthUpdate(virDomainNetDefPtr iface,
> virNetDevBandwidthPtr newBandwidth)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b23d3c9891..43f7f02af3 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -455,7 +455,6 @@ virDomainMemorySourceTypeFromString;
> virDomainMemorySourceTypeToString;
> virDomainNetAllocateActualDevice;
> virDomainNetAppendIPAddress;
> -virDomainNetBandwidthChangeAllowed;
> virDomainNetBandwidthUpdate;
> virDomainNetDefActualFromNetworkPort;
> virDomainNetDefActualToNetworkPort;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 1080684043..534d144464 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -5324,63 +5324,6 @@ networkNetworkObjTaint(virNetworkObjPtr obj,
> }
>
>
> -static bool
> -networkBandwidthGenericChecks(virDomainNetDefPtr iface,
> - virNetDevBandwidthPtr newBandwidth)
> -{
> - virNetDevBandwidthPtr ifaceBand;
> - unsigned long long old_floor, new_floor;
> -
> - if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
> - iface->data.network.actual->data.bridge.brname == NULL) {
> - /* This is not an interface that's plugged into a network.
> - * We don't care. Thus from our POV bandwidth change is allowed. */
> - return false;
> - }
> -
> - ifaceBand = virDomainNetGetActualBandwidth(iface);
> - old_floor = new_floor = 0;
> -
> - if (ifaceBand && ifaceBand->in)
> - old_floor = ifaceBand->in->floor;
> - if (newBandwidth && newBandwidth->in)
> - new_floor = newBandwidth->in->floor;
> -
> - return new_floor != old_floor;
> -}
> -
> -
> -static bool
> -networkBandwidthChangeAllowed(virDomainNetDefPtr iface,
> - virNetDevBandwidthPtr newBandwidth)
> -{
> - virNetworkDriverStatePtr driver = networkGetDriver();
> - virNetworkObjPtr obj = NULL;
> - virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
> - bool ret = false;
> -
> - if (!networkBandwidthGenericChecks(iface, newBandwidth))
> - return true;
> -
> - obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
> - if (!obj) {
> - virReportError(VIR_ERR_NO_NETWORK,
> - _("no network with matching name '%s'"),
> - iface->data.network.name);
> - return false;
> - }
> -
> - if (networkCheckBandwidth(obj, newBandwidth, ifaceBand, &iface->mac, NULL) < 0)
> - goto cleanup;
> -
> - ret = true;
> -
> - cleanup:
> - virNetworkObjEndAPI(&obj);
> - return ret;
> -}
> -
> -
> static int
> networkBandwidthUpdate(virDomainNetDefPtr iface,
> virNetDevBandwidthPtr newBandwidth)
> @@ -5391,6 +5334,7 @@ networkBandwidthUpdate(virDomainNetDefPtr iface,
> unsigned long long tmp_floor_sum;
> virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
> unsigned long long new_rate = 0;
> + unsigned long long old_floor, new_floor;
> int plug_ret;
> int ret = -1;
>
> @@ -5400,7 +5344,21 @@ networkBandwidthUpdate(virDomainNetDefPtr iface,
> return -1;
> }
>
> - if (!networkBandwidthGenericChecks(iface, newBandwidth))
> + if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + iface->data.network.actual->data.bridge.brname == NULL) {
> + /* This is not an interface that's plugged into a network.
> + * We don't care. Thus from our POV bandwidth change is allowed. */
This comment is more confusing than it is helpful. Especially since it's
wrong :-)
> + return 0;
> + }
> +
> + old_floor = new_floor = 0;
> +
> + if (ifaceBand && ifaceBand->in)
> + old_floor = ifaceBand->in->floor;
> + if (newBandwidth && newBandwidth->in)
> + new_floor = newBandwidth->in->floor;
> +
> + if (new_floor == old_floor)
> return 0;
>
> obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
> @@ -5546,7 +5504,6 @@ networkRegister(void)
> networkAllocateActualDevice,
> networkNotifyActualDevice,
> networkReleaseActualDevice,
> - networkBandwidthChangeAllowed,
> networkBandwidthUpdate);
>
> return 0;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b81c411007..baf188ae40 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11729,17 +11729,17 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
> }
>
> if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> - !virDomainNetBandwidthChangeAllowed(net, newBandwidth))
> + virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
> goto endjob;
>
> if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
> - !virDomainNetTypeSharesHostView(net)) < 0 ||
> - (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> - virDomainNetBandwidthUpdate(net, newBandwidth) < 0)) {
> + !virDomainNetTypeSharesHostView(net)) < 0) {
> ignore_value(virNetDevBandwidthSet(net->ifname,
> net->bandwidth,
> false,
> !virDomainNetTypeSharesHostView(net)));
> + ignore_value(virDomainNetBandwidthUpdate(net,
> + net->bandwidth));
> goto endjob;
> }
>
The new version of the code is calling virDomainNetBandwidthUpdate() and
virNetDevBandwidthSet() in reverse order from what it previously did. If
either of the functions were to modify newBandwidth's contents, then the
results would change. Fortunately it looks like neither of them do that,
so I think we're safe.
Reviewed-by: Laine Stump <laine at laine.org>
More information about the libvir-list
mailing list