Re: [PATCH net-next 10/11] net: marvell: prestera: add storm control (rate limiter) implementation

On Fri, Jun 11, 2021 at 07:08:00PM +0200, Andrew Lunn wrote:

On Fri, Jun 11, 2021 at 01:19:13PM +0000, Oleksandr Mazur wrote:

quoted

quoted

quoted

 On Wed, Jun 09, 2021 at 06:16:00PM +0300, Oleksandr Mazur wrote:

Storm control (BUM) provides a mechanism to limit rate of ingress

quoted

port traffic (matched by type). Devlink port parameter API is used:
driver registers a set of per-port parameters that can be accessed to both
get/set per-port per-type rate limit.
Add new FW command - RATE_LIMIT_MODE_SET.

quoted

Hi Oleksandr

quoted

Just expanding on the two comments you already received about this.

quoted

We often see people miss that switchdev is about. It is not about
writing switch drivers. It is about writing network stack
accelerators. You take a feature of the Linux network stack and you
accelerate it by offloading it to the hardware. So look around the
network stack and see how you configure it to perform rate limiting of
broadcast traffic ingress. Once you have found a suitable mechanism,
accelerate it via offloading.

quoted

If you find Linux has no way to perform a feature the hardware could
accelerate, you first need to add a pure software version of that
feature to the network stack, and then add acceleration support for
it.

Hello Andrew, Ido, Nikolay,
I appreciate your time and comments provided over this patchset, though i have a few questions to ask, if you don't mind:

quoted

1. Does it mean that in order to support storm control in switchdev
driver i need to implement software storm control in bridge driver,
and then using the switchdev attributes (notifiers) mechanism
offload the configuration itself to the HW?

Hi Oleksandr

Not necessarily. Is storm control anything more than ingress packet
matching and rate limiting?

I'm not TC expert, but look for example at
https://man7.org/linux/man-pages/man8/tc-police.8.html

and the example:

# tc qdisc add dev eth0 handle ffff: ingress
# tc filter add dev eth0 parent ffff: u32 \
                   match u32 0 0 \
                   police rate 1mbit burst 100k

Replace the "match u32 0 0" with something which matches on broadcast
frames.  Maybe "flower dst_mac ff:ff:ff:ff:ff:ff"

So there is a software solution. Now accelerate it.

Storm control also needs the ability to limit other types of flooded
traffic such unknown unicast and unregistered multicast packets. The
entity which classifies packets as such is the bridge, which happens
after the ingress hook.

I see two options to support storm control in Linux:

1. By adding support in the bridge itself as a new bridge slave option.
Something like:

# ip link set dev swp1 type bridge_slave \
	storm_control type { uuc | umc | bc} rate RATE mode { packet | byte }

I suspect this similar to more traditional implementations that users
might be used to and also maps nicely to hardware implementations

2. Teaching tc to call into the bridge to classify a packet. Not sure a
whole new classifier is needed for this. Maybe just extend flower with a
new key: dst_mac_type { uuc | umc }. I personally find this a bit weird,
but it is more flexible and allows to reuse existing actions

quoted

2. Is there any chance of keeping devlink solution untill the
discussed (storm control implemented in the bridge driver) mechanism
will be ready/implemented?

No. Please do it correctly from the beginning. No hacks.

+1