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

On Fri, Jan 29, 2021 at 2:29 AM Simon Horman [off-list ref] wrote:

+/**
+ * psched_ratecfg_precompute__() - Pre-compute values for reciprocal division
+ * @rate:   Rate to compute reciprocal division values of
+ * @mult:   Multiplier for reciprocal division
+ * @shift:  Shift for reciprocal division
+ *
+ * The multiplier and shift for reciprocal division by rate are stored
+ * in mult and shift.
+ *
+ * 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 * len) / rate_bps
+ *
+ * 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.
+ *
+ * reciprocal_value() is not used here it doesn't handle 64-bit values.
+ */
+static void psched_ratecfg_precompute__(u64 rate, u32 *mult, u8 *shift)

Am I the only one who thinks "foo__()" is an odd name? We usually use
"__foo()" for helper or unlocked version of "foo()".

And, you probably want to move the function doc to its exported variant
instead, right?

Thanks.

On Fri, Jan 29, 2021 at 03:04:51PM -0800, Cong Wang wrote:

On Fri, Jan 29, 2021 at 2:29 AM Simon Horman [off-list ref] wrote:

I didn't get v2 (didn't made it to the list), but I did leave feedback
on v1 [1]. Not sure if you got it or not given the recent issues.

[1] https://lore.kernel.org/netdev/20210128161933.GA3285394@shredder.lan/#t

Hi Cong,

On Fri, Jan 29, 2021 at 03:04:51PM -0800, Cong Wang wrote:

On Fri, Jan 29, 2021 at 2:29 AM Simon Horman [off-list ref] wrote:

quoted

+/**
+ * psched_ratecfg_precompute__() - Pre-compute values for reciprocal division
+ * @rate:   Rate to compute reciprocal division values of
+ * @mult:   Multiplier for reciprocal division
+ * @shift:  Shift for reciprocal division
+ *
+ * The multiplier and shift for reciprocal division by rate are stored
+ * in mult and shift.
+ *
+ * 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 * len) / rate_bps
+ *
+ * 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.
+ *
+ * reciprocal_value() is not used here it doesn't handle 64-bit values.
+ */
+static void psched_ratecfg_precompute__(u64 rate, u32 *mult, u8 *shift)

Am I the only one who thinks "foo__()" is an odd name? We usually use
"__foo()" for helper or unlocked version of "foo()".

Sorry, I will fix that.

And, you probably want to move the function doc to its exported variant
instead, right?

Would something like this incremental change your concerns?

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 44991ea726fc..63cb69df4240 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c

@@ -1325,14 +1325,7 @@ void dev_shutdown(struct net_device *dev)
 	WARN_ON(timer_pending(&dev->watchdog_timer));
 }
 
-/**
- * psched_ratecfg_precompute__() - Pre-compute values for reciprocal division
- * @rate:   Rate to compute reciprocal division values of
- * @mult:   Multiplier for reciprocal division
- * @shift:  Shift for reciprocal division
- *
- * The multiplier and shift for reciprocal division by rate are stored
- * in mult and shift.
+/* Pre-compute values for reciprocol division for a rate limit.
  *
  * The deal here is to replace a divide by a reciprocal one
  * in fast path (a reciprocal divide is a multiply and a shift)

@@ -1348,7 +1341,7 @@ void dev_shutdown(struct net_device *dev)
  *
  * reciprocal_value() is not used here it doesn't handle 64-bit values.
  */
-static void psched_ratecfg_precompute__(u64 rate, u32 *mult, u8 *shift)
+static void __psched_ratecfg_precompute(u64 rate, u32 *mult, u8 *shift)
 {
 	u64 factor = NSEC_PER_SEC;
 

@@ -1367,6 +1360,15 @@ static void psched_ratecfg_precompute__(u64 rate, u32 *mult, u8 *shift)
 	}
 }
 
+/**
+ * psched_ratecfg_precompute() - Pre-compute values for byte rate limiting
+ * @r:      Byte-per-second rate struct to store configuration in
+ * @conf:   Rate specification
+ * @rate64: Rate in bytes-per-second
+ *
+ * Pre-compute configuration, including values for reciprocal division,
+ * for a byte-per-second rate limit.
+ */
 void psched_ratecfg_precompute(struct psched_ratecfg *r,
 			       const struct tc_ratespec *conf,
 			       u64 rate64)

@@ -1375,14 +1377,22 @@ void psched_ratecfg_precompute(struct psched_ratecfg *r,
 	r->overhead = conf->overhead;
 	r->rate_bytes_ps = max_t(u64, conf->rate, rate64);
 	r->linklayer = (conf->linklayer & TC_LINKLAYER_MASK);
-	psched_ratecfg_precompute__(r->rate_bytes_ps, &r->mult, &r->shift);
+	__psched_ratecfg_precompute(r->rate_bytes_ps, &r->mult, &r->shift);
 }
 EXPORT_SYMBOL(psched_ratecfg_precompute);
 
+/**
+ * psched_ppscfg_precompute() - Pre-compute values for packet rate limiting
+ * @r:         Packet-per-second rate struct to store configuration in
+ * @pktrate64: Rate in packets-per-second
+ *
+ * Pre-compute configuration, including values for reciprocal division,
+ * for a packet-per-second rate limit.
+ */
 void psched_ppscfg_precompute(struct psched_pktrate *r, u64 pktrate64)
 {
 	r->rate_pkts_ps = pktrate64;
-	psched_ratecfg_precompute__(r->rate_pkts_ps, &r->mult, &r->shift);
+	__psched_ratecfg_precompute(r->rate_pkts_ps, &r->mult, &r->shift);
 }
 EXPORT_SYMBOL(psched_ppscfg_precompute);
 

On Sat, Jan 30, 2021 at 04:57:38PM +0200, Ido Schimmel wrote:

On Fri, Jan 29, 2021 at 03:04:51PM -0800, Cong Wang wrote:

quoted

On Fri, Jan 29, 2021 at 2:29 AM Simon Horman [off-list ref] wrote:

I didn't get v2 (didn't made it to the list), but I did leave feedback
on v1 [1]. Not sure if you got it or not given the recent issues.

[1] https://lore.kernel.org/netdev/20210128161933.GA3285394@shredder.lan/#t

Sorry, I had missed that.
I have now responded in-thread.

On Mon, Feb 1, 2021 at 5:02 AM Simon Horman [off-list ref] wrote:

Would something like this incremental change your concerns?

Yes of course.

Thanks!