> 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
> > 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:
> > 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