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 |
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 |
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!
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 --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)