> On October 5, 2016 at 6:55 PM Ben Pfaff <b...@ovn.org> wrote: > > > On Mon, Oct 03, 2016 at 04:49:57PM +0200, Wolfgang Bumiller wrote: > > > On September 30, 2016 at 5:15 PM Ben Pfaff <b...@ovn.org> wrote: > > > > > > > > > On Fri, Sep 30, 2016 at 02:33:21PM +0200, Wolfgang Bumiller wrote: > > > > I've noticed openvswitch apparently blindly deletes the ingress qdisc > > > > of bridge ports even if no ingress policing is configured. > > > > This also prevents the use of this qdisc for purposes other than the > > > > in OVS implemented ingress policing (eg. handling fw-marks, bpf based > > > > filters, or early port/address/mac filtering can be convenient at this > > > > stage). Reading the source code didn't reveal any option to prevent > > > > the qdisc from staying completely unmanaged, which would be the > > > > simplest solution here. > > > > > > > > What are your thoughts on this? > > > > > > This is analogous to the situation OVS had for egress qdiscs, which I > > > summarized in this email: > > > http://openvswitch.org/pipermail/discuss/2015-May/017687.html > > > > > > We solved this in OVS 2.6 with commit 6cf888b821c: > > > > > > https://github.com/openvswitch/ovs/commit/6cf888b821cffb75c5723ee76b7103e54b8fa2b5 > > > > > > Probably, some similar scheme would work for ingress qdiscs. Your > > > thoughts? > > > > Makes sense, but the ingress policing currently doesn't go through a > > tc-ops layer. > > The resolution there was simply to add a way to prevent OVS from messing > with the qdisc. The resolution here could be the same: add a way to > prevent OVS from messing with the ingress qdisc. (It's regrettable that > it works this way: we should have had enough insight from the beginning > so that OVS only messed with either qdisc if actually invited to do so. > However, we did what we did and backward compatibility remains > important.) > > > I have, however, noticed a related patch on the devel list which > > looks like there's some ongoing work in that area: > > http://openvswitch.org/pipermail/dev/2016-September/079948.html > > I wonder how this interacts with the current code, since when > > iface_configure_qos() gets called it causes the ingress qdisc to be > > deleted and recreated directly (netdev-linux.c, > > netdev_linux_set_policing()). > > It looks like this would currently conflict? > > I don't think that it makes sense to conflate the ingress and egress > qdiscs the way this patch does: > http://openvswitch.org/pipermail/dev/2016-October/080261.html > > > Basically, iface_configure_qos() currently directly uses > > `iface->cfg->ingress_policing_rate/burst` (from an ovsrec_interface, > > which is autogenerated, which is why I mentioned the autogenerated > > code initially, as I haven't found an obvious way to deal with this > > yet), > > I don't know what you're trying to deal with. The autogenerated code > provides a C view of all of the features that the underlying database > does. If you need something else, then it probably means extending the > database schema, at which point the autogenerated code will give a view > of whatever you add to the schema.
I was just worried that the schema would be involved in interface/rpc code which could potentially break tools built with the old schema. (Considering it comes with a checksum) But I don't see another option. Would you generally prefer an additional boolean flag for a change like this or perhaps add -1 special value for the ingress_policing_rate. A quick glance at the existing schema didn't point out many existing special values other than a semi intuitive active_timeout where 0 means default and -1 means disabled. In the case of ingress_policing_rate 0 would mean disabled and -1 would mean untouched. > > > and sends it off to netdev_linux_set_policing() which > > deletes+recreates the ingress qdisc, potentially replacing the ingress > > qdisc which the above patch allows me to create. > > > _______________________________________________ discuss mailing list discuss@openvswitch.org http://openvswitch.org/mailman/listinfo/discuss
