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: > > On Mon, Jan 25, 2021 at 04:18:19PM +0100, Simon Horman wrote: > > > From: Baowen Zheng <baowen.zh...@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. > > > > 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. > > > > 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 <baowen.zh...@corigine.com> > > > Signed-off-by: Simon Horman <simon.hor...@netronome.com> > > > Signed-off-by: Louis Peens <louis.pe...@netronome.com> > > > --- > > > 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.
