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

[Ido Schimmel <idosch@idosch.org>]

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!