| Message ID | 167577884295.1166983.17250724968371889141.stgit@ebuild.local |
|---|---|
| State | Accepted |
| Commit | 594d1fee5b04c0a219cc87488507b021820020a1 |
| 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 Tue, Feb 07, 2023 at 03:07:24PM +0100, Eelco Chaudron wrote: > With some datapaths, read TC, it takes a bit longer to update the > OpenFlow statistics. Rather than adding an additional delay, try > to read the counters multiple times until we get the desired value. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > Acked-by: Roi Dayan <roid@nvidia.com> Hi Eelco, I'm not sure how much this buys us. I notice that in the following patch the tests are disabled (maybe I am reading that wrong) as they are not reliable. And, FWIIW, in my environment, I couldn't get these tests to pass at all (17 iterations). I'd be happy to set up a Fedora, or other environment, to verify this patch if you think it's worthwhile. Lastly, just to clarify my response to other patches. The tests were for the SW TC datapath.
On 8 Feb 2023, at 17:50, Simon Horman wrote: > On Tue, Feb 07, 2023 at 03:07:24PM +0100, Eelco Chaudron wrote: >> With some datapaths, read TC, it takes a bit longer to update the >> OpenFlow statistics. Rather than adding an additional delay, try >> to read the counters multiple times until we get the desired value. >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> Acked-by: Roi Dayan <roid@nvidia.com> > > Hi Eelco, > > I'm not sure how much this buys us. > I notice that in the following patch the tests are disabled > (maybe I am reading that wrong) as they are not reliable. > And, FWIIW, in my environment, I couldn't get these tests to pass > at all (17 iterations). > > I'd be happy to set up a Fedora, or other environment, to verify > this patch if you think it's worthwhile. I think it’s fine for now. For me, it failed every now and then on 50 runs. I decided to keep them out in the final patch to avoid re-run errors. As I’ve seen that, if it fails in the first run, the changes are high it will fail in the re-run. But I did not find any clue yet why. I have some revalidator patches pending upstreanm, and my plan is to revisit these two errors once those changes are in. > Lastly, just to clarify my response to other patches. > The tests were for the SW TC datapath. Thanks for doing this! Cheers, Eelco
On Thu, Feb 09, 2023 at 09:42:03AM +0100, Eelco Chaudron wrote: > > > On 8 Feb 2023, at 17:50, Simon Horman wrote: > > > On Tue, Feb 07, 2023 at 03:07:24PM +0100, Eelco Chaudron wrote: > >> With some datapaths, read TC, it takes a bit longer to update the > >> OpenFlow statistics. Rather than adding an additional delay, try > >> to read the counters multiple times until we get the desired value. > >> > >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > >> Acked-by: Roi Dayan <roid@nvidia.com> > > > > Hi Eelco, > > > > I'm not sure how much this buys us. > > I notice that in the following patch the tests are disabled > > (maybe I am reading that wrong) as they are not reliable. > > And, FWIIW, in my environment, I couldn't get these tests to pass > > at all (17 iterations). > > > > I'd be happy to set up a Fedora, or other environment, to verify > > this patch if you think it's worthwhile. > > I think it’s fine for now. For me, it failed every now and then on 50 runs. I decided to keep them out in the final patch to avoid re-run errors. As I’ve seen that, if it fails in the first run, the changes are high it will fail in the re-run. But I did not find any clue yet why. > > I have some revalidator patches pending upstreanm, and my plan is to revisit these two errors once those changes are in. Thanks, in that case, feel free to add: Reviewed-by: Simon Horman <simon.horman@corigine.com> > > Lastly, just to clarify my response to other patches. > > The tests were for the SW TC datapath. > > Thanks for doing this! > > Cheers, > > Eelco >
diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 76f1f39a2..2d46e0639 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -1638,7 +1638,6 @@ 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() @@ -1709,9 +1708,8 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | sed -n 's/.*\(n\_bytes=[ n_bytes=242 ]) dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B -AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl -n_bytes=138 -]) +OVS_WAIT_UNTIL_EQUAL([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [dnl +n_bytes=138]) dnl check tunnel pop path, from at_ns0 to at_ns1 dnl This 200-byte packet is simulated on behalf of ns_gre0 @@ -1719,9 +1717,9 @@ ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 packet=02908ca8a149faa dnl After truncation = 100 byte at loopback device p2(4) AT_CHECK([ovs-appctl revalidator/purge], [0]) -AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], [dnl - n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop -]) +OVS_WAIT_UNTIL_EQUAL([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [dnl + n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop]) + dnl SLOW_ACTION: disable datapath truncate support dnl Repeat the test above, but exercise the SLOW_ACTION code path @@ -1746,9 +1744,8 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | sed -n 's/.*\(n\_bytes=[ n_bytes=242 ]) dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B -AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl -n_bytes=138 -]) +OVS_WAIT_UNTIL_EQUAL([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [dnl +n_bytes=138]) dnl check tunnel pop path, from at_ns0 to at_ns1 dnl This 200-byte packet is simulated on behalf of ns_gre0 @@ -1773,7 +1770,6 @@ 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])