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