| Message ID | 167533618630.904861.5096323296221106231.stgit@ebuild.local |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tests: Add system-traffic.at tests to check-offloads. | 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 Thu, Feb 02, 2023 at 12:09:47PM +0100, Eelco Chaudron wrote: > Include and run the system-traffic.at tests as part of the system offload > testsuite. Exclude all the tests that will not run without any special > modifications. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > Acked-by: Roi Dayan <roid@nvidia.com> ... > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > index 676d55aa9..690331f35 100644 > --- a/tests/ofproto-macros.at > +++ b/tests/ofproto-macros.at > @@ -195,6 +195,7 @@ m4_define([_OVS_VSWITCHD_START], > /netdev_offload|INFO|netdev: Flow API Enabled/d > /probe tc:/d > /setting extended ack support failed/d > +/recirc_id sharing not supported/d > /tc: Using policy/d']]) > ]) Hi Eelco, I gather the above is to address a problem I reported with an older kernel, which indeed doesn't support recirc_id sharing not supported. I am, to be honest, not entirely sure of the wisdom in me, or anyone else, running this code in such an environment. But perhaps this particular problem is harmless enough to fix. However, I don't think the above fixes the problem. This is because the false-positive on the logs is triggered not when starting OVS, but rather when stopping it. The following, somewhat global, change did address the problem for me. A more local version might bean override of OVS_VSWITCHD_STOP for system-offloads-traffic. diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 690331f3561a..567cb281c75d 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -195,7 +195,6 @@ m4_define([_OVS_VSWITCHD_START], /netdev_offload|INFO|netdev: Flow API Enabled/d /probe tc:/d /setting extended ack support failed/d -/recirc_id sharing not supported/d /tc: Using policy/d']]) ]) @@ -335,7 +334,7 @@ m4_divert_pop([PREPARE_TESTS]) # # OVS_VSWITCHD_STOP(["/expected error/d"]) m4_define([OVS_VSWITCHD_STOP], - [AT_CHECK([check_logs $1]) + [AT_CHECK([check_logs $1";/recirc_id sharing not supported/d"]) OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) OVS_APP_EXIT_AND_WAIT([ovsdb-server])]) ...
On 2/3/23 17:48, Simon Horman wrote: > On Thu, Feb 02, 2023 at 12:09:47PM +0100, Eelco Chaudron wrote: >> Include and run the system-traffic.at tests as part of the system offload >> testsuite. Exclude all the tests that will not run without any special >> modifications. >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> Acked-by: Roi Dayan <roid@nvidia.com> > > ... > >> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >> index 676d55aa9..690331f35 100644 >> --- a/tests/ofproto-macros.at >> +++ b/tests/ofproto-macros.at >> @@ -195,6 +195,7 @@ m4_define([_OVS_VSWITCHD_START], >> /netdev_offload|INFO|netdev: Flow API Enabled/d >> /probe tc:/d >> /setting extended ack support failed/d >> +/recirc_id sharing not supported/d >> /tc: Using policy/d']]) >> ]) > > Hi Eelco, > > I gather the above is to address a problem I reported with an older kernel, > which indeed doesn't support recirc_id sharing not supported. > > I am, to be honest, not entirely sure of the wisdom in me, or anyone else, > running this code in such an environment. But perhaps this particular > problem is harmless enough to fix. > > However, I don't think the above fixes the problem. > This is because the false-positive on the logs is triggered not when > starting OVS, but rather when stopping it. > > The following, somewhat global, change did address the problem for me. > A more local version might bean override of OVS_VSWITCHD_STOP for > system-offloads-traffic. Another option might be to reduce the log level from ERR to something more reasonable. There is really no reason to report unsupported feature as an error. If we'll demote it down to INFO level or even DBG, check_logs will stop complaining. What do you think? Best regards, Ilya Maximets. > > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > index 690331f3561a..567cb281c75d 100644 > --- a/tests/ofproto-macros.at > +++ b/tests/ofproto-macros.at > @@ -195,7 +195,6 @@ m4_define([_OVS_VSWITCHD_START], > /netdev_offload|INFO|netdev: Flow API Enabled/d > /probe tc:/d > /setting extended ack support failed/d > -/recirc_id sharing not supported/d > /tc: Using policy/d']]) > ]) > > @@ -335,7 +334,7 @@ m4_divert_pop([PREPARE_TESTS]) > # > # OVS_VSWITCHD_STOP(["/expected error/d"]) > m4_define([OVS_VSWITCHD_STOP], > - [AT_CHECK([check_logs $1]) > + [AT_CHECK([check_logs $1";/recirc_id sharing not supported/d"]) > OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) > OVS_APP_EXIT_AND_WAIT([ovsdb-server])]) > > > ...
On Fri, Feb 03, 2023 at 08:36:12PM +0100, Ilya Maximets wrote: > On 2/3/23 17:48, Simon Horman wrote: > > On Thu, Feb 02, 2023 at 12:09:47PM +0100, Eelco Chaudron wrote: > >> Include and run the system-traffic.at tests as part of the system offload > >> testsuite. Exclude all the tests that will not run without any special > >> modifications. > >> > >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > >> Acked-by: Roi Dayan <roid@nvidia.com> > > > > ... > > > >> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > >> index 676d55aa9..690331f35 100644 > >> --- a/tests/ofproto-macros.at > >> +++ b/tests/ofproto-macros.at > >> @@ -195,6 +195,7 @@ m4_define([_OVS_VSWITCHD_START], > >> /netdev_offload|INFO|netdev: Flow API Enabled/d > >> /probe tc:/d > >> /setting extended ack support failed/d > >> +/recirc_id sharing not supported/d > >> /tc: Using policy/d']]) > >> ]) > > > > Hi Eelco, > > > > I gather the above is to address a problem I reported with an older kernel, > > which indeed doesn't support recirc_id sharing not supported. > > > > I am, to be honest, not entirely sure of the wisdom in me, or anyone else, > > running this code in such an environment. But perhaps this particular > > problem is harmless enough to fix. > > > > However, I don't think the above fixes the problem. > > This is because the false-positive on the logs is triggered not when > > starting OVS, but rather when stopping it. > > > > The following, somewhat global, change did address the problem for me. > > A more local version might bean override of OVS_VSWITCHD_STOP for > > system-offloads-traffic. > > Another option might be to reduce the log level from ERR to something > more reasonable. There is really no reason to report unsupported > feature as an error. If we'll demote it down to INFO level or even DBG, > check_logs will stop complaining. > > What do you think? Sounds good to me.
On 4 Feb 2023, at 9:11, Simon Horman wrote: > On Fri, Feb 03, 2023 at 08:36:12PM +0100, Ilya Maximets wrote: >> On 2/3/23 17:48, Simon Horman wrote: >>> On Thu, Feb 02, 2023 at 12:09:47PM +0100, Eelco Chaudron wrote: >>>> Include and run the system-traffic.at tests as part of the system offload >>>> testsuite. Exclude all the tests that will not run without any special >>>> modifications. >>>> >>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>>> Acked-by: Roi Dayan <roid@nvidia.com> >>> >>> ... >>> >>>> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >>>> index 676d55aa9..690331f35 100644 >>>> --- a/tests/ofproto-macros.at >>>> +++ b/tests/ofproto-macros.at >>>> @@ -195,6 +195,7 @@ m4_define([_OVS_VSWITCHD_START], >>>> /netdev_offload|INFO|netdev: Flow API Enabled/d >>>> /probe tc:/d >>>> /setting extended ack support failed/d >>>> +/recirc_id sharing not supported/d >>>> /tc: Using policy/d']]) >>>> ]) >>> >>> Hi Eelco, >>> >>> I gather the above is to address a problem I reported with an older kernel, >>> which indeed doesn't support recirc_id sharing not supported. >>> >>> I am, to be honest, not entirely sure of the wisdom in me, or anyone else, >>> running this code in such an environment. But perhaps this particular >>> problem is harmless enough to fix. >>> >>> However, I don't think the above fixes the problem. >>> This is because the false-positive on the logs is triggered not when >>> starting OVS, but rather when stopping it. >>> >>> The following, somewhat global, change did address the problem for me. >>> A more local version might bean override of OVS_VSWITCHD_STOP for >>> system-offloads-traffic. >> >> Another option might be to reduce the log level from ERR to something >> more reasonable. There is really no reason to report unsupported >> feature as an error. If we'll demote it down to INFO level or even DBG, >> check_logs will stop complaining. >> >> What do you think? > > Sounds good to me. Sounds good to me, as all other messages for EOPNOTSUPP are of debug level, I change this one also. Running a final test run right now, forcing no support for recirc_id sharing, and will send a v11 if it works fine. //Eelco
diff --git a/tests/automake.mk b/tests/automake.mk index c8de3fe28..86e496a5b 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -186,7 +186,8 @@ SYSTEM_TESTSUITE_AT = \ SYSTEM_OFFLOADS_TESTSUITE_AT = \ tests/system-common-macros.at \ tests/system-offloads-traffic.at \ - tests/system-offloads-testsuite.at + tests/system-offloads-testsuite.at \ + tests/system-offloads-testsuite-macros.at SYSTEM_DPDK_TESTSUITE_AT = \ tests/system-common-macros.at \ diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 676d55aa9..690331f35 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -195,6 +195,7 @@ m4_define([_OVS_VSWITCHD_START], /netdev_offload|INFO|netdev: Flow API Enabled/d /probe tc:/d /setting extended ack support failed/d +/recirc_id sharing not supported/d /tc: Using policy/d']]) ]) diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at index 11920e60b..822a80618 100644 --- a/tests/system-kmod-macros.at +++ b/tests/system-kmod-macros.at @@ -224,3 +224,8 @@ m4_define([VSCTL_ADD_DATAPATH_TABLE], # or necessary for the userspace datapath as it is checking for a kernel # specific regression. m4_define([CHECK_L3L4_CONNTRACK_REASM]) + +# CHECK_NO_TC_OFFLOAD +# +# The kernel module tests do not use TC offload. +m4_define([CHECK_NO_TC_OFFLOAD]) diff --git a/tests/system-offloads-testsuite-macros.at b/tests/system-offloads-testsuite-macros.at new file mode 100644 index 000000000..2129cf7f0 --- /dev/null +++ b/tests/system-offloads-testsuite-macros.at @@ -0,0 +1,36 @@ +AT_COPYRIGHT([Copyright (c) 2022 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at: + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License.]) + +# The goal is to run as many as possible of the system-traffic tests with +# OVS tc offload enabled. We do this by overriding the +# OVS_TRAFFIC_VSWITCHD_START() with offloading enabled. +m4_define([OVS_TRAFFIC_VSWITCHD_START], + [AT_CHECK([modprobe openvswitch]) + on_exit 'modprobe -r openvswitch' + m4_foreach([mod], [[vport_geneve], [vport_gre], [vport_lisp], [vport_stt], [vport_vxlan]], + [modprobe -q mod || echo "Module mod not loaded." + on_exit 'modprobe -q -r mod' + ]) + on_exit 'ovs-dpctl del-dp ovs-system' + on_exit 'ovs-appctl dpctl/flush-conntrack' + _OVS_VSWITCHD_START([], [-- set Open_vSwitch . other_config:hw-offload=true $3]) + dnl Add bridges, ports, etc. + AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2]) +]) + +# Macro to exclude tests that will fail with TC offload enabled. +m4_define([CHECK_NO_TC_OFFLOAD], +[ + AT_SKIP_IF([:]) +]) diff --git a/tests/system-offloads-testsuite.at b/tests/system-offloads-testsuite.at index eb5d2d4b3..23637d4f5 100644 --- a/tests/system-offloads-testsuite.at +++ b/tests/system-offloads-testsuite.at @@ -23,3 +23,6 @@ m4_include([tests/system-common-macros.at]) m4_include([tests/system-kmod-macros.at]) m4_include([tests/system-offloads-traffic.at]) + +m4_include([tests/system-offloads-testsuite-macros.at]) +m4_include([tests/system-traffic.at]) diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 470c3f640..c84fecce3 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -1199,6 +1199,7 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([datapath - mpls actions]) +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) ADD_NAMESPACES(at_ns0, at_ns1) @@ -1236,6 +1237,7 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([datapath - multiple mpls label pop]) +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])]) ADD_NAMESPACES(at_ns0, at_ns1) @@ -1638,6 +1640,7 @@ dnl br-underlay: with IP: 172.31.1.100 dnl ns0: connect to br-underlay, with IP: 10.1.1.1 AT_SETUP([datapath - truncate and output to gre tunnel by simulated packets]) OVS_CHECK_MIN_KERNEL(3, 10) +CHECK_NO_TC_OFFLOAD() AT_SKIP_IF([test $HAVE_NC = no]) OVS_TRAFFIC_VSWITCHD_START() @@ -1772,6 +1775,7 @@ AT_SETUP([datapath - truncate and output to gre tunnel]) AT_SKIP_IF([test $HAVE_NC = no]) OVS_CHECK_KERNEL_EXCL(3, 10, 4, 15) OVS_CHECK_GRE() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_BR([br-underlay]) @@ -2856,6 +2860,7 @@ AT_CLEANUP AT_SETUP([conntrack - zones from other field, more tests]) CHECK_CONNTRACK() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(at_ns0, at_ns1) @@ -2987,6 +2992,7 @@ AT_CLEANUP AT_SETUP([conntrack - multiple namespaces, internal ports]) CHECK_CONNTRACK() CHECK_CONNTRACK_LOCAL_STACK() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START( [set-fail-mode br0 secure -- ]) @@ -3609,6 +3615,7 @@ AT_CLEANUP AT_SETUP([conntrack - IPv4 fragmentation + cvlan]) CHECK_CONNTRACK() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START([set Open_vSwitch . other_config:vlan-limit=0]) OVS_CHECK_8021AD() @@ -4132,6 +4139,7 @@ AT_SETUP([conntrack - Fragmentation over vxlan]) OVS_CHECK_VXLAN() CHECK_CONNTRACK() CHECK_CONNTRACK_LOCAL_STACK() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_BR([br-underlay]) @@ -4322,6 +4330,7 @@ AT_CLEANUP AT_SETUP([conntrack - zone-based timeout policy]) CHECK_CONNTRACK() CHECK_CONNTRACK_TIMEOUT() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(at_ns0, at_ns1) @@ -4815,6 +4824,7 @@ OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP AT_SETUP([conntrack - FTP]) +CHECK_NO_TC_OFFLOAD() AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_ALG() @@ -4924,6 +4934,7 @@ AT_SETUP([conntrack - FTP over IPv6]) AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(at_ns0, at_ns1) @@ -4979,6 +4990,7 @@ AT_SETUP([conntrack - IPv6 FTP Passive]) AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(at_ns0, at_ns1) @@ -5038,6 +5050,7 @@ AT_SETUP([conntrack - FTP with multiple expectations]) AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(at_ns0, at_ns1) @@ -5104,6 +5117,7 @@ AT_SETUP([conntrack - TFTP]) AT_SKIP_IF([test $HAVE_TFTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(at_ns0, at_ns1) @@ -5739,6 +5753,7 @@ m4_define([CHECK_FTP_NAT], CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() + CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() @@ -6046,6 +6061,7 @@ AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() @@ -6106,6 +6122,7 @@ AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() @@ -6166,6 +6183,7 @@ AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() @@ -6226,6 +6244,7 @@ AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() @@ -6286,6 +6305,7 @@ AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() @@ -6488,6 +6508,7 @@ AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() @@ -6548,6 +6569,7 @@ AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() @@ -6609,6 +6631,7 @@ AT_SKIP_IF([test $HAVE_FTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(at_ns0, at_ns1) @@ -6669,6 +6692,7 @@ AT_SKIP_IF([test $HAVE_TFTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() @@ -7043,6 +7067,7 @@ AT_SETUP([conntrack - Multiple ICMP traverse]) dnl This tracks sending ICMP packets via conntrack multiple times for the dnl same packet CHECK_CONNTRACK() +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() OVS_CHECK_CT_CLEAR() @@ -7085,6 +7110,7 @@ AT_CLEANUP AT_SETUP([conntrack - can match and clear ct_state from outside OVS]) CHECK_CONNTRACK_LOCAL_STACK() +CHECK_NO_TC_OFFLOAD() OVS_CHECK_TUNNEL_TSO() OVS_CHECK_GENEVE() @@ -7133,6 +7159,7 @@ AT_CLEANUP AT_BANNER([IGMP]) AT_SETUP([IGMP - flood under normal action]) +CHECK_NO_TC_OFFLOAD() OVS_TRAFFIC_VSWITCHD_START() ADD_NAMESPACES(at_ns0, at_ns1) diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at index b34a84775..610fa2e94 100644 --- a/tests/system-userspace-macros.at +++ b/tests/system-userspace-macros.at @@ -325,3 +325,8 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM], [ AT_SKIP_IF([:]) ]) + +# CHECK_NO_TC_OFFLOAD +# +# Userspace tests do not use TC offload. +m4_define([CHECK_NO_TC_OFFLOAD])