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 <[email protected]> > --- > 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/[email protected]/ 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
