On 2/23/23 15:19, Ilya Maximets wrote: > On 2/23/23 14:02, 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 inappopriate. >> >> All these problems are addressed by this patch. > > Thanks, Simon! These are indeed very annoying problems. > > I didn't test, but I have a few minor comment inline. > > Best regards, Ilya Maximets. > >> >> Signed-off-by: Simon Horman <[email protected]> >> Reviewed-by: Louis Peens <[email protected]> >> --- >> tests/atlocal.in | 11 ----------- >> tests/system-offloads-traffic.at | 23 +++++++++++++++++++++-- >> 2 files changed, 21 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..ff3e4c63d127 100644 >> --- a/tests/system-offloads-traffic.at >> +++ b/tests/system-offloads-traffic.at >> @@ -18,6 +18,25 @@ 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 veth0 type veth peer name veth1 || exit 77]) > > Since we're here, we should probably re-name these interfaces to > something more specific. I can imagine a scenario where veth0 > port already exists in the system. > Maybe s/veth/ovs_tc_pps/ or something like that? > >> + AT_CHECK([ >> + if ! tc qdisc add dev veth0 handle ffff: ingress; then >> + ip link del veth0 > > Instead of deleting in every branch, we might add on_exit call somewhere > at the beginning of this function. The port will stick around till the > end of the test, but I'm not sure if that's a problem if the name is > special enough. Or we may keep one extra del at the very end. > > With on_exit, can probably be transformed into something like (untested): > > AT_CHECK([tc qdisc add dev veth0 handle ffff: ingress || exit 77])
Just AT_SKIP_IF([! tc qdisc add dev veth0 handle ffff: ingress]) should work, I guess. > > What do you think? > >> + exit 77 >> + fi >> + ]) >> + AT_CHECK([ >> + if ! tc filter add dev veth0 parent ffff: \ > > 'dnl' is prefered for the line break. autotest refuses to print out > commands with '\' in them. > >> + u32 match u32 0 0 police pkts_rate 100 pkts_burst 10; then >> + ip link del veth0 >> + exit 77 >> + fi >> + ]) >> + ip link del veth0 >> +]) >> >> AT_SETUP([offloads - ping between two ports - offloads disabled]) >> OVS_TRAFFIC_VSWITCHD_START() >> @@ -132,7 +151,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 +175,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) > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
