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@xxxxxxxxxx> > --- > 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list