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 <simon.hor...@corigine.com> > Reviewed-by: Louis Peens <louis.pe...@corigine.com> > --- > 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]) 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
