Re: [PATCH net-next 6/6] net: dsa: sja1105: add broadcast and per-traffic class policers

[Nikolay Aleksandrov <nikolay@cumulusnetworks.com>]

On 29/03/2020 14:46, Vladimir Oltean wrote:
> 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>
[snip]
>>> 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.

Yep, unknown unicast is hard to pass outside of the bridge, especially at ingress
where the bridge hasn't been hit yet. One possible solution is to expose a function
from the bridge which can make such a decision at the cost of 1 more fdb hash lookup,
but if the packet is going to hit the bridge anyway that cost won't be that high
since it will have to do the same. We already have some internal bridge functionality
exposed for netfilter, tc and some drivers so it would be in line with that.
I haven't looked into how feasible the above is, so I'm open to other ideas (the
bridge_slave functions for example, we've discussed such extensions before in other
contexts). But I think this can be much simpler if we just expose the unknown unicast
information, the mcast/bcast can be decided by the classifier already or with very
little change. I think such exposed function can be useful to netfilter as well.

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