> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Thursday, May 16, 2024 10:31 PM > To: Chaudron, Eelco <[email protected]>; Finn, Emma > <[email protected]> > Cc: [email protected]; [email protected] > Subject: Re: [ovs-dev] [v2] odp-execute: Fix AVX checksum calculation. > > 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 <[email protected]> > >> Reported-by: Eelco Chaudron <[email protected]> > > > > 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. >
Apologies, I pushed the wrong version of this patch. I will push a v3 that does actual fuzzing. I think this being a fuzzed test is the right approach, it will cover corner cases not just with checksum wrapping but any potential issues with the entire AVX Actions implementation as well. In the next version I have increased the amount of generated fuzz packets to 10K. Testing locally here I can see 10/10 runs catch the failures without my fixes. Thanks, Emma > > > >> +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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
