On Mon, Nov 16, 2020 at 08:39:54PM +0200, Grygorii Strashko wrote:
>
>
> On 14/11/2020 21:17, Vladimir Oltean wrote:
> > On Sat, Nov 14, 2020 at 05:56:54AM +0200, Grygorii Strashko wrote:
> > > This patch enables support for ingress broadcast(BC)/multicast(MC) rate
> > > limiting
> > > in TI AM65x CPSW driver (the corresponding ALE support was added in
> > > previous
> > > patch) by implementing HW offload for simple tc-flower policer with
> > > matches
> > > on dst_mac:
> > > - ff:ff:ff:ff:ff:ff has to be used for BC rate limiting
> > > - 01:00:00:00:00:00 fixed value has to be used for MC rate limiting
> > >
> > > Hence tc policer defines rate limit in terms of bits per second, but the
> > > ALE supports limiting in terms of packets per second - the rate limit
> > > bits/sec is converted to number of packets per second assuming minimum
> > > Ethernet packet size ETH_ZLEN=60 bytes.
> > >
> > > Examples:
> > > - BC rate limit to 1000pps:
> > > tc qdisc add dev eth0 clsact
> > > tc filter add dev eth0 ingress flower skip_sw dst_mac
> > > ff:ff:ff:ff:ff:ff \
> > > action police rate 480kbit burst 64k
> > >
> > > rate 480kbit - 1000pps * 60 bytes * 8, burst - not used.
> > >
> > > - MC rate limit to 20000pps:
> > > tc qdisc add dev eth0 clsact
> > > tc filter add dev eth0 ingress flower skip_sw dst_mac
> > > 01:00:00:00:00:00 \
> > > action police rate 9600kbit burst 64k
> > >
> > > rate 9600kbit - 20000pps * 60 bytes * 8, burst - not used.
> > >
> > > Signed-off-by: Grygorii Strashko <grygorii.stras...@ti.com>
> > > ---
> >
> > I understand this is unpleasant feedback, but don't you want to extend
> > tc-police to have an option to rate-limit based on packet count and not
> > based on byte count?
>
> Huh.
> I'd be appreciated if you could provide more detailed opinion of how it can
> look like?
> Sry, it's my first experience with tc.
Same as above, just in packets per second.
tc qdisc add dev eth0 clsact
tc filter add dev eth0 ingress flower skip_sw \
dst_mac 01:00:00:00:00:00/01:00:00:00:00:00 \
action police rate 20kpps
> > The assumption you make in the driver that the
> > packets are all going to be minimum-sized is not a great one.
> > I can
> > imagine that the user's policer budget is vastly exceeded if they enable
> > jumbo frames and they put a policer at 9.6 Mbps, and this is not at all
> > according to their expectation. 20Kpps assuming 60 bytes per packet
> > might be 9.6 Mbps, and the user will assume this bandwidth profile is
> > not exceeded, that's the whole point. But 20Kpps assuming 9KB per packet
> > is 1.44Gbps. Weird.
>
> Sry, not sure I completely understood above. This is specific HW feature,
> which can
> imit packet rate only. And it is expected to be applied by admin who know
> what he is doing.
Yes but you're not helping the admin to "know what he's doing" if you're
asking them to translate apples into oranges. A policer that counts
packets is not equivalent to a policer that counts bytes, unless all
packets are guaranteed to be of equal length, something which you cannot
ensure.
> The main purpose is to preserve CPU resource, which first of all affected by
> packet rate.
> So, I see it as not "assumption", but requirement/agreement which will be
> reflected
> in docs and works for a specific use case which is determined by presence of:
> - skip_sw flag
> - specific dst_mac/dst_mac_mask pair
> in which case rate determines pps and not K/Mbps.
>
>
> Also some ref on previous discussion [1] [2]
> [1] https://www.spinics.net/lists/netdev/msg494630.html
> [2] https://lore.kernel.org/patchwork/patch/481285/
ethtool coalescing as a tool to configure a policer makes zero sense.
You are definitely on the right path with the tc-police action. This was
just trying to be constructive feedback that the software implementation
of tc-police needs more work before your hardware could offload its job
in a way that would not violate its semantics.
