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

[Michal Privoznik]

On 29.01.2014 15:26, Laine Stump wrote:
> On 01/23/2014 03:44 PM, 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'/>
>
> You've picked a bad example config - "passthrough" forward mode requires
> exclusive use of a host interface by a single guest, so this network
> could never have more than a single guest interface using it.
>
> To see the problem in your method of solution, you would need to have
> multiple <interface> elements (or <forward mode='bridge'/>). In either
> of those cases you end up with an aggregate network bandwidth of
> "Netlimit" * "# of attached guests".
>
> My understanding of the <bandwidth> setting in a <network> is that it is
> specifying the maximum aggregate bandwidth of all guest interfaces
> connected to that network not the maximum bandwidth of a single guest
> interface. If you want to easily specify the individual bandwidth to all
> guest interfaces on a network, you can just add a default <portgroup> to
> the network, and put a <bandwidth> element in there.
>
> (Of course, any <bandwidth> in a guest's <interface> definition would
> override the bandwidth in the portgroup (we discussed precedence at the
> time that code was added, and decided this was the more useful way,
> since at the time anyone who could modify any domain definition could
> modify any network definition too), but if you want a method for a
> <network> definition to override individual bandwidth limits, that could
> be added as an option to the <portgroup> definition.)
>
>>      </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
>> 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(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 13caf93..0c16343 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1469,6 +1469,7 @@ virNetDevBandwidthClear;
>>   virNetDevBandwidthCopy;
>>   virNetDevBandwidthEqual;
>>   virNetDevBandwidthFree;
>> +virNetDevBandwidthMinimal;
>>   virNetDevBandwidthPlug;
>>   virNetDevBandwidthSet;
>>   virNetDevBandwidthUnplug;
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 0b43a67..904aad5 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -3284,9 +3284,30 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>>       else if (portgroup && portgroup->bandwidth)
>>           bandwidth = portgroup->bandwidth;
>>
>> -    if (bandwidth && virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth,
>> -                                            bandwidth) < 0)
>> +    /* For a bridgeless networks we must take into account any QoS set to them.
>> +     * This is, however, a bit tricky. With bridged network there are two
>> +     * points where QoS can be set: domain's interface and the bridge. The
>> +     * limits on those differ (in general) in which case the lower limit gets
>> +     * into effect. For example, if domain's interface average is 10Mbps but
>> +     * the network's is just 1Mbps, the domain will not be able to send data
>> +     * any faster than 1Mbps. However, on bridgeless networks we can't just set
>> +     * those two points and let kernel do its job since we have only single
>> +     * point. Therefore, we must combine the QoS settings from both domain's
>> +     * interface and network and choose the minimal value from pairs.
>
>  From this description, it sounds like if you have a network with a limit
> of 1Mbps, and 20 guest interfaces connected to that network, then you
> will set a limit of 1Mbps on each guest interface, meaning that the
> maximum bandwidth that could come from the guests on that network is
> 20Mbps, not 1Mbps. So in fact this is not implementing the intent of the
> XML.
>
> I know this has already been pushed (I was unfortunately out of town),
> but if it hadn't been, I would NACK it pending a correction of my
> interpretation.
>

Your interpretation is indeed correct. I must have gone mad when writing 
this patch. Now that I'm thinking over the problem again, there's no 
easy way how to achieve what's being asked.

Hence, I'm sending patch to revert the commit.

Sorry for the noise.

Michal