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

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>
---
 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(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 639e465a108f..1a93ad634fee 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h

@@ -1234,6 +1234,21 @@ static inline void psched_ratecfg_getrate(struct tc_ratespec *res,
 	res->linklayer = (r->linklayer & TC_LINKLAYER_MASK);
 }
 
+struct psched_pktrate {
+	u64	rate_pkts_ps; /* packets per second */
+	u32	mult;
+	u8	shift;
+};
+
+static inline u64 psched_pkt2t_ns(const struct psched_pktrate *r,
+				  unsigned int pkt_num)
+{
+	return ((u64)pkt_num * r->mult) >> r->shift;
+}
+
+void psched_ppscfg_precompute(struct psched_pktrate *r,
+			      u64 pktrate64);
+
 /* Mini Qdisc serves for specific needs of ingress/clsact Qdisc.
  * The fast path only needs to access filter list and to update stats
  */

diff --git a/include/net/tc_act/tc_police.h b/include/net/tc_act/tc_police.h
index 6d1e26b709b5..b106460a8d66 100644
--- a/include/net/tc_act/tc_police.h
+++ b/include/net/tc_act/tc_police.h

@@ -10,10 +10,13 @@ struct tcf_police_params {
 	s64			tcfp_burst;
 	u32			tcfp_mtu;
 	s64			tcfp_mtu_ptoks;
+	s64			tcfp_pkt_burst;
 	struct psched_ratecfg	rate;
 	bool			rate_present;
 	struct psched_ratecfg	peak;
 	bool			peak_present;
+	struct psched_pktrate	ppsrate;
+	bool			pps_present;
 	struct rcu_head rcu;
 };
 

@@ -24,6 +27,7 @@ struct tcf_police {
 	spinlock_t		tcfp_lock ____cacheline_aligned_in_smp;
 	s64			tcfp_toks;
 	s64			tcfp_ptoks;
+	s64			tcfp_pkttoks;
 	s64			tcfp_t_c;
 };
 

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index ee95f42fb0ec..bffa54e73785 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h

@@ -190,6 +190,8 @@ enum {
 	TCA_POLICE_PAD,
 	TCA_POLICE_RATE64,
 	TCA_POLICE_PEAKRATE64,
+	TCA_POLICE_PKTRATE64,
+	TCA_POLICE_PKTBURST64,
 	__TCA_POLICE_MAX
 #define TCA_POLICE_RESULT TCA_POLICE_RESULT
 };

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 },
 };
 
 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]) {
+		pps = nla_get_u64(tb[TCA_POLICE_PKTRATE64]);
+		ppsburst = nla_get_u64(tb[TCA_POLICE_PKTBURST64]);
+		if (pps) {
+			new->pps_present = true;
+			new->tcfp_pkt_burst = PSCHED_TICKS2NS(ppsburst);
+			psched_ppscfg_precompute(&new->ppsrate, pps);
+		}
+	}
+
 	spin_lock_bh(&police->tcf_lock);
 	spin_lock_bh(&police->tcfp_lock);
 	police->tcfp_t_c = ktime_get_ns();

@@ -217,8 +230,8 @@ static int tcf_police_act(struct sk_buff *skb, const struct tc_action *a,
 			  struct tcf_result *res)
 {
 	struct tcf_police *police = to_police(a);
+	s64 now, toks, ppstoks = 0, ptoks = 0;
 	struct tcf_police_params *p;
-	s64 now, toks, ptoks = 0;
 	int ret;
 
 	tcf_lastuse_update(&police->tcf_tm);

@@ -236,7 +249,7 @@ static int tcf_police_act(struct sk_buff *skb, const struct tc_action *a,
 	}
 
 	if (qdisc_pkt_len(skb) <= p->tcfp_mtu) {
-		if (!p->rate_present) {
+		if (!p->rate_present && !p->pps_present) {
 			ret = p->tcfp_result;
 			goto end;
 		}

@@ -251,14 +264,22 @@ static int tcf_police_act(struct sk_buff *skb, const struct tc_action *a,
 			ptoks -= (s64)psched_l2t_ns(&p->peak,
 						    qdisc_pkt_len(skb));
 		}
+		if (p->pps_present) {
+			ppstoks = min_t(s64, now - police->tcfp_t_c, p->tcfp_pkt_burst);
+			ppstoks += police->tcfp_pkttoks;
+			if (ppstoks > p->tcfp_pkt_burst)
+				ppstoks = p->tcfp_pkt_burst;
+			ppstoks -= (s64)psched_pkt2t_ns(&p->ppsrate, 1);
+		}
 		toks += police->tcfp_toks;
 		if (toks > p->tcfp_burst)
 			toks = p->tcfp_burst;
 		toks -= (s64)psched_l2t_ns(&p->rate, qdisc_pkt_len(skb));
-		if ((toks|ptoks) >= 0) {
+		if ((toks | ptoks | ppstoks) >= 0) {
 			police->tcfp_t_c = now;
 			police->tcfp_toks = toks;
 			police->tcfp_ptoks = ptoks;
+			police->tcfp_pkttoks = ppstoks;
 			spin_unlock_bh(&police->tcfp_lock);
 			ret = p->tcfp_result;
 			goto inc_drops;

@@ -331,6 +352,16 @@ static int tcf_police_dump(struct sk_buff *skb, struct tc_action *a,
 				      TCA_POLICE_PAD))
 			goto nla_put_failure;
 	}
+	if (p->pps_present) {
+		if (nla_put_u64_64bit(skb, TCA_POLICE_PKTRATE64,
+				      police->params->ppsrate.rate_pkts_ps,
+				      TCA_POLICE_PAD))
+			goto nla_put_failure;
+		if (nla_put_u64_64bit(skb, TCA_POLICE_PKTBURST64,
+				      PSCHED_NS2TICKS(p->tcfp_pkt_burst),
+				      TCA_POLICE_PAD))
+			goto nla_put_failure;
+	}
 	if (nla_put(skb, TCA_POLICE_TBF, sizeof(opt), &opt))
 		goto nla_put_failure;
 	if (p->tcfp_result &&

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 49eae93d1489..209e34ee4bd3 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c

@@ -1361,6 +1361,38 @@ void psched_ratecfg_precompute(struct psched_ratecfg *r,
 }
 EXPORT_SYMBOL(psched_ratecfg_precompute);
 
+void psched_ppscfg_precompute(struct psched_pktrate *r,
+			      u64 pktrate64)
+{
+	memset(r, 0, sizeof(*r));
+	r->rate_pkts_ps = pktrate64;
+	r->mult = 1;
+	/* The deal here is to replace a divide by a reciprocal one
+	 * in fast path (a reciprocal divide is a multiply and a shift)
+	 *
+	 * Normal formula would be :
+	 *  time_in_ns = (NSEC_PER_SEC * pkt_num) / pktrate64
+	 *
+	 * We compute mult/shift to use instead :
+	 *  time_in_ns = (len * mult) >> shift;
+	 *
+	 * We try to get the highest possible mult value for accuracy,
+	 * but have to make sure no overflows will ever happen.
+	 */
+	if (r->rate_pkts_ps > 0) {
+		u64 factor = NSEC_PER_SEC;
+
+		for (;;) {
+			r->mult = div64_u64(factor, r->rate_pkts_ps);
+			if (r->mult & (1U << 31) || factor & (1ULL << 63))
+				break;
+			factor <<= 1;
+			r->shift++;
+		}
+	}
+}
+EXPORT_SYMBOL(psched_ppscfg_precompute);
+
 static void mini_qdisc_rcu_func(struct rcu_head *head)
 {
 }

-- 
2.20.1

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

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>

Should we set the policy so that .min = 1?

Should we reject if only one is present?

+		pps = nla_get_u64(tb[TCA_POLICE_PKTRATE64]);
+		ppsburst = nla_get_u64(tb[TCA_POLICE_PKTBURST64]);
+		if (pps) {
+			new->pps_present = true;
+			new->tcfp_pkt_burst = PSCHED_TICKS2NS(ppsburst);
+			psched_ppscfg_precompute(&new->ppsrate, pps);
+		}
+	}
+
 	spin_lock_bh(&police->tcf_lock);
 	spin_lock_bh(&police->tcfp_lock);
 	police->tcfp_t_c = ktime_get_ns();

+void psched_ppscfg_precompute(struct psched_pktrate *r,
+			      u64 pktrate64)
+{
+	memset(r, 0, sizeof(*r));
+	r->rate_pkts_ps = pktrate64;
+	r->mult = 1;
+	/* The deal here is to replace a divide by a reciprocal one
+	 * in fast path (a reciprocal divide is a multiply and a shift)
+	 *
+	 * Normal formula would be :
+	 *  time_in_ns = (NSEC_PER_SEC * pkt_num) / pktrate64
+	 *
+	 * We compute mult/shift to use instead :
+	 *  time_in_ns = (len * mult) >> shift;
+	 *
+	 * We try to get the highest possible mult value for accuracy,
+	 * but have to make sure no overflows will ever happen.
+	 */
+	if (r->rate_pkts_ps > 0) {
+		u64 factor = NSEC_PER_SEC;
+
+		for (;;) {
+			r->mult = div64_u64(factor, r->rate_pkts_ps);
+			if (r->mult & (1U << 31) || factor & (1ULL << 63))
+				break;
+			factor <<= 1;
+			r->shift++;

Aren't there helpers somewhere for the reciprocal divide
pre-calculation?

+		}
+	}
+}
+EXPORT_SYMBOL(psched_ppscfg_precompute);

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:

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.

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.

quoted

+		pps = nla_get_u64(tb[TCA_POLICE_PKTRATE64]);
+		ppsburst = nla_get_u64(tb[TCA_POLICE_PKTBURST64]);
+		if (pps) {
+			new->pps_present = true;
+			new->tcfp_pkt_burst = PSCHED_TICKS2NS(ppsburst);
+			psched_ppscfg_precompute(&new->ppsrate, pps);
+		}
+	}
+
 	spin_lock_bh(&police->tcf_lock);
 	spin_lock_bh(&police->tcfp_lock);
 	police->tcfp_t_c = ktime_get_ns();

quoted

+void psched_ppscfg_precompute(struct psched_pktrate *r,
+			      u64 pktrate64)
+{
+	memset(r, 0, sizeof(*r));
+	r->rate_pkts_ps = pktrate64;
+	r->mult = 1;
+	/* The deal here is to replace a divide by a reciprocal one
+	 * in fast path (a reciprocal divide is a multiply and a shift)
+	 *
+	 * Normal formula would be :
+	 *  time_in_ns = (NSEC_PER_SEC * pkt_num) / pktrate64
+	 *
+	 * We compute mult/shift to use instead :
+	 *  time_in_ns = (len * mult) >> shift;
+	 *
+	 * We try to get the highest possible mult value for accuracy,
+	 * but have to make sure no overflows will ever happen.
+	 */
+	if (r->rate_pkts_ps > 0) {
+		u64 factor = NSEC_PER_SEC;
+
+		for (;;) {
+			r->mult = div64_u64(factor, r->rate_pkts_ps);
+			if (r->mult & (1U << 31) || factor & (1ULL << 63))
+				break;
+			factor <<= 1;
+			r->shift++;

Aren't there helpers somewhere for the reciprocal divide
pre-calculation?

Now that you mention it, yes.

Looking over reciprocal_divide() I don't think it a good fit here as it
operates on 32bit values, whereas the packet rate is 64 bit.

Packet rate could be changed to a 32 bit entity if we convince ourselves we
don't want more than 2^32 - 1 packets per second (a plausible position
IMHO) - but that leads us to a secondary issue.

The code above is very similar to an existing (long existing)
byte rate variant of this helper - psched_ratecfg_precompute().
And I do think we want to:
a) Support 64-bit byte rates. Indeed such support seems to have
   been added to support 25G use-cases
b) Calculate byte and packet rates the same way

So I feel less and less that reciprocal_divide() is a good fit.
But perhaps I am mistaken.

In the meantime I will take a look to see if a helper common function can
be made to do (64 bit) reciprocal divides for the packet and byte rate
use-cases.  I.e. the common code in psched_ppscfg_precompute() and
psched_ratecfg_precompute().

quoted

+		}
+	}
+}
+EXPORT_SYMBOL(psched_ppscfg_precompute);

On Wed, 27 Jan 2021 12:02:23 +0100 Simon Horman wrote:

quoted

quoted

+void psched_ppscfg_precompute(struct psched_pktrate *r,
+			      u64 pktrate64)
+{
+	memset(r, 0, sizeof(*r));
+	r->rate_pkts_ps = pktrate64;
+	r->mult = 1;
+	/* The deal here is to replace a divide by a reciprocal one
+	 * in fast path (a reciprocal divide is a multiply and a shift)
+	 *
+	 * Normal formula would be :
+	 *  time_in_ns = (NSEC_PER_SEC * pkt_num) / pktrate64
+	 *
+	 * We compute mult/shift to use instead :
+	 *  time_in_ns = (len * mult) >> shift;
+	 *
+	 * We try to get the highest possible mult value for accuracy,
+	 * but have to make sure no overflows will ever happen.
+	 */
+	if (r->rate_pkts_ps > 0) {
+		u64 factor = NSEC_PER_SEC;
+
+		for (;;) {
+			r->mult = div64_u64(factor, r->rate_pkts_ps);
+			if (r->mult & (1U << 31) || factor & (1ULL << 63))
+				break;
+			factor <<= 1;
+			r->shift++;  

Aren't there helpers somewhere for the reciprocal divide
pre-calculation?  

Now that you mention it, yes.

Looking over reciprocal_divide() I don't think it a good fit here as it
operates on 32bit values, whereas the packet rate is 64 bit.

Packet rate could be changed to a 32 bit entity if we convince ourselves we
don't want more than 2^32 - 1 packets per second (a plausible position
IMHO) - but that leads us to a secondary issue.

The code above is very similar to an existing (long existing)
byte rate variant of this helper - psched_ratecfg_precompute().
And I do think we want to:
a) Support 64-bit byte rates. Indeed such support seems to have
   been added to support 25G use-cases
b) Calculate byte and packet rates the same way

So I feel less and less that reciprocal_divide() is a good fit.
But perhaps I am mistaken.

In the meantime I will take a look to see if a helper common function can
be made to do (64 bit) reciprocal divides for the packet and byte rate
use-cases.  I.e. the common code in psched_ppscfg_precompute() and
psched_ratecfg_precompute().

No strong feelings, I'll just ask to document the reasoning in the
commit message or the comment above.

On Wed, Jan 27, 2021 at 12:59:05PM -0800, Jakub Kicinski wrote:

On Wed, 27 Jan 2021 12:02:23 +0100 Simon Horman wrote:

quoted

quoted

quoted

+void psched_ppscfg_precompute(struct psched_pktrate *r,
+			      u64 pktrate64)
+{
+	memset(r, 0, sizeof(*r));
+	r->rate_pkts_ps = pktrate64;
+	r->mult = 1;
+	/* The deal here is to replace a divide by a reciprocal one
+	 * in fast path (a reciprocal divide is a multiply and a shift)
+	 *
+	 * Normal formula would be :
+	 *  time_in_ns = (NSEC_PER_SEC * pkt_num) / pktrate64
+	 *
+	 * We compute mult/shift to use instead :
+	 *  time_in_ns = (len * mult) >> shift;
+	 *
+	 * We try to get the highest possible mult value for accuracy,
+	 * but have to make sure no overflows will ever happen.
+	 */
+	if (r->rate_pkts_ps > 0) {
+		u64 factor = NSEC_PER_SEC;
+
+		for (;;) {
+			r->mult = div64_u64(factor, r->rate_pkts_ps);
+			if (r->mult & (1U << 31) || factor & (1ULL << 63))
+				break;
+			factor <<= 1;
+			r->shift++;  

Aren't there helpers somewhere for the reciprocal divide
pre-calculation?  

Now that you mention it, yes.

Looking over reciprocal_divide() I don't think it a good fit here as it
operates on 32bit values, whereas the packet rate is 64 bit.

Packet rate could be changed to a 32 bit entity if we convince ourselves we
don't want more than 2^32 - 1 packets per second (a plausible position
IMHO) - but that leads us to a secondary issue.

The code above is very similar to an existing (long existing)
byte rate variant of this helper - psched_ratecfg_precompute().
And I do think we want to:
a) Support 64-bit byte rates. Indeed such support seems to have
   been added to support 25G use-cases
b) Calculate byte and packet rates the same way

So I feel less and less that reciprocal_divide() is a good fit.
But perhaps I am mistaken.

In the meantime I will take a look to see if a helper common function can
be made to do (64 bit) reciprocal divides for the packet and byte rate
use-cases.  I.e. the common code in psched_ppscfg_precompute() and
psched_ratecfg_precompute().

No strong feelings, I'll just ask to document the reasoning in the
commit message or the comment above.

Thanks. I'll include some text explaining this when reposting.

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

...

On Mon, Jan 25, 2021 at 04:18:19PM +0100, Simon Horman wrote:

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.

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.

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

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:

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.

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.

quoted

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

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:

quoted

On Mon, Jan 25, 2021 at 04:18:19PM +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.

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.

quoted

quoted

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

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:

quoted

On Mon, Jan 25, 2021 at 04:18:19PM +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.

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.

Hi,

No problem. There were (are?) some issues with netdev mails lately.

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 -

The lower layer need to respect whatever is implemented in the software
data path, but with this approach it is not clear which limit is imposed
first. One needs to check act_police's code for that. With the more
discrete approach (two actions), user is in complete control.

There is also an issue of visibility into how many packets were dropped
due to which limit. With this approach both drops are squashed to the
same counter. In the hardware offload case, I assume this would entail
reading the drop counters of two different policers which might not be
atomic (at least on Spectrum).

it should be no surprise that we plan to provide a hardware offload of this
feature. 

Sure. I assumed this was the intention. With Netronome hardware, would
such an action be translated into two actions / two policers?

It also seems to offer nice code re-use.

Yes, the diff is nice, but I do not think it would be much worse if rate
and bandwidth limiting were made to be mutually exclusive.

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.

quoted

quoted

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