[ovs-dev] [PATCH] vswitchd: Ingress policing to use matchall instead of basic

[Ilya Maximets]

On 10/14/21 21:44, Michael Pattrick wrote:
> Currently ingress policing uses the basic classifier to apply traffic
> control filters.

Hi, Michael.  Thanks for submitting a patch!
I'm not going to review technical details of implementation, I'll leave
this to Simon and others, but I want to make a few high level comments.

First, please increase the version number for a patch while sending new
versions, this can be done by adding --subject-prefix='PATCH v2' or simply
-v2 to the list of 'git format-patch' arguments.  Otherwise it's confusing
for reviewers and any automated patch tracking systems we have.

And the patch name should start with 'netdev-linux:', not 'vswitchd:', as
'vswitchd' is way too broad area, and this patch is only really touching
netdev-linux part.

More comments below.

> 
> However, cls_basic was removed from the upcoming RHEL9.

This itself doesn't sound like a correct justification, as RHEL is not
the only distribution in a world.  Does other distributions getting rid
of cls_basic too or was it deprecated in upstream linux kernel?

> Basic is probably
> not the proper classifier to use when the more accurage matchall

What does "not the proper classifier" mean?  I think, the difference
between classifiers should be better described in a commit message.

> classifier is available and already in use in the code base. Switching
> from basic to matchall allows us to remove a function.

Even though 'matchall' is used in a codebase, it's used only if HW offloading
is enabled.  HW offloading itself is a relatively high requirement, because
it requires more recent kernel versions in most cases.  'matchall' is
available starting from kernel v4.8, while 'basic' goes back to 2.16.12.
It's fair to expect that users of HW offloading will use more recent kernel
than 4.8.  However, 4.4 is still a supported longterm kernel version and
switching to 'matchall' for everyone means dropping support for policing
in OVS for currently supported upstream kernels older than 4.8.
Can we detect the support of 'matchall' and fall back if not available?

Not sure how important this is, but that is something that definitely
needs to be discussed and documented.

> 
> I've also modified tests to detect when ingress policing fails to apply.
> 
> Signed-off-by: Michael Pattrick <mkp at redhat.com>
> ---
>  lib/netdev-linux.c | 85 +++++++++-------------------------------------
>  tests/ovs-vsctl.at | 24 ++++++-------
>  2 files changed, 28 insertions(+), 81 deletions(-)
> 

<snip>

> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index d6cd2c084..193bfc5c7 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -1670,22 +1670,22 @@ AT_BANNER([set ingress policing test])
>  
>  AT_SETUP([set ingress_policing_rate and ingress_policing_burst])
>  AT_KEYWORDS([ingress_policing])
> -OVS_VSCTL_SETUP
> +AT_CHECK([ip link add a1 type veth])
> +AT_CHECK([ip link set a1 up])
> +on_exit 'ip link del a1'

These tests are part of "unit" tests.  They should not touch system
interfaces, hence no 'ip' commands should be here.  Also, these tests
should be platform-independent, e.g. they should run on Windows and
DSB systems too.  Moreover, these tests are most of the time not started
from a root user, so they will just fail.  And they do fail all our CI
runs because of that, you can see in the ovsrobot's reports on a patchwork:
  https://patchwork.ozlabs.org/project/openvswitch/patch/160bc3cabaf546fb15bb5172b49c5956f6f1e744.camel@redhat.com/

Another thing is that you're not changing tests/system-offload-traffic.at.
So these tests will fail as they look for configured 'basic' policer.
You may run these tests with 'make check-offloads'.

Best regards, Ilya Maximets.