[libvirt] [PATCH 1/4] networkAllocateActualDevice: Set QoS for bridgeless networks too

[Eric Blake]

On 01/23/2014 06:44 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1055484
> 
> Currently, libvirt's XML schema of network allows QoS to be defined for
> every network even though it has no bridge. For instance:
> 
> <network>
>     <name>vdsm-no-bridge</name>
>     <forward mode='passthrough'>
>       <interface dev='em1.10'/>
>     </forward>
>     <bandwidth>
>         <inbound average='1000' peak='5000' burst='1024'/>
>         <outbound average='1000' burst='1024'/>
>     </bandwidth>
> </network>
> 
> The bandwidth limitations can be, however, applied even on such
> networks. In fact, they are gonna be applied on the interface that will

s/gonna/going to/

> be connected to the network on a domain startup. This approach, however,
> has one limitation. With bridged networks, there are two points where
> QoS can be set: bridge and domain interface. The lower limit of the two
> is enforced then. For instance, if the interface has 10Mbps average, but
> the network only 1Mbps, there's no way for interface to transmit packets
> faster than the 1Mbps limit. With two points this is enforced by kernel.
> With only one point, we must combine both QoS settings into one which is
> set afterwards. Look at virNetDevBandwidthMinimal() and you'll
> understand immediately what I mean.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/libvirt_private.syms      |  1 +
>  src/network/bridge_driver.c   | 25 +++++++++++-
>  src/util/virnetdevbandwidth.c | 88 +++++++++++++++++++++++++++++++++++++++++++
>  src/util/virnetdevbandwidth.h |  6 +++
>  4 files changed, 118 insertions(+), 2 deletions(-)
> 


>  
> -    if (bandwidth && virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth,
> -                                            bandwidth) < 0)
> +    /* For a bridgeless networks we must take into account any QoS set to them.

s/For a/For/

> +
> +static void
> +virNetDevBandwidthRateMinimal(virNetDevBandwidthRatePtr result,
> +                              virNetDevBandwidthRatePtr band1,
> +                              virNetDevBandwidthRatePtr band2)
> +{
> +    if (!band1 || !band2) {
> +        memcpy(result, band1 ? band1 : band2, sizeof(*result));

Isn't this a NULL deref if both band1 and band2 are NULL? [1]

> +        return;
> +    }
> +
> +    /* We can't do a simple MIN() here, because zero value doesn't mean the
> +     * narrowest limit, but and unset value. Hence we need symmetric F(a, b)

s/and/an/

> +     * so that:
> +     * F(a, 0) = F(0, a) = a; special corner case: F(0, 0) = 0
> +     * F(a, b) = MIN(a, b) for a != 0 and b != 0
> +     */
> +#define NON_ZERO_MIN(a, b) \
> +    ((a) && (b) ? MIN(a, b) : (a) ? (a) : (b))

Nice.

> +virNetDevBandwidthMinimal(virNetDevBandwidthPtr *result,
> +                          virNetDevBandwidthPtr band1,
> +                          virNetDevBandwidthPtr band2)
> +{
> +    int ret = -1;
> +
> +    if (!band1 && !band2) {
> +        /* Nothing to compute */
> +        *result = NULL;
> +        return 0;
> +    }

[1] Ahh, nice, you never call the helper function with both sides NULL.

> +
> +    if (!band1->in && !band2->in) {
> +        /* nada */
> +    } else {

This looks odd; I would use deMorgan's law and write it as a one-liner
condition instead of an awkward 3-line if/else:

if (band1->in || band2->in) {

> +        if (VIR_ALLOC((*result)->in) < 0)
> +            goto cleanup;
> +
> +        virNetDevBandwidthRateMinimal((*result)->in, band1->in, band2->in);
> +    }
> +
> +    if (!band1->out && !band2->out) {
> +        /* nada */
> +    } else {

and again.

ACK with those fixes.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140124/ac47850f/attachment-0001.sig>