* 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 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-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-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
[parent not found: <0c47b7d7-dc2b-3422-62ff-92fea8300036@mojatatu.com>]
* 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-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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).