On 08/03/2015 02:39 AM, Michal Privoznik wrote:
> When a domain vNIC's bandwidth is to be changed (at runtime) it is
> possible that guaranteed minimal bandwidth (@floor) will change too.
> Well, so far it is, because we still don't have an implementation that
> allows setting it dynamically, so it's effectively erased on:
>
> #virsh domiftune $dom vnet0 --inbound 0
>
> However, that's slightly unfortunate. We do some checks on domain
> startup to see if @floor can be guaranteed. We ought do the same if
> QoS is changed at runtime.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/network/bridge_driver.c | 72 +++++++++++++++++++++++++++++++++++++++++++--
> src/network/bridge_driver.h | 12 ++++++++
> 2 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 17fc430..fa60ba4 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4686,9 +4686,18 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
> * networkCheckBandwidth:
> * @net: network QoS
> * @ifaceBand: interface QoS (may be NULL if no QoS)
> + * @oldBandwidth: new interface QoS (may be NULL if no QoS)
> * @ifaceMac: interface MAC (used in error messages for identification)
> * @new_rate: new rate for non guaranteed class
> *
> + * Function checks if @ifaceBand can be satisfied on @net. However, sometimes it
> + * may happen that the interface that @ifaceBand corresponds to is already
> + * plugged into the @net and the bandwidth is to be updated. In that case we
> + * need to check if new bandwidth can be satisfied. If that's the case
> + * @ifaceBand should point to new bandwidth settings and @oldBandwidth to
> + * current ones. If you want to suppress this functionality just pass
> + * @oldBandwidth == NULL.
> + *
> * Returns: -1 if plugging would overcommit network QoS
> * 0 if plugging is safe (@new_rate updated)
> * 1 if no QoS is set (@new_rate untouched)
new_rate is unchanged if passed as NULL too
Part of me says - who cares - just set it and let the caller decide -
both paths will "always have" that extra "if" check now... and it only
ever matters if plugging is safe.
I'm OK keeping as is - so I'll ACK and let you decide. At the very least
point out that new_rate can be NULL in the comments. I'm guessing that
if the caller that has it NULL had a value, then there'd be a warning
about an unused variable...
John
> @@ -4696,6 +4705,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
> static int
> networkCheckBandwidth(virNetworkObjPtr net,
> virNetDevBandwidthPtr ifaceBand,
> + virNetDevBandwidthPtr oldBandwidth,
> virMacAddr ifaceMac,
> unsigned long long *new_rate)
> {
> @@ -4723,6 +4733,8 @@ networkCheckBandwidth(virNetworkObjPtr net,
> }
>
> tmp_new_rate = netBand->in->average;
> + if (oldBandwidth && oldBandwidth->in)
> + tmp_floor_sum -= oldBandwidth->in->floor;
> tmp_floor_sum += ifaceBand->in->floor;
>
> /* check against peak */
> @@ -4749,7 +4761,8 @@ networkCheckBandwidth(virNetworkObjPtr net,
> goto cleanup;
> }
>
> - *new_rate = tmp_new_rate;
> + if (new_rate)
> + *new_rate = tmp_new_rate;
> ret = 0;
>
> cleanup:
> @@ -4791,7 +4804,7 @@ networkPlugBandwidth(virNetworkObjPtr net,
> char ifmac[VIR_MAC_STRING_BUFLEN];
> virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
>
> - if ((plug_ret = networkCheckBandwidth(net, ifaceBand,
> + if ((plug_ret = networkCheckBandwidth(net, ifaceBand, NULL,
> iface->mac, &new_rate)) < 0) {
> /* helper reported error */
> goto cleanup;
> @@ -4917,3 +4930,58 @@ networkNetworkObjTaint(virNetworkObjPtr net,
> virNetworkTaintTypeToString(taint));
> }
> }
> +
> +
> +static bool
> +networkBandwidthGenericChecks(virDomainNetDefPtr iface,
> + virNetDevBandwidthPtr newBandwidth)
> +{
> + virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
> + unsigned long long old_floor, new_floor;
> +
> + if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK) {
> + /* 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;
> + }
> +
> + 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;
> +}
> +
> +
> +bool
> +networkBandwidthChangeAllowed(virDomainNetDefPtr iface,
> + virNetDevBandwidthPtr newBandwidth)
> +{
> + virNetworkDriverStatePtr driver = networkGetDriver();
> + virNetworkObjPtr network = NULL;
> + virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
> + bool ret = false;
> +
> + if (!networkBandwidthGenericChecks(iface, newBandwidth))
> + return true;
> +
> + network = virNetworkObjFindByName(driver->networks, iface->data.network.name);
> + if (!network) {
> + virReportError(VIR_ERR_NO_NETWORK,
> + _("no network with matching name '%s'"),
> + iface->data.network.name);
> + return false;
> + }
> +
> + if (networkCheckBandwidth(network, newBandwidth, ifaceBand, iface->mac, NULL) < 0)
> + goto cleanup;
> +
> + ret = true;
> +
> + cleanup:
> + virNetworkObjEndAPI(&network);
> + return ret;
> +}
> diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
> index 513ccf7..cce9237 100644
> --- a/src/network/bridge_driver.h
> +++ b/src/network/bridge_driver.h
> @@ -52,6 +52,11 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,
> char **configstr,
> dnsmasqContext *dctx,
> dnsmasqCapsPtr caps);
> +
> +bool networkBandwidthChangeAllowed(virDomainNetDefPtr iface,
> + virNetDevBandwidthPtr newBandwidth)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
> # else
> /* Define no-op replacements that don't drag in any link dependencies. */
> # define networkAllocateActualDevice(dom, iface) 0
> @@ -73,6 +78,13 @@ networkReleaseActualDevice(virDomainDefPtr dom ATTRIBUTE_UNUSED,
> return 0;
> }
>
> +static inline bool
> +networkBandwidthChangeAllowed(virDomainNetDefPtr iface ATTRIBUTE_UNUSED,
> + virNetDevBandwidthPtr newBandwidth ATTRIBUTE_UNUSED)
> +{
> + return true;
> +}
> +
> # endif
>
> typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list