Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 14 Nov 2020 21:17:23 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Grygorii Strashko <grygorii.strashko@ti.com>
Cc:     "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Sekhar Nori <nsekhar@ti.com>, linux-kernel@vger.kernel.org,
        linux-omap@vger.kernel.org, Tony Lindgren <tony@atomide.com>
Subject: Re: [PATCH net-next 3/3] net: ethernet: ti: am65-cpsw: enable
 broadcast/multicast rate limit support
Message-ID: <20201114191723.rvmhyrqinkhdjtpr@skbuf>
References: <20201114035654.32658-1-grygorii.strashko@ti.com>
 <20201114035654.32658-4-grygorii.strashko@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201114035654.32658-4-grygorii.strashko@ti.com>
Precedence: bulk

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.strashko@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? 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.