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(a)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