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

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.