* Re: [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing
[not found] <20210129102856.6225-1-simon.horman@netronome.com>
@ 2021-01-29 23:04 ` Cong Wang
2021-01-30 14:57 ` Ido Schimmel
2021-02-01 13:02 ` Simon Horman
[not found] ` <0c47b7d7-dc2b-3422-62ff-92fea8300036@mojatatu.com>
1 sibling, 2 replies; 7+ messages in thread
From: Cong Wang @ 2021-01-29 23:04 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jakub Kicinski, Jamal Hadi Salim, Jiri Pirko,
Linux Kernel Network Developers, oss-drivers, Baowen Zheng
On Fri, Jan 29, 2021 at 2:29 AM Simon Horman <simon.horman@netronome.com> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing
2021-01-29 23:04 ` [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing Cong Wang
@ 2021-01-30 14:57 ` Ido Schimmel
2021-02-01 13:07 ` Simon Horman
2021-02-01 13:02 ` Simon Horman
1 sibling, 1 reply; 7+ messages in thread
From: Ido Schimmel @ 2021-01-30 14:57 UTC (permalink / raw)
To: Cong Wang, simon.horman
Cc: David Miller, Jakub Kicinski, Jamal Hadi Salim, Jiri Pirko,
Linux Kernel Network Developers, oss-drivers, Baowen Zheng
On Fri, Jan 29, 2021 at 03:04:51PM -0800, Cong Wang wrote:
> On Fri, Jan 29, 2021 at 2:29 AM Simon Horman <simon.horman@netronome.com> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing
[not found] ` <0c47b7d7-dc2b-3422-62ff-92fea8300036@mojatatu.com>
@ 2021-02-01 12:33 ` Simon Horman
2021-02-02 13:59 ` Jamal Hadi Salim
0 siblings, 1 reply; 7+ messages in thread
From: Simon Horman @ 2021-02-01 12:33 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko, netdev,
oss-drivers, Baowen Zheng
On Fri, Jan 29, 2021 at 09:30:00AM -0500, Jamal Hadi Salim wrote:
> On 2021-01-29 5:28 a.m., 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.
> >
>
> Ido's comment is important: Why not make packet rate vs byte rate
> mutually exclusive? If someone uses packet rate then you make sure
> they dont interleave with attributes for byte rate and vice-versa.
>
> I dont see featurewise impact - but certainly the extra check
> in the fastpath will likely have a small performance impact
> at high rates. Probably not a big deal (if Eric D. is not looking).
> Side note: this policer (with your addition) is now supporting 3 policer
> algorithms - i wonder if it makes sense to start spliting the different
> policers (which will solve the performance impact).
Sorry, I somehow missed Ido's email until you and he pointed it out
in this thread.
Regarding splitting up the policer action. I think there is some value to
the current setup in terms of code re-use and allowing combinations of
features. But I do agree it would be a conversation worth having at some
point.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing
2021-01-29 23:04 ` [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing Cong Wang
2021-01-30 14:57 ` Ido Schimmel
@ 2021-02-01 13:02 ` Simon Horman
2021-02-02 5:26 ` Cong Wang
1 sibling, 1 reply; 7+ messages in thread
From: Simon Horman @ 2021-02-01 13:02 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Jakub Kicinski, Jamal Hadi Salim, Jiri Pirko,
Linux Kernel Network Developers, oss-drivers, Baowen Zheng
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 <simon.horman@netronome.com> 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()".
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);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing
2021-01-30 14:57 ` Ido Schimmel
@ 2021-02-01 13:07 ` Simon Horman
0 siblings, 0 replies; 7+ messages in thread
From: Simon Horman @ 2021-02-01 13:07 UTC (permalink / raw)
To: Ido Schimmel
Cc: Cong Wang, David Miller, Jakub Kicinski, Jamal Hadi Salim,
Jiri Pirko, Linux Kernel Network Developers, oss-drivers,
Baowen Zheng
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:
> > On Fri, Jan 29, 2021 at 2:29 AM Simon Horman <simon.horman@netronome.com> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing
2021-02-01 13:02 ` Simon Horman
@ 2021-02-02 5:26 ` Cong Wang
0 siblings, 0 replies; 7+ messages in thread
From: Cong Wang @ 2021-02-02 5:26 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jakub Kicinski, Jamal Hadi Salim, Jiri Pirko,
Linux Kernel Network Developers, oss-drivers, Baowen Zheng
On Mon, Feb 1, 2021 at 5:02 AM Simon Horman <simon.horman@netronome.com> wrote:
> Would something like this incremental change your concerns?
Yes of course.
Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing
2021-02-01 12:33 ` Simon Horman
@ 2021-02-02 13:59 ` Jamal Hadi Salim
0 siblings, 0 replies; 7+ messages in thread
From: Jamal Hadi Salim @ 2021-02-02 13:59 UTC (permalink / raw)
To: Simon Horman
Cc: David Miller, Jakub Kicinski, Cong Wang, Jiri Pirko, netdev,
oss-drivers, Baowen Zheng
On 2021-02-01 7:33 a.m., Simon Horman wrote:
> On Fri, Jan 29, 2021 at 09:30:00AM -0500, Jamal Hadi Salim wrote:
>> Ido's comment is important: Why not make packet rate vs byte rate
>> mutually exclusive? If someone uses packet rate then you make sure
>> they dont interleave with attributes for byte rate and vice-versa.
>>
>
> Sorry, I somehow missed Ido's email until you and he pointed it out
> in this thread.
>
This one i think is still important. Potential for misconfig
exists with both on.
The check for exclusivity is rather simple in init().
Also please see if you can add a test in the policer tests in tdc.
> Regarding splitting up the policer action. I think there is some value to
> the current setup in terms of code re-use and allowing combinations of
> features. But I do agree it would be a conversation worth having at some
> point.
Sounds reasonable.
cheers,
jamal
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-02-02 19:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210129102856.6225-1-simon.horman@netronome.com>
2021-01-29 23:04 ` [PATCH net-next v2] net/sched: act_police: add support for packet-per-second policing Cong Wang
2021-01-30 14:57 ` Ido Schimmel
2021-02-01 13:07 ` Simon Horman
2021-02-01 13:02 ` Simon Horman
2021-02-02 5:26 ` Cong Wang
[not found] ` <0c47b7d7-dc2b-3422-62ff-92fea8300036@mojatatu.com>
2021-02-01 12:33 ` Simon Horman
2021-02-02 13:59 ` Jamal Hadi Salim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.