On Fri, Mar 22, 2019 at 10:39:40AM -0400, Cole Robinson wrote: > On 3/22/19 5:04 AM, Michal Privoznik wrote: > > On 3/22/19 3:02 AM, Laine Stump wrote: > >> On 3/21/19 9:52 PM, Laine Stump wrote: > >>> On 3/21/19 8:58 PM, Cole Robinson wrote: > >>>> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote: > >>>>> In the case of a network with forward=bridge, which has a bridge > >>>>> device > >>>>> listed, we are capable of setting bandwidth limits but fail to call > >>>>> the > >>>>> function to register them. > >>>>> > >>>>> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx> > >>>>> --- > >>>>> src/network/bridge_driver.c | 39 > >>>>> ++++++++++++++++++++++++++----------- > >>>>> 1 file changed, 28 insertions(+), 11 deletions(-) > >>>>> > >>>> One thing missing is class_id XML reading in > >>>> virDomainActualNetDefParseXML, that needs to be adjusted for > >>>> TYPE_BRIDGE > >>>> > >>>> With that, code wise I'll give: > >>>> > >>>> Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx> > >>>> > >>>> but I can't really comment on if there's any hidden pitfalls. > >>> > >>> > >>> I seem to recall that Michal omitted bandwidth support on those types > >>> of networks for a reason (floor can't be supported because there > >>> isn't a single egress that we have exclusive control over, or > >>> something like that), but he should probably give the definitive > >>> response to that. > > > > Floor of an <interface/> is actually set on the bridge it's connected to > > because that is where all the traffic goes through so that's where we > > can set up traffic shaping so that floor can be honoured. By the way > > that is the reason why corresponding network is required to have > > <inbound/> at least. If there is no traffic shaping set on the bridge > > (in direction from bridge to VM interfaces) then: > > a) we don't know where to place qdisc that guarantees the floor > > b) what is the upper limit for the sum of all floor values > > > > Long story short, qdiscs are a black box that can do various actions on > > packets that land in them. For traffic shaping I choose HTB which can > > delay packets so that required values are met (link rate, ceil, burst). > > In order to implement floor I needed to create some hierarchy of hose > > HTBs at a place where the whole traffic can be seen => the network > > bridge. Now, by default there is this qdisc named 1:1 aka 'class id' > > (there is some hierarchy in the naming too, unimportant for now) and > > whole traffic that's directed to <interface/>-s without floor goes > > through there. By default this qdisc is allowed to send packets at > > network's <inbound average/>. At the moment that new <interface/> with > > floor is plugged into the bridge, the default qdisc has its rate > > decreased and new qdisc is created (the traffic for the new <interface/> > > will go through there). > > > > That is why we need a) and b). This is also the reason why we can't > > enable floor for network type='bridge'. Libvirt has no control over the > > bridge, nor traffic shaping rules on it. > > > > FWIW patch 12 deals with some stuff relating to the 'floor' bit, not > sure if it impacts this discussion at all That patch is dealing with precisely the situation described here. It ensures we only allow use of floor when we have type=network. > > class_id is a private attribute to keep mapping of <interface/> <--> > > qdisc and should never be set by user. Nor they need to know its value. > > > > Right, this subthread was just about the private runtime VM status XML, > where the class id is set so it can be restored on daemon restart. > Current code only sets it for actualType == TYPE_NETWORK but sounds like > it needs to do so now for actualType == TYPE_BRIDGE Yes it does need to. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
