| Message ID | 20240514134815.2576245-1-emma.finn@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | [ovs-dev,v2] odp-execute: Fix AVX checksum calculation. | 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 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.finn@intel.com> > Reported-by: Eelco Chaudron <echaudro@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. 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. > +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
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.finn@intel.com> >> Reported-by: Eelco Chaudron <echaudro@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Thursday, May 16, 2024 10:31 PM > To: Chaudron, Eelco <echaudro@redhat.com>; Finn, Emma > <emma.finn@intel.com> > Cc: ovs-dev@openvswitch.org; i.maximets@ovn.org > 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 <emma.finn@intel.com> > >> Reported-by: Eelco Chaudron <echaudro@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. > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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]) +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
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.finn@intel.com> Reported-by: Eelco Chaudron <echaudro@redhat.com> --- lib/odp-execute-avx512.c | 5 +++++ tests/dpif-netdev.at | 26 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+)