Re: [PATCH RFC net-next] net/sched: act_police: add support for packet-per-second policing

[Simon Horman <simon.horman@netronome.com>]

On Mon, Feb 01, 2021 at 01:31:17PM +0100, Simon Horman wrote:
> On Thu, Jan 28, 2021 at 06:19:33PM +0200, Ido Schimmel wrote:
> > On Mon, Jan 25, 2021 at 04:18:19PM +0100, Simon Horman wrote:
> > > From: Baowen Zheng <baowen.zheng@corigine.com>
> > > 
> > > Allow a policer action to enforce a rate-limit based on packets-per-second,
> > > configurable using a packet-per-second rate and burst parameters. This may
> > > be used in conjunction with existing byte-per-second rate limiting in the
> > > same policer action.
> > 
> > Hi Simon,
> > 
> > Any reason to allow metering based on both packets and bytes at the same
> > action versus adding a mode (packets / bytes) parameter? You can then
> > chain two policers if you need to rate limit based on both. Something
> > like:
> > 
> > # tc filter add dev tap1 ingress pref 1 matchall \
> > 	action police rate 1000Mbit burst 128k conform-exceed drop/pipe \
> > 	action police pkts_rate 3000 pkts_burst 1000
> > 
> > I'm asking because the policers in the Spectrum ASIC are built that way
> > and I also don't remember seeing such a mixed mode online.
> 
> Hi Ido,
> 
> sorry for missing this email until you pointed it out to me in another
> thread.
> 
> We did consider this question during development and our conclusion was
> that it was useful as we do have use-cases which call for both to be used
> and it seems nice to allow lower layers to determine the order in which the
> actions are applied to satisfied the user's more general request for both -
> it should be no surprise that we plan to provide a hardware offload of this
> feature. It also seems to offer nice code re-use. We did also try to
> examine the performance impact of this change on existing use-cases and it
> appeared to be negligible/within noise of our measurements.
> 
> > > e.g.
> > > tc filter add dev tap1 parent ffff: u32 match \
> > >               u32 0 0 police pkts_rate 3000 pkts_burst 1000
> > > 
> > > Testing was unable to uncover a performance impact of this change on
> > > existing features.
> > > 
> > > Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
> > > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > > Signed-off-by: Louis Peens <louis.peens@netronome.com>
> > > ---
> > >  include/net/sch_generic.h      | 15 ++++++++++++++
> > >  include/net/tc_act/tc_police.h |  4 ++++
> > >  include/uapi/linux/pkt_cls.h   |  2 ++
> > >  net/sched/act_police.c         | 37 +++++++++++++++++++++++++++++++---
> > >  net/sched/sch_generic.c        | 32 +++++++++++++++++++++++++++++
> > >  5 files changed, 87 insertions(+), 3 deletions(-)
> > 
> > The intermediate representation in include/net/flow_offload.h needs to
> > carry the new configuration so that drivers will be able to veto
> > unsupported configuration.

Thanks for pointing that out. I do have a patch for that but was planning
to post it as a follow-up to keep this series simple. I do see your point
that the flow_offload.h change is a dependency of this patch. I'll include
it when reposting.