mbox series

[v3,net-next,0/3] net/sched: act_police: add support for packet-per-second policing

Message ID 20210312140831.23346-1-simon.horman@netronome.com (mailing list archive)
Headers show
Series net/sched: act_police: add support for packet-per-second policing | expand

Message

Simon Horman March 12, 2021, 2:08 p.m. UTC
This series enhances the TC policer action implementation to allow a
policer action instance to enforce a rate-limit based on
packets-per-second, configurable using a packet-per-second rate and burst
parameters.

In the hope of aiding review this is broken up into three patches.

* [PATCH 1/3] flow_offload: add support for packet-per-second policing

  Add support for this feature to the flow_offload API that is used to allow
  programming flows, including TC rules and their actions, into hardware.

* [PATCH 2/3] flow_offload: reject configuration of packet-per-second policing in offload drivers

  Teach all exiting users of the flow_offload API that allow offload of
  policer action instances to reject offload if packet-per-second rate
  limiting is configured: none support it at this time

* [PATCH 3/3] net/sched: act_police: add support for packet-per-second policing

  With the above ground-work in place add the new feature to the TC policer
  action itself

With the above in place the feature may be used.

As follow-ups we plan to provide:
* Corresponding updates to iproute2
* Corresponding self tests (which depend on the iproute2 changes)
* Hardware offload support for the NFP driver

Key changes since v2:
* Added patches 1 and 2, which makes adding patch 3 safe for existing
  hardware offload of the policer action
* Re-worked patch 3 so that a TC policer action instance may be configured
  for packet-per-second or byte-per-second rate limiting, but not both.
* Corrected kdoc usage

Baowen Zheng (2):
  flow_offload: reject configuration of packet-per-second policing in
    offload drivers
  net/sched: act_police: add support for packet-per-second policing

Xingfeng Hu (1):
  flow_offload: add support for packet-per-second policing

 drivers/net/dsa/sja1105/sja1105_flower.c      |  6 ++
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         | 11 ++-
 .../net/ethernet/freescale/enetc/enetc_qos.c  |  5 ++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  4 +
 .../ethernet/mellanox/mlxsw/spectrum_flower.c |  5 ++
 drivers/net/ethernet/mscc/ocelot_flower.c     |  5 ++
 drivers/net/ethernet/mscc/ocelot_net.c        |  6 ++
 .../ethernet/netronome/nfp/flower/qos_conf.c  |  5 ++
 include/net/flow_offload.h                    |  2 +
 include/net/sch_generic.h                     | 14 ++++
 include/net/tc_act/tc_police.h                | 52 +++++++++++++
 include/uapi/linux/pkt_cls.h                  |  2 +
 net/sched/act_police.c                        | 59 +++++++++++++--
 net/sched/cls_api.c                           |  3 +
 net/sched/sch_generic.c                       | 75 ++++++++++++-------
 15 files changed, 221 insertions(+), 33 deletions(-)

Comments

Simon Horman March 12, 2021, 2:22 p.m. UTC | #1
On Fri, Mar 12, 2021 at 03:08:28PM +0100, Simon Horman wrote:
> This series enhances the TC policer action implementation to allow a
> policer action instance to enforce a rate-limit based on
> packets-per-second, configurable using a packet-per-second rate and burst
> parameters.

...

Sorry, I missed CCing a number of interested parties when posting
this patch-set. I've added them to this email.

Ref: https://lore.kernel.org/netdev/20210312140831.23346-1-simon.horman@netronome.com/
patchwork-bot+netdevbpf@kernel.org March 13, 2021, 10:40 p.m. UTC | #2
Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Fri, 12 Mar 2021 15:08:28 +0100 you wrote:
> This series enhances the TC policer action implementation to allow a
> policer action instance to enforce a rate-limit based on
> packets-per-second, configurable using a packet-per-second rate and burst
> parameters.
> 
> In the hope of aiding review this is broken up into three patches.
> 
> [...]

Here is the summary with links:
  - [v3,net-next,1/3] flow_offload: add support for packet-per-second policing
    https://git.kernel.org/netdev/net-next/c/25660156f4cc
  - [v3,net-next,2/3] flow_offload: reject configuration of packet-per-second policing in offload drivers
    https://git.kernel.org/netdev/net-next/c/6a56e19902af
  - [v3,net-next,3/3] net/sched: act_police: add support for packet-per-second policing
    https://git.kernel.org/netdev/net-next/c/2ffe0395288a

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Ido Schimmel March 14, 2021, 8:17 a.m. UTC | #3
On Fri, Mar 12, 2021 at 03:08:28PM +0100, Simon Horman wrote:
> This series enhances the TC policer action implementation to allow a
> policer action instance to enforce a rate-limit based on
> packets-per-second, configurable using a packet-per-second rate and burst
> parameters.
> 
> In the hope of aiding review this is broken up into three patches.
> 
> * [PATCH 1/3] flow_offload: add support for packet-per-second policing
> 
>   Add support for this feature to the flow_offload API that is used to allow
>   programming flows, including TC rules and their actions, into hardware.
> 
> * [PATCH 2/3] flow_offload: reject configuration of packet-per-second policing in offload drivers
> 
>   Teach all exiting users of the flow_offload API that allow offload of
>   policer action instances to reject offload if packet-per-second rate
>   limiting is configured: none support it at this time
> 
> * [PATCH 3/3] net/sched: act_police: add support for packet-per-second policing
> 
>   With the above ground-work in place add the new feature to the TC policer
>   action itself
> 
> With the above in place the feature may be used.
> 
> As follow-ups we plan to provide:
> * Corresponding updates to iproute2
> * Corresponding self tests (which depend on the iproute2 changes)

I was about to ask :)

FYI, there is this selftest:
tools/testing/selftests/net/forwarding/tc_police.sh

Which can be extended to also test packet rate policing

> * Hardware offload support for the NFP driver
> 
> Key changes since v2:
> * Added patches 1 and 2, which makes adding patch 3 safe for existing
>   hardware offload of the policer action
> * Re-worked patch 3 so that a TC policer action instance may be configured
>   for packet-per-second or byte-per-second rate limiting, but not both.
> * Corrected kdoc usage

Thanks!
Simon Horman March 15, 2021, 2:41 p.m. UTC | #4
On Sun, Mar 14, 2021 at 10:17:37AM +0200, Ido Schimmel wrote:
> On Fri, Mar 12, 2021 at 03:08:28PM +0100, Simon Horman wrote:
> > This series enhances the TC policer action implementation to allow a
> > policer action instance to enforce a rate-limit based on
> > packets-per-second, configurable using a packet-per-second rate and burst
> > parameters.
> > 
> > In the hope of aiding review this is broken up into three patches.
> > 
> > * [PATCH 1/3] flow_offload: add support for packet-per-second policing
> > 
> >   Add support for this feature to the flow_offload API that is used to allow
> >   programming flows, including TC rules and their actions, into hardware.
> > 
> > * [PATCH 2/3] flow_offload: reject configuration of packet-per-second policing in offload drivers
> > 
> >   Teach all exiting users of the flow_offload API that allow offload of
> >   policer action instances to reject offload if packet-per-second rate
> >   limiting is configured: none support it at this time
> > 
> > * [PATCH 3/3] net/sched: act_police: add support for packet-per-second policing
> > 
> >   With the above ground-work in place add the new feature to the TC policer
> >   action itself
> > 
> > With the above in place the feature may be used.
> > 
> > As follow-ups we plan to provide:
> > * Corresponding updates to iproute2
> > * Corresponding self tests (which depend on the iproute2 changes)
> 
> I was about to ask :)
> 
> FYI, there is this selftest:
> tools/testing/selftests/net/forwarding/tc_police.sh
> 
> Which can be extended to also test packet rate policing

Thanks Ido,

The approach we have taken is to add tests to
tools/testing/selftests/tc-testing/tc-tests/actions/police.json

Do you think adding a test to tc_police.sh is also worthwhile? Or should be
done instead of updating police.json?

Lastly, my assumption is that the tests should be posted once iproute2
changes they depend on have been accepted. Is this correct in your opinion?

In any case, I'll get moving on posting the iproute2 changes.

> > * Hardware offload support for the NFP driver
> > 
> > Key changes since v2:
> > * Added patches 1 and 2, which makes adding patch 3 safe for existing
> >   hardware offload of the policer action
> > * Re-worked patch 3 so that a TC policer action instance may be configured
> >   for packet-per-second or byte-per-second rate limiting, but not both.
> > * Corrected kdoc usage
> 
> Thanks!
Ido Schimmel March 16, 2021, 8:35 a.m. UTC | #5
Sorry for the delay. Was AFK yesterday

On Mon, Mar 15, 2021 at 03:41:56PM +0100, Simon Horman wrote:
> On Sun, Mar 14, 2021 at 10:17:37AM +0200, Ido Schimmel wrote:
> > On Fri, Mar 12, 2021 at 03:08:28PM +0100, Simon Horman wrote:
> > > This series enhances the TC policer action implementation to allow a
> > > policer action instance to enforce a rate-limit based on
> > > packets-per-second, configurable using a packet-per-second rate and burst
> > > parameters.
> > > 
> > > In the hope of aiding review this is broken up into three patches.
> > > 
> > > * [PATCH 1/3] flow_offload: add support for packet-per-second policing
> > > 
> > >   Add support for this feature to the flow_offload API that is used to allow
> > >   programming flows, including TC rules and their actions, into hardware.
> > > 
> > > * [PATCH 2/3] flow_offload: reject configuration of packet-per-second policing in offload drivers
> > > 
> > >   Teach all exiting users of the flow_offload API that allow offload of
> > >   policer action instances to reject offload if packet-per-second rate
> > >   limiting is configured: none support it at this time
> > > 
> > > * [PATCH 3/3] net/sched: act_police: add support for packet-per-second policing
> > > 
> > >   With the above ground-work in place add the new feature to the TC policer
> > >   action itself
> > > 
> > > With the above in place the feature may be used.
> > > 
> > > As follow-ups we plan to provide:
> > > * Corresponding updates to iproute2
> > > * Corresponding self tests (which depend on the iproute2 changes)
> > 
> > I was about to ask :)
> > 
> > FYI, there is this selftest:
> > tools/testing/selftests/net/forwarding/tc_police.sh
> > 
> > Which can be extended to also test packet rate policing
> 
> Thanks Ido,
> 
> The approach we have taken is to add tests to
> tools/testing/selftests/tc-testing/tc-tests/actions/police.json
> 
> Do you think adding a test to tc_police.sh is also worthwhile? Or should be
> done instead of updating police.json?

IIUC, police.json only performs configuration tests. tc_police.sh on the
other hand, configures a topology, injects traffic and validates that
the bandwidth after the police action is according to user
configuration. You can test the software data path by using veth pairs
or the hardware data path by using physical ports looped to each other.

So I think that extending both tests is worthwhile.

> 
> Lastly, my assumption is that the tests should be posted once iproute2
> changes they depend on have been accepted. Is this correct in your opinion?

Personally, I prefer selftests to be posted together with the
implementation, regardless if they depend on new iproute2 functionality.
In the unlikely case that the kernel patches were accepted, but changes
were requested for the command line interface, you can always patch the
selftests later.

Jakub recently added this section:
https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#how-do-i-post-corresponding-changes-to-user-space-components

He writes "User space code exercising kernel features should be posted
alongside kernel patches."

And you can see that in the example the last patch is a selftest:

```
[PATCH net-next 0/3] net: some feature cover letter
 └─ [PATCH net-next 1/3] net: some feature prep
 └─ [PATCH net-next 2/3] net: some feature do it
 └─ [PATCH net-next 3/3] selftest: net: some feature

[PATCH iproute2-next] ip: add support for some feature
```

> 
> In any case, I'll get moving on posting the iproute2 changes.

Thanks!

> 
> > > * Hardware offload support for the NFP driver
> > > 
> > > Key changes since v2:
> > > * Added patches 1 and 2, which makes adding patch 3 safe for existing
> > >   hardware offload of the policer action
> > > * Re-worked patch 3 so that a TC policer action instance may be configured
> > >   for packet-per-second or byte-per-second rate limiting, but not both.
> > > * Corrected kdoc usage
> > 
> > Thanks!
Simon Horman March 16, 2021, 10:42 a.m. UTC | #6
Hi Ido,

On Tue, Mar 16, 2021 at 10:35:35AM +0200, Ido Schimmel wrote:
> Sorry for the delay. Was AFK yesterday

No problem at all.

...

> > > > As follow-ups we plan to provide:
> > > > * Corresponding updates to iproute2
> > > > * Corresponding self tests (which depend on the iproute2 changes)
> > > 
> > > I was about to ask :)
> > > 
> > > FYI, there is this selftest:
> > > tools/testing/selftests/net/forwarding/tc_police.sh
> > > 
> > > Which can be extended to also test packet rate policing
> > 
> > Thanks Ido,
> > 
> > The approach we have taken is to add tests to
> > tools/testing/selftests/tc-testing/tc-tests/actions/police.json
> > 
> > Do you think adding a test to tc_police.sh is also worthwhile? Or should be
> > done instead of updating police.json?
> 
> IIUC, police.json only performs configuration tests. tc_police.sh on the
> other hand, configures a topology, injects traffic and validates that
> the bandwidth after the police action is according to user
> configuration. You can test the software data path by using veth pairs
> or the hardware data path by using physical ports looped to each other.
> 
> So I think that extending both tests is worthwhile.

Thanks, we'll see about making it so.

> > Lastly, my assumption is that the tests should be posted once iproute2
> > changes they depend on have been accepted. Is this correct in your opinion?
> 
> Personally, I prefer selftests to be posted together with the
> implementation, regardless if they depend on new iproute2 functionality.
> In the unlikely case that the kernel patches were accepted, but changes
> were requested for the command line interface, you can always patch the
> selftests later.
> 
> Jakub recently added this section:
> https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#how-do-i-post-corresponding-changes-to-user-space-components
> 
> He writes "User space code exercising kernel features should be posted
> alongside kernel patches."
> 
> And you can see that in the example the last patch is a selftest:
> 
> ```
> [PATCH net-next 0/3] net: some feature cover letter
>  └─ [PATCH net-next 1/3] net: some feature prep
>  └─ [PATCH net-next 2/3] net: some feature do it
>  └─ [PATCH net-next 3/3] selftest: net: some feature
> 
> [PATCH iproute2-next] ip: add support for some feature
> ```

Thanks, we'll try to follow this in our next feature submission.

...
Jamal Hadi Salim March 16, 2021, 1:53 p.m. UTC | #7
On 2021-03-12 9:22 a.m., Simon Horman wrote:
> On Fri, Mar 12, 2021 at 03:08:28PM +0100, Simon Horman wrote:
>> This series enhances the TC policer action implementation to allow a
>> policer action instance to enforce a rate-limit based on
>> packets-per-second, configurable using a packet-per-second rate and burst
>> parameters.
> 
> ...
> 
> Sorry, I missed CCing a number of interested parties when posting
> this patch-set. I've added them to this email.
> 
> Ref: https://lore.kernel.org/netdev/20210312140831.23346-1-simon.horman@netronome.com/
> 

For 2/3:
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

If you submit one or two testcases to tools/testing/selftests/tc-testing
you'll be both a hero and someone committing an act of kindness.

cheers,
jamal
Simon Horman March 16, 2021, 4:40 p.m. UTC | #8
On Tue, Mar 16, 2021 at 09:53:49AM -0400, Jamal Hadi Salim wrote:
> On 2021-03-12 9:22 a.m., Simon Horman wrote:
> > On Fri, Mar 12, 2021 at 03:08:28PM +0100, Simon Horman wrote:
> > > This series enhances the TC policer action implementation to allow a
> > > policer action instance to enforce a rate-limit based on
> > > packets-per-second, configurable using a packet-per-second rate and burst
> > > parameters.
> > 
> > ...
> > 
> > Sorry, I missed CCing a number of interested parties when posting
> > this patch-set. I've added them to this email.
> > 
> > Ref: https://lore.kernel.org/netdev/20210312140831.23346-1-simon.horman@netronome.com/
> > 
> 
> For 2/3:
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> If you submit one or two testcases to tools/testing/selftests/tc-testing
> you'll be both a hero and someone committing an act of kindness.

Thanks Jamal,

my team are heroes and are working on testcases.