diff mbox series

[ovs-dev,v2] test: move check for tc ingress pps support to test script

Message ID 20230224130110.2055221-1-simon.horman@corigine.com
State Accepted
Commit 5f0fdf5e2c2e959048fc8ea8be1a57d518805644
Headers show
Series [ovs-dev,v2] test: move check for tc ingress pps support to test script | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Simon Horman Feb. 24, 2023, 1:01 p.m. UTC 
Move check for tc ingress pps support to from aclocal to test script

This has several problems:

1. Stderror from failing commands is output when executing
   various make targets.

2. There are various failure conditions that lead
   to veth0 and veth1 being created by not cleaned up.

3. The check seems to execute for many make targets.
   And it attempts to temporarily modify system state.
   This seems inappropriate.

4. veth0 and veth1 seem far too generic and could easily
   conflict with other parts of the system.

All these problems are addressed by this patch.

Signed-off-by: Simon Horman <simon.horman@corigine.com>
Reviewed-by: Louis Peens <louis.peens@corigine.com>
---
v2
* As suggested by Ilya, use:
  - ovs_tc_pps rather than veth as prefix for interface names
  - on_exit for cleanup, rather than handling each case
  - dnl rather than '/' for multi-line commands
---
 tests/atlocal.in                 | 11 -----------
 tests/system-offloads-traffic.at | 14 ++++++++++++--
 2 files changed, 12 insertions(+), 13 deletions(-)

Comments

Ilya Maximets Feb. 27, 2023, 3:44 p.m. UTC | #1 
On 2/24/23 14:01, Simon Horman wrote:
> Move check for tc ingress pps support to from aclocal to test script
> 
> This has several problems:
> 
> 1. Stderror from failing commands is output when executing
>    various make targets.
> 
> 2. There are various failure conditions that lead
>    to veth0 and veth1 being created by not cleaned up.
> 
> 3. The check seems to execute for many make targets.
>    And it attempts to temporarily modify system state.
>    This seems inappropriate.
> 
> 4. veth0 and veth1 seem far too generic and could easily
>    conflict with other parts of the system.
> 
> All these problems are addressed by this patch.
> 
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> Reviewed-by: Louis Peens <louis.peens@corigine.com>
> ---
> v2
> * As suggested by Ilya, use:
>   - ovs_tc_pps rather than veth as prefix for interface names
>   - on_exit for cleanup, rather than handling each case
>   - dnl rather than '/' for multi-line commands
> ---
>  tests/atlocal.in                 | 11 -----------
>  tests/system-offloads-traffic.at | 14 ++++++++++++--
>  2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/atlocal.in b/tests/atlocal.in
> index e02248f6f829..859668586299 100644
> --- a/tests/atlocal.in
> +++ b/tests/atlocal.in
> @@ -172,17 +172,6 @@ fi
>  # Set HAVE_TC
>  find_command tc
>  
> -# When HAVE_TC=yes, check if the current tc supports adding pps filter
> -SUPPORT_TC_INGRESS_PPS="no"
> -if test $HAVE_TC="yes"; then
> -    ip link add veth0 type veth peer name veth1
> -    tc qdisc add dev veth0 handle ffff: ingress
> -    if tc filter add dev veth0 parent ffff: u32 match u32 0 0 police pkts_rate 100 pkts_burst 10; then
> -        SUPPORT_TC_INGRESS_PPS="yes"
> -    fi
> -    ip link del veth0
> -fi
> -
>  # Set HAVE_TCPDUMP
>  find_command tcpdump
>  
> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
> index f2bf9c0639aa..1d21da5f196d 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -18,6 +18,16 @@ m4_define([OVS_CHECK_ACTIONS], [
>             [0], [$1])
>  ])
>  
> +m4_define([CHECK_TC_INGRESS_PPS],
> +[
> +    AT_SKIP_IF([test $HAVE_TC = "no"])
> +    AT_CHECK([ip link add ovs_tc_pps0 type veth peer name ovs_tc_pps1 || dnl
> +                  exit 77])

I'd break the line before '||', but that's not very important.

And a capital letter and a period in the subject line might be
good to add just for consistency.

In any case, the change seems correct to me:

Acked-by: Ilya Maximets <i.maximets@ovn.org>

Thanks!
Simon Horman Feb. 28, 2023, 9:51 a.m. UTC | #2 
On Mon, Feb 27, 2023 at 04:44:02PM +0100, Ilya Maximets wrote:
> On 2/24/23 14:01, Simon Horman wrote:
> > Move check for tc ingress pps support to from aclocal to test script
> > 
> > This has several problems:
> > 
> > 1. Stderror from failing commands is output when executing
> >    various make targets.
> > 
> > 2. There are various failure conditions that lead
> >    to veth0 and veth1 being created by not cleaned up.
> > 
> > 3. The check seems to execute for many make targets.
> >    And it attempts to temporarily modify system state.
> >    This seems inappropriate.
> > 
> > 4. veth0 and veth1 seem far too generic and could easily
> >    conflict with other parts of the system.
> > 
> > All these problems are addressed by this patch.
> > 
> > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> > Reviewed-by: Louis Peens <louis.peens@corigine.com>

...

> > diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
> > index f2bf9c0639aa..1d21da5f196d 100644
> > --- a/tests/system-offloads-traffic.at
> > +++ b/tests/system-offloads-traffic.at
> > @@ -18,6 +18,16 @@ m4_define([OVS_CHECK_ACTIONS], [
> >             [0], [$1])
> >  ])
> >  
> > +m4_define([CHECK_TC_INGRESS_PPS],
> > +[
> > +    AT_SKIP_IF([test $HAVE_TC = "no"])
> > +    AT_CHECK([ip link add ovs_tc_pps0 type veth peer name ovs_tc_pps1 || dnl
> > +                  exit 77])
> 
> I'd break the line before '||', but that's not very important.
> 
> And a capital letter and a period in the subject line might be
> good to add just for consistency.
> 
> In any case, the change seems correct to me:
> 
> Acked-by: Ilya Maximets <i.maximets@ovn.org>

Thanks. I fixed up the line-break and patch subject when applying.

* 5f0fdf5e2c2e ("test: Move check for tc ingress pps support to test script.")
  - https://github.com/openvswitch/ovs/commit/5f0fdf5e2c2e

I did not backport this as it doesn't seem to be a fix to me.
But I'm happy to do so if you feel otherwise.
diff mbox series

Patch

diff --git a/tests/atlocal.in b/tests/atlocal.in
index e02248f6f829..859668586299 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -172,17 +172,6 @@  fi
 # Set HAVE_TC
 find_command tc
 
-# When HAVE_TC=yes, check if the current tc supports adding pps filter
-SUPPORT_TC_INGRESS_PPS="no"
-if test $HAVE_TC="yes"; then
-    ip link add veth0 type veth peer name veth1
-    tc qdisc add dev veth0 handle ffff: ingress
-    if tc filter add dev veth0 parent ffff: u32 match u32 0 0 police pkts_rate 100 pkts_burst 10; then
-        SUPPORT_TC_INGRESS_PPS="yes"
-    fi
-    ip link del veth0
-fi
-
 # Set HAVE_TCPDUMP
 find_command tcpdump
 
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index f2bf9c0639aa..1d21da5f196d 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -18,6 +18,16 @@  m4_define([OVS_CHECK_ACTIONS], [
            [0], [$1])
 ])
 
+m4_define([CHECK_TC_INGRESS_PPS],
+[
+    AT_SKIP_IF([test $HAVE_TC = "no"])
+    AT_CHECK([ip link add ovs_tc_pps0 type veth peer name ovs_tc_pps1 || dnl
+                  exit 77])
+    on_exit 'ip link del ovs_tc_pps0'
+    AT_CHECK([tc qdisc add dev ovs_tc_pps0 handle ffff: ingress || exit 77])
+    AT_CHECK([tc filter add dev ovs_tc_pps0 parent ffff: u32 match dnl
+                  u32 0 0 police pkts_rate 100 pkts_burst 10 || exit 77])
+])
 
 AT_SETUP([offloads - ping between two ports - offloads disabled])
 OVS_TRAFFIC_VSWITCHD_START()
@@ -132,7 +142,7 @@  AT_CLEANUP
 
 AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads disabled])
 AT_KEYWORDS([ingress_policing_kpkts])
-AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
+CHECK_TC_INGRESS_PPS()
 OVS_TRAFFIC_VSWITCHD_START()
 AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
 AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
@@ -156,7 +166,7 @@  AT_CLEANUP
 
 AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads enabled])
 AT_KEYWORDS([ingress_policing_kpkts])
-AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
+CHECK_TC_INGRESS_PPS()
 OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . other_config:hw-offload=true])
 AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
 ADD_NAMESPACES(at_ns0)