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

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

On Wed, Jan 27, 2021 at 12:02:23PM +0100, Simon Horman wrote:
> Hi Jakub,
> 
> On Tue, Jan 26, 2021 at 06:38:12PM -0800, Jakub Kicinski wrote:
> > On Mon, 25 Jan 2021 16:18:19 +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.
> > > 
> > > 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>
> > 
> > > diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> > > index 8d8452b1cdd4..d700b2105535 100644
> > > --- a/net/sched/act_police.c
> > > +++ b/net/sched/act_police.c
> > > @@ -42,6 +42,8 @@ static const struct nla_policy police_policy[TCA_POLICE_MAX + 1] = {
> > >  	[TCA_POLICE_RESULT]	= { .type = NLA_U32 },
> > >  	[TCA_POLICE_RATE64]     = { .type = NLA_U64 },
> > >  	[TCA_POLICE_PEAKRATE64] = { .type = NLA_U64 },
> > > +	[TCA_POLICE_PKTRATE64]  = { .type = NLA_U64 },
> > > +	[TCA_POLICE_PKTBURST64] = { .type = NLA_U64 },
> > 
> > Should we set the policy so that .min = 1?
> 
> Yes, I think so.
> Thanks for spotting that.

It seems that I was mistaken.

A value of 0 is used to clear packet-per-second rate limiting
while leaving bit-per-second rate configuration in place for a
policer action.

So I think the policy should be left as is...

> > >  };
> > >  
> > >  static int tcf_police_init(struct net *net, struct nlattr *nla,
> > > @@ -61,6 +63,7 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
> > >  	bool exists = false;
> > >  	u32 index;
> > >  	u64 rate64, prate64;
> > > +	u64 pps, ppsburst;
> > >  
> > >  	if (nla == NULL)
> > >  		return -EINVAL;
> > > @@ -183,6 +186,16 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
> > >  	if (tb[TCA_POLICE_AVRATE])
> > >  		new->tcfp_ewma_rate = nla_get_u32(tb[TCA_POLICE_AVRATE]);
> > >  
> > > +	if (tb[TCA_POLICE_PKTRATE64] && tb[TCA_POLICE_PKTBURST64]) {
> > 
> > Should we reject if only one is present?
> 
> Again, yes I think so.
> I'll confirm this with the author too.

... but add this restriction so the code will require either:

1. Both PKTRATE64 and PKTBURST64 are non-zero: packet-per-second limit is set;
   or
2. Both PKTRATE64 and PKTBURST64 are zero: packet-per-second limit is cleared

...