Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200329005202.17926-1-olteanv@gmail.com> <20200329005202.17926-7-olteanv@gmail.com>
 <20200329095712.GA2188467@splinter> <CA+h21hoybhxhR3KgfRkAaKyPPJPesbGLWDaDp5O_2yTz05y5jQ@mail.gmail.com>
In-Reply-To: <CA+h21hoybhxhR3KgfRkAaKyPPJPesbGLWDaDp5O_2yTz05y5jQ@mail.gmail.com>
From:   Vladimir Oltean <olteanv@gmail.com>
Date:   Sun, 29 Mar 2020 14:46:16 +0300
Message-ID: <CA+h21hoBp6=Zyc3mX3BVguVs0f8Un6-A3pk9YaZKPgs0efTi3g@mail.gmail.com>
Subject: Re: [PATCH net-next 6/6] net: dsa: sja1105: add broadcast and
 per-traffic class policers
To:     Ido Schimmel <idosch@idosch.org>
Cc:     Nikolay Aleksandrov <nikolay@cumulusnetworks.com>,
        Roopa Prabhu <roopa@cumulusnetworks.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jiri Pirko <jiri@resnulli.us>,
        Jakub Kicinski <kuba@kernel.org>,
        netdev <netdev@vger.kernel.org>,
        Xiaoliang Yang <xiaoliang.yang_1@nxp.com>,
        lkml <linux-kernel@vger.kernel.org>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        "Allan W. Nielsen" <allan.nielsen@microchip.com>,
        Joergen Andreasen <joergen.andreasen@microchip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
        "Y.b. Lu" <yangbo.lu@nxp.com>,
        Alexandru Marginean <alexandru.marginean@nxp.com>,
        Po Liu <po.liu@nxp.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Li Yang <leoyang.li@nxp.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 29 Mar 2020 at 14:37, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Sun, 29 Mar 2020 at 12:57, Ido Schimmel <idosch@idosch.org> wrote:
> >
> > + Nik, Roopa
> >
> > On Sun, Mar 29, 2020 at 02:52:02AM +0200, Vladimir Oltean wrote:
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > >
> > > This patch adds complete support for manipulating the L2 Policing Tables
> > > from this switch. There are 45 table entries, one entry per each port
> > > and traffic class, and one dedicated entry for broadcast traffic for
> > > each ingress port.
> > >
> > > Policing entries are shareable, and we use this functionality to support
> > > shared block filters.
> > >
> > > We are modeling broadcast policers as simple tc-flower matches on
> > > dst_mac. As for the traffic class policers, the switch only deduces the
> > > traffic class from the VLAN PCP field, so it makes sense to model this
> > > as a tc-flower match on vlan_prio.
> > >
> > > How to limit broadcast traffic coming from all front-panel ports to a
> > > cumulated total of 10 Mbit/s:
> > >
> > > tc qdisc add dev sw0p0 ingress_block 1 clsact
> > > tc qdisc add dev sw0p1 ingress_block 1 clsact
> > > tc qdisc add dev sw0p2 ingress_block 1 clsact
> > > tc qdisc add dev sw0p3 ingress_block 1 clsact
> > > tc filter add block 1 flower skip_sw dst_mac ff:ff:ff:ff:ff:ff \
> > >       action police rate 10mbit burst 64k
> > >
> > > How to limit traffic with VLAN PCP 0 (also includes untagged traffic) to
> > > 100 Mbit/s on port 0 only:
> > >
> > > tc filter add dev sw0p0 ingress protocol 802.1Q flower skip_sw \
> > >       vlan_prio 0 action police rate 100mbit burst 64k
> > >
> > > The broadcast, VLAN PCP and port policers are compatible with one
> > > another (can be installed at the same time on a port).
> >
> > Hi Vladimir,
> >
> > Some switches have a feature called "storm control". It allows one to
> > police incoming BUM traffic.
>
> Yes, I am aware.
> DPAA2 switches have a single (as far as I am aware) knob for 'flood
> policers', and Ocelot has individual 'storm policers' for unknown
> unicast, for multicast, broadcast and for 'learn frames'.
>
> > See this entry from Cumulus Linux
> > documentation:
> >
> > https://docs.cumulusnetworks.com/cumulus-linux-40/Layer-2/Spanning-Tree-and-Rapid-Spanning-Tree/#storm-control
> >
> > In the past I was thinking about ways to implement this in Linux. The
> > only place in the pipeline where packets are actually classified to
> > broadcast / unknown unicast / multicast is at bridge ingress. Therefore,
>
> Actually I think only 'unknown unicast' is tricky here, and indeed the
> bridge driver is the only place in the software datapath that would
> know that.
> I know very little about frame classification in the Linux network
> stack, but would it be possible to introduce a match key in tc-flower
> for whether packets have a known destination or not?
>
> > my thinking was to implement these storm control policers as a
> > "bridge_slave" operation. It can then be offloaded to capable drivers
> > via the switchdev framework.
> >
>
> I think it would be a bit odd to duplicate tc functionality in the
> bridge sysfs. I don't have a better suggestion though.
>

Not to mention that for hardware like this, to have the same level of
flexibility via a switchdev control would mean to duplicate quite a
lot of tc functionality. On this 5-port switch I can put a shared
broadcast policer on 2 ports (via the ingress_block functionality),
and individual policers on the other 3, and the bandwidth budgeting is
separate. I can only assume that there are more switches out there
that allow this.

> > I think that if we have this implemented in the Linux bridge, then your
> > patch can be used to support the policing of broadcast packets while
> > returning an error if user tries to police unknown unicast or multicast
> > packets.
>
> So even if the Linux bridge gains these knobs for flood policers,
> still have the dst_mac ff:ff:ff:ff:ff:ff as a valid way to configure
> one of those knobs?
>
> > Or maybe the hardware you are working with supports these types
> > as well?
>
> Nope, on this hardware it's just broadcast, I just checked that. Which
> simplifies things quite a bit.
>
> >
> > WDYT?
> >
>
> I don't know.
>
> Thanks,
> -Vladimir

-Vladimir