On Thu, Feb 23, 2023 at 03:19:37PM +0100, 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 <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?
Yes, good idea. Actually, I had noticed that too. But forgot to do anything about it. > > + 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? Yes, I thought you might say that. It is certainly the ugliest part of this patch. I was concerned that the cleanup would happen to late and the veth0/veth1 scheme may conflict. But if we move to a different scheme, as you suggested above, then on_exit may work. I will give it a try. > > > + 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
