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

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:

quoted

On Mon, 25 Jan 2021 16:18:19 +0100 Simon Horman wrote:

quoted

From: Baowen Zheng <redacted>

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 <redacted>
Signed-off-by: Simon Horman <redacted>
Signed-off-by: Louis Peens <redacted>

quoted

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...

quoted

quoted

 };
 
 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

...