On 5/15/24 12:12, Eelco Chaudron wrote: > > > On 14 May 2024, at 15:48, Emma Finn wrote: > >> The AVX implementation for calcualting checksums was not >> handling carry-over addition correctly in some cases. >> This patch adds an additional shuffle to add 16-bit padding >> to the final part of the calculation to handle such cases. >> This commit also adds a unit test to fuzz test the actions >> autovalidator. >> >> Signed-off-by: Emma Finn <emma.f...@intel.com> >> Reported-by: Eelco Chaudron <echau...@redhat.com> > > Hi Emma, > > Thanks for also fixing the IPv6 case, however, the test you added does > not seem to catch the issue. See notes below. > > Cheers, > > Eelco > >> --- >> lib/odp-execute-avx512.c | 5 +++++ >> tests/dpif-netdev.at | 26 ++++++++++++++++++++++++++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c >> index 50c48bfd4..a74a85dc1 100644 >> --- a/lib/odp-execute-avx512.c >> +++ b/lib/odp-execute-avx512.c >> @@ -366,6 +366,8 @@ avx512_get_delta(__m256i old_header, __m256i new_header) >> 0xF, 0xF, 0xF, 0xF); >> v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta); >> >> + v_delta = _mm256_hadd_epi32(v_delta, v_zeros); >> + v_delta = _mm256_shuffle_epi8(v_delta, v_swap16a); >> v_delta = _mm256_hadd_epi32(v_delta, v_zeros); >> v_delta = _mm256_hadd_epi16(v_delta, v_zeros); >> >> @@ -575,6 +577,9 @@ avx512_ipv6_sum_header(__m512i ip6_header) >> 0xF, 0xF, 0xF, 0xF); >> >> v_delta = _mm256_permutexvar_epi32(v_swap32a, v_delta); >> + >> + v_delta = _mm256_hadd_epi32(v_delta, v_zeros); >> + v_delta = _mm256_shuffle_epi8(v_delta, v_swap16a); >> v_delta = _mm256_hadd_epi32(v_delta, v_zeros); >> v_delta = _mm256_hadd_epi16(v_delta, v_zeros); >> >> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >> index 790b5a43a..4db6a99e1 100644 >> --- a/tests/dpif-netdev.at >> +++ b/tests/dpif-netdev.at >> @@ -1091,3 +1091,29 @@ OVS_VSWITCHD_STOP(["dnl >> /Error: unknown miniflow extract implementation superstudy./d >> /Error: invalid study_pkt_cnt value: -pmd./d"]) >> AT_CLEANUP >> + >> +AT_SETUP([datapath - Actions Autovalidator Fuzzy]) > > This is not a Fuzzy test, but a normal Actions Autovalidator.
FWIW, even if it was, I don't think we should add any more fuzzy tests in a general testsuite. And we should find a way to get rid of the existing ones. Having non-reproducible tests is not good. > > However, the main problem with this test is that it does not find the problem. > Even without the C code changes, it’s passing the test. > > Maybe it will be better to add a specific test to capture checksum wrapping > for > IPv4 and 6. In addition, you should also make sure the received packet is ok. > You can use options:pcap=p1.pcap for this, see other test cases. I'd suggest to model the test after 'userspace offload - ip csum offload' test case we have in tests/dpif-netdev.at. It does very similar checks. > >> +AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], []) >> +AT_SKIP_IF([! $PYTHON3 $srcdir/genpkts.py 2000 > packets]) >> + >> +OVS_VSWITCHD_START(add-port br0 p0 -- set Interface p0 type=dummy \ >> + -- add-port br0 p1 -- set Interface p1 type=dummy) >> + >> +AT_CHECK([ovs-appctl odp-execute/action-impl-set autovalidator], [0], [dnl >> +Action implementation set to autovalidator. >> +]) >> + >> +AT_DATA([flows.txt], [dnl >> + in_port=p0,ip,actions=mod_nw_src=10.1.1.1,p1 >> + in_port=p0,ipv6,actions=set_field:fc00::100->ipv6_src,p1 >> +]) >> + >> +AT_CHECK([ovs-ofctl del-flows br0]) >> +AT_CHECK([ovs-ofctl -Oopenflow13 add-flows br0 flows.txt]) >> + >> +cat packets | while read line; do >> + AT_CHECK([ovs-appctl netdev-dummy/receive p0 $line], [0], [ignore]) >> +done >> + >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP >> -- >> 2.25.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
