| Message ID | 20240524092018.1152491-1-emma.finn@intel.com |
|---|---|
| State | Accepted |
| Commit | 7af0716ea621a8cebcd9c3061fcb7a044e343f14 |
| Delegated to: | Eelco Chaudron |
| Headers | show |
| Series | [ovs-dev,v4] 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 24 May 2024, at 11:20, 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 check the checksum carry-bits > issue with actions autovalidator enabled. Hi Emma, Thanks for sending out the v4. I have some small nits below, which I can fix during commit time. Assuming Ilya has no other simple to fix comments. Cheers, Eelco > 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 | 64 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 69 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..260986ba9 100644 > --- a/tests/dpif-netdev.at > +++ b/tests/dpif-netdev.at > @@ -1091,3 +1091,67 @@ 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 Checksum]) > + > +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. > +]) > + > +# Add flows to trigger checksum calculation Comments should end with a dot(.). Also, not sure if ‘#’ is fine here, as we are moving to ‘dnl’, but this file has both (most are ‘#’). Ilya? > +AT_DATA([flows.txt], [ddl > + 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]) > + > +# Make sure checksum won't be offloaded > +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum=false]) > +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum_set_good=false]) > + > +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) > + > +# IPv4 packet with values that will trigger carry-over addition for checksum > +flow_s_v4="\ > + eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x0800,\ > + nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,nw_frag=no,\ > + tp_src=54392,tp_dst=5201,tcp_flags=ack" > + > +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") > +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) > + > +# Checksum should change to 0xAC33 with ip_src changed to 10.1.1.1 > +# by the datapath while processing the packet. > +flow_expected=$(echo "${flow_s_v4}" | sed 's/229.167.36.90/10.1.1.1/g') > +good_expected=$(ovs-ofctl compose-packet --bare "${flow_expected}") > +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) > +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected} > +]) > + > +#Repeat similar test for IPv6 Space between # and Repeat. > +flow_s_v6="\ > + eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86dd, \ > + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ > + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ > + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ > + tp_src=20405,tp_dst=20662,tcp_flags=ack" > + > + A single new line is enough here. > +good_frame_v6=$(ovs-ofctl compose-packet --bare "${flow_s_v6}") > +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame_v6}]) > + > +# Checksum should change to 0x59FD with ipv6_src changed to fc00::100 > +# by the datapath while processing the packet. > +flow_expected_v6=$(echo "${flow_s_v6}" | \ > + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') > +good_expected_v6=$(ovs-ofctl compose-packet --bare "${flow_expected_v6}") > +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) > +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected_v6} > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > -- > 2.34.1
On 5/28/24 14:36, Eelco Chaudron wrote: > > > On 24 May 2024, at 11:20, 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 check the checksum carry-bits >> issue with actions autovalidator enabled. > > Hi Emma, > > Thanks for sending out the v4. I have some small nits below, which I can fix during commit time. Assuming Ilya has no other simple to fix comments. > > Cheers, > > Eelco > >> 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 | 64 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 69 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..260986ba9 100644 >> --- a/tests/dpif-netdev.at >> +++ b/tests/dpif-netdev.at >> @@ -1091,3 +1091,67 @@ 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 Checksum]) >> + >> +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. >> +]) >> + >> +# Add flows to trigger checksum calculation > > Comments should end with a dot(.). Also, not sure if ‘#’ is fine here, as we are > moving to ‘dnl’, but this file has both (most are ‘#’). Ilya? Both are fine, 'dnl' is a bit cleaner, so if you want to swap those on commit that's fine, but there is no point in new version just for that. Note that while backporting the fix we'll need to substitute the 'compose-packet' calls with their results, since bare packet compose is not available pre 3.3. > >> +AT_DATA([flows.txt], [ddl >> + 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]) >> + >> +# Make sure checksum won't be offloaded >> +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum=false]) >> +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum_set_good=false]) >> + >> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) >> + >> +# IPv4 packet with values that will trigger carry-over addition for checksum >> +flow_s_v4="\ >> + eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x0800,\ >> + nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,nw_frag=no,\ >> + tp_src=54392,tp_dst=5201,tcp_flags=ack" >> + >> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") >> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) >> + >> +# Checksum should change to 0xAC33 with ip_src changed to 10.1.1.1 >> +# by the datapath while processing the packet. >> +flow_expected=$(echo "${flow_s_v4}" | sed 's/229.167.36.90/10.1.1.1/g') >> +good_expected=$(ovs-ofctl compose-packet --bare "${flow_expected}") >> +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) >> +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected} >> +]) >> + >> +#Repeat similar test for IPv6 > > Space between # and Repeat. > >> +flow_s_v6="\ >> + eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86dd, \ >> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ >> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ >> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ >> + tp_src=20405,tp_dst=20662,tcp_flags=ack" Nit: Line continuation ('\') is not necessary within strings. >> + >> + > A single new line is enough here. > >> +good_frame_v6=$(ovs-ofctl compose-packet --bare "${flow_s_v6}") >> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame_v6}]) >> + >> +# Checksum should change to 0x59FD with ipv6_src changed to fc00::100 >> +# by the datapath while processing the packet. >> +flow_expected_v6=$(echo "${flow_s_v6}" | \ >> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') >> +good_expected_v6=$(ovs-ofctl compose-packet --bare "${flow_expected_v6}") >> +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) >> +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected_v6} >> +]) >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> -- >> 2.34.1 >
On 28 May 2024, at 16:49, Ilya Maximets wrote: > On 5/28/24 14:36, Eelco Chaudron wrote: >> >> >> On 24 May 2024, at 11:20, 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 check the checksum carry-bits >>> issue with actions autovalidator enabled. >> >> Hi Emma, >> >> Thanks for sending out the v4. I have some small nits below, which I can fix during commit time. Assuming Ilya has no other simple to fix comments. >> >> Cheers, >> >> Eelco >> >>> 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 | 64 ++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 69 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..260986ba9 100644 >>> --- a/tests/dpif-netdev.at >>> +++ b/tests/dpif-netdev.at >>> @@ -1091,3 +1091,67 @@ 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 Checksum]) >>> + >>> +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. >>> +]) >>> + >>> +# Add flows to trigger checksum calculation >> >> Comments should end with a dot(.). Also, not sure if ‘#’ is fine here, as we are >> moving to ‘dnl’, but this file has both (most are ‘#’). Ilya? > > Both are fine, 'dnl' is a bit cleaner, so if you want to swap those > on commit that's fine, but there is no point in new version just for > that. > > Note that while backporting the fix we'll need to substitute the > 'compose-packet' calls with their results, since bare packet compose > is not available pre 3.3. > >> >>> +AT_DATA([flows.txt], [ddl >>> + 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]) >>> + >>> +# Make sure checksum won't be offloaded >>> +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum=false]) >>> +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum_set_good=false]) >>> + >>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) >>> + >>> +# IPv4 packet with values that will trigger carry-over addition for checksum >>> +flow_s_v4="\ >>> + eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x0800,\ >>> + nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,nw_frag=no,\ >>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" >>> + >>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") >>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) >>> + >>> +# Checksum should change to 0xAC33 with ip_src changed to 10.1.1.1 >>> +# by the datapath while processing the packet. >>> +flow_expected=$(echo "${flow_s_v4}" | sed 's/229.167.36.90/10.1.1.1/g') >>> +good_expected=$(ovs-ofctl compose-packet --bare "${flow_expected}") >>> +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) >>> +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected} >>> +]) >>> + >>> +#Repeat similar test for IPv6 >> >> Space between # and Repeat. >> >>> +flow_s_v6="\ >>> + eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86dd, \ >>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ >>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ >>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ >>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" > > Nit: Line continuation ('\') is not necessary within strings. Right, I can fix all this on commit. Let me add my ACK below, and if you have no other objections, I’ll commit? Acked-by: Eelco Chaudron <echaudro@redhat.com> >>> + >>> + >> A single new line is enough here. >> >>> +good_frame_v6=$(ovs-ofctl compose-packet --bare "${flow_s_v6}") >>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame_v6}]) >>> + >>> +# Checksum should change to 0x59FD with ipv6_src changed to fc00::100 >>> +# by the datapath while processing the packet. >>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ >>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') >>> +good_expected_v6=$(ovs-ofctl compose-packet --bare "${flow_expected_v6}") >>> +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) >>> +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected_v6} >>> +]) >>> + >>> +OVS_VSWITCHD_STOP >>> +AT_CLEANUP >>> -- >>> 2.34.1 >>
On 5/29/24 11:01, Eelco Chaudron wrote: > > > On 28 May 2024, at 16:49, Ilya Maximets wrote: > >> On 5/28/24 14:36, Eelco Chaudron wrote: >>> >>> >>> On 24 May 2024, at 11:20, 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 check the checksum carry-bits >>>> issue with actions autovalidator enabled. >>> >>> Hi Emma, >>> >>> Thanks for sending out the v4. I have some small nits below, which I can fix during commit time. Assuming Ilya has no other simple to fix comments. >>> >>> Cheers, >>> >>> Eelco >>> >>>> 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 | 64 ++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 69 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..260986ba9 100644 >>>> --- a/tests/dpif-netdev.at >>>> +++ b/tests/dpif-netdev.at >>>> @@ -1091,3 +1091,67 @@ 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 Checksum]) >>>> + >>>> +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. >>>> +]) >>>> + >>>> +# Add flows to trigger checksum calculation >>> >>> Comments should end with a dot(.). Also, not sure if ‘#’ is fine here, as we are >>> moving to ‘dnl’, but this file has both (most are ‘#’). Ilya? >> >> Both are fine, 'dnl' is a bit cleaner, so if you want to swap those >> on commit that's fine, but there is no point in new version just for >> that. >> >> Note that while backporting the fix we'll need to substitute the >> 'compose-packet' calls with their results, since bare packet compose >> is not available pre 3.3. >> >>> >>>> +AT_DATA([flows.txt], [ddl >>>> + 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]) >>>> + >>>> +# Make sure checksum won't be offloaded >>>> +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum=false]) >>>> +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum_set_good=false]) >>>> + >>>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) >>>> + >>>> +# IPv4 packet with values that will trigger carry-over addition for checksum >>>> +flow_s_v4="\ >>>> + eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x0800,\ >>>> + nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,nw_frag=no,\ >>>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" >>>> + >>>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") >>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) >>>> + >>>> +# Checksum should change to 0xAC33 with ip_src changed to 10.1.1.1 >>>> +# by the datapath while processing the packet. >>>> +flow_expected=$(echo "${flow_s_v4}" | sed 's/229.167.36.90/10.1.1.1/g') >>>> +good_expected=$(ovs-ofctl compose-packet --bare "${flow_expected}") >>>> +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) >>>> +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected} >>>> +]) >>>> + >>>> +#Repeat similar test for IPv6 >>> >>> Space between # and Repeat. >>> >>>> +flow_s_v6="\ >>>> + eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86dd, \ >>>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ >>>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ >>>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ >>>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" >> >> Nit: Line continuation ('\') is not necessary within strings. > > Right, I can fix all this on commit. Let me add my ACK below, and if you > have no other objections, I’ll commit? No objections from my side. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > >>>> + >>>> + >>> A single new line is enough here. >>> >>>> +good_frame_v6=$(ovs-ofctl compose-packet --bare "${flow_s_v6}") >>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame_v6}]) >>>> + >>>> +# Checksum should change to 0x59FD with ipv6_src changed to fc00::100 >>>> +# by the datapath while processing the packet. >>>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ >>>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') >>>> +good_expected_v6=$(ovs-ofctl compose-packet --bare "${flow_expected_v6}") >>>> +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) >>>> +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected_v6} >>>> +]) >>>> + >>>> +OVS_VSWITCHD_STOP >>>> +AT_CLEANUP >>>> -- >>>> 2.34.1 >>> >
On 29 May 2024, at 14:51, Ilya Maximets wrote: > On 5/29/24 11:01, Eelco Chaudron wrote: >> >> >> On 28 May 2024, at 16:49, Ilya Maximets wrote: >> >>> On 5/28/24 14:36, Eelco Chaudron wrote: >>>> >>>> >>>> On 24 May 2024, at 11:20, 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 check the checksum carry-bits >>>>> issue with actions autovalidator enabled. Hi Emma, I made the small changes, and did some more testing before I committed. However, there are more failures in the same area with or without your patch. I’m holding of committing this patch as it might be related. The failing tests are (on latest main branch): 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED (ofproto.at:6668) 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED (nsh.at:816) Here are some details: 2024-05-29T14:18:53.923Z|00119|odp_execute_impl|ERR|Autovalidation of avx512 failed. Details: Packet: 0 Action : set(ipv6(tclass=0x2/0x3)) Good hex: 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 20 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f 2024-05-29T14:18:53.926Z|00120|unixctl|DBG|received request netdev-dummy/receive["p1","in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x86dd),ipv6(src=2001:db8::1,dst=111:db8::6,proto=1,tclass=0,hlimit=64,frag=no),icmpv6(type=0,code=8)"], id=0 2024-05-29T14:18:53.926Z|00121|unixctl|DBG|replying with success, id=0: "" 2024-05-29T14:18:53.926Z|00122|odp_execute_impl|ERR|Autovalidation of avx512 failed. Details: Packet: 0 Action : set(ipv6(tclass=0x40/0xfc)) Good hex: 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 64 00 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f And 2024-05-29T14:18:54.503Z|00659|odp_execute_impl|ERR|Autovalidation of avx512 failed. Details: Packet: 0 Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) Good hex: 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 2024-05-29T14:18:54.506Z|00660|unixctl|DBG|received request netdev-dummy/receive["n1","1e2ce92a669e3a6dd2099cab0800450000548a83400040011aadc0a80a0ac0a80a1e0800b7170a4d0002fd509a5800000000de1c020000000000101112131415161718191a1b1c1d1e1f202122232425262728292a2b2c2d2e2f3031323334353637"], id=0 2024-05-29T14:18:54.506Z|00661|unixctl|DBG|replying with success, id=0: "" 2024-05-29T14:18:54.506Z|00662|odp_execute_impl|ERR|Autovalidation of avx512 failed. Details: Packet: 0 Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) Good hex: 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Etc. etc. Let me know if this requires a v5 of your patch, or is in a different area? >>>> Hi Emma, >>>> >>>> Thanks for sending out the v4. I have some small nits below, which I can fix during commit time. Assuming Ilya has no other simple to fix comments. >>>> >>>> Cheers, >>>> >>>> Eelco >>>> >>>>> 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 | 64 ++++++++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 69 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..260986ba9 100644 >>>>> --- a/tests/dpif-netdev.at >>>>> +++ b/tests/dpif-netdev.at >>>>> @@ -1091,3 +1091,67 @@ 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 Checksum]) >>>>> + >>>>> +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. >>>>> +]) >>>>> + >>>>> +# Add flows to trigger checksum calculation >>>> >>>> Comments should end with a dot(.). Also, not sure if ‘#’ is fine here, as we are >>>> moving to ‘dnl’, but this file has both (most are ‘#’). Ilya? >>> >>> Both are fine, 'dnl' is a bit cleaner, so if you want to swap those >>> on commit that's fine, but there is no point in new version just for >>> that. >>> >>> Note that while backporting the fix we'll need to substitute the >>> 'compose-packet' calls with their results, since bare packet compose >>> is not available pre 3.3. >>> >>>> >>>>> +AT_DATA([flows.txt], [ddl >>>>> + 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]) >>>>> + >>>>> +# Make sure checksum won't be offloaded >>>>> +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum=false]) >>>>> +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum_set_good=false]) >>>>> + >>>>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) >>>>> + >>>>> +# IPv4 packet with values that will trigger carry-over addition for checksum >>>>> +flow_s_v4="\ >>>>> + eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x0800,\ >>>>> + nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,nw_frag=no,\ >>>>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" >>>>> + >>>>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") >>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) >>>>> + >>>>> +# Checksum should change to 0xAC33 with ip_src changed to 10.1.1.1 >>>>> +# by the datapath while processing the packet. >>>>> +flow_expected=$(echo "${flow_s_v4}" | sed 's/229.167.36.90/10.1.1.1/g') >>>>> +good_expected=$(ovs-ofctl compose-packet --bare "${flow_expected}") >>>>> +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) >>>>> +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected} >>>>> +]) >>>>> + >>>>> +#Repeat similar test for IPv6 >>>> >>>> Space between # and Repeat. >>>> >>>>> +flow_s_v6="\ >>>>> + eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86dd, \ >>>>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ >>>>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ >>>>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ >>>>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" >>> >>> Nit: Line continuation ('\') is not necessary within strings. >> >> Right, I can fix all this on commit. Let me add my ACK below, and if you >> have no other objections, I’ll commit? > > No objections from my side. > >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> >>>>> + >>>>> + >>>> A single new line is enough here. >>>> >>>>> +good_frame_v6=$(ovs-ofctl compose-packet --bare "${flow_s_v6}") >>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame_v6}]) >>>>> + >>>>> +# Checksum should change to 0x59FD with ipv6_src changed to fc00::100 >>>>> +# by the datapath while processing the packet. >>>>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ >>>>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') >>>>> +good_expected_v6=$(ovs-ofctl compose-packet --bare "${flow_expected_v6}") >>>>> +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) >>>>> +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected_v6} >>>>> +]) >>>>> + >>>>> +OVS_VSWITCHD_STOP >>>>> +AT_CLEANUP >>>>> -- >>>>> 2.34.1 >>>> >>
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Wednesday, May 29, 2024 3:23 PM > To: Finn, Emma <emma.finn@intel.com> > Cc: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Van > Haaren, Harry <harry.van.haaren@intel.com> > Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. > > > > On 29 May 2024, at 14:51, Ilya Maximets wrote: > > > On 5/29/24 11:01, Eelco Chaudron wrote: > >> > >> > >> On 28 May 2024, at 16:49, Ilya Maximets wrote: > >> > >>> On 5/28/24 14:36, Eelco Chaudron wrote: > >>>> > >>>> > >>>> On 24 May 2024, at 11:20, 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 check the checksum carry-bits issue with > >>>>> actions autovalidator enabled. > > Hi Emma, > > I made the small changes, and did some more testing before I committed. > However, there are more failures in the same area with or without your patch. > I’m holding of committing this patch as it might be related. > Hi Eelco, These tests are unrelated to this patch so I think we should go ahead and merge this. > The failing tests are (on latest main branch): > > 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED > (ofproto.at:6668) I investigated this test and the SIMD implementation isn't handling traffic class field correctly. I'm on PTO for the next week but I will make a fix for this once I'm back. > 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED > (nsh.at:816) > For this one it looks like the scalar is expecting an ipv4 checksum of 0x000 and the SIMD implementation has calculated an ipv4 checksum of 0xDF77. This is more a logic question whether or not the checksum should be calculated for this? Thoughts? Thanks, Emma > > Here are some details: > > 2024-05-29T14:18:53.923Z|00119|odp_execute_impl|ERR|Autovalidation > of avx512 failed. Details: > Packet: 0 > Action : set(ipv6(tclass=0x2/0x3)) > Good hex: > 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 20 > 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 > 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: > 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 > 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 > 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f 2024-05- > 29T14:18:53.926Z|00120|unixctl|DBG|received request netdev- > dummy/receive["p1","in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:0 > 0:00:0c),eth_type(0x86dd),ipv6(src=2001:db8::1,dst=111:db8::6,proto=1,tcl > ass=0,hlimit=64,frag=no),icmpv6(type=0,code=8)"], id=0 2024-05- > 29T14:18:53.926Z|00121|unixctl|DBG|replying with success, id=0: "" > 2024-05-29T14:18:53.926Z|00122|odp_execute_impl|ERR|Autovalidation > of avx512 failed. Details: > Packet: 0 > Action : set(ipv6(tclass=0x40/0xfc)) > Good hex: > 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 64 00 > 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 > 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: > 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 > 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 > 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f > > And > > 2024-05-29T14:18:54.503Z|00659|odp_execute_impl|ERR|Autovalidation > of avx512 failed. Details: > Packet: 0 > Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) > Good hex: > 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 > 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 > 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 > 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 > 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: > 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 > 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 > 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 > 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 > 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 2024-05- > 29T14:18:54.506Z|00660|unixctl|DBG|received request netdev- > dummy/receive["n1","1e2ce92a669e3a6dd2099cab0800450000548a8340 > 0040011aadc0a80a0ac0a80a1e0800b7170a4d0002fd509a5800000000de1 > c020000000000101112131415161718191a1b1c1d1e1f20212223242526 > 2728292a2b2c2d2e2f3031323334353637"], id=0 2024-05- > 29T14:18:54.506Z|00661|unixctl|DBG|replying with success, id=0: "" > 2024-05-29T14:18:54.506Z|00662|odp_execute_impl|ERR|Autovalidation > of avx512 failed. Details: > Packet: 0 > Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) > Good hex: > 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 > 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 > 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 > 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c > 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: > 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 > 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 > 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 > 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c > 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 > > Etc. etc. > > > Let me know if this requires a v5 of your patch, or is in a different area? > > >>>> Hi Emma, > >>>> > >>>> Thanks for sending out the v4. I have some small nits below, which I can > fix during commit time. Assuming Ilya has no other simple to fix comments. > >>>> > >>>> Cheers, > >>>> > >>>> Eelco > >>>> > >>>>> 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 | 64 > ++++++++++++++++++++++++++++++++++++++++ > >>>>> 2 files changed, 69 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..260986ba9 100644 > >>>>> --- a/tests/dpif-netdev.at > >>>>> +++ b/tests/dpif-netdev.at > >>>>> @@ -1091,3 +1091,67 @@ 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 Checksum]) > >>>>> + > >>>>> +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. > >>>>> +]) > >>>>> + > >>>>> +# Add flows to trigger checksum calculation > >>>> > >>>> Comments should end with a dot(.). Also, not sure if ‘#’ is fine > >>>> here, as we are moving to ‘dnl’, but this file has both (most are ‘#’). Ilya? > >>> > >>> Both are fine, 'dnl' is a bit cleaner, so if you want to swap those > >>> on commit that's fine, but there is no point in new version just for > >>> that. > >>> > >>> Note that while backporting the fix we'll need to substitute the > >>> 'compose-packet' calls with their results, since bare packet compose > >>> is not available pre 3.3. > >>> > >>>> > >>>>> +AT_DATA([flows.txt], [ddl > >>>>> + 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]) > >>>>> + > >>>>> +# Make sure checksum won't be offloaded AT_CHECK([ovs-vsctl set > >>>>> +Interface p0 options:ol_ip_csum=false]) AT_CHECK([ovs-vsctl set > >>>>> +Interface p0 options:ol_ip_csum_set_good=false]) > >>>>> + > >>>>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) > >>>>> + > >>>>> +# IPv4 packet with values that will trigger carry-over addition > >>>>> +for checksum flow_s_v4="\ > >>>>> + > >>>>> > +eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x080 > >>>>> +0,\ > >>>>> + > >>>>> > +nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,n > >>>>> +w_frag=no,\ > >>>>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" > >>>>> + > >>>>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") > >>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) > >>>>> + > >>>>> +# Checksum should change to 0xAC33 with ip_src changed to > >>>>> +10.1.1.1 # by the datapath while processing the packet. > >>>>> +flow_expected=$(echo "${flow_s_v4}" | sed > >>>>> +'s/229.167.36.90/10.1.1.1/g') good_expected=$(ovs-ofctl > >>>>> +compose-packet --bare "${flow_expected}") AT_CHECK([ovs-pcap > >>>>> +p1.pcap > p1.pcap.txt 2>&1]) AT_CHECK_UNQUOTED([tail -n 1 > >>>>> +p1.pcap.txt], [0], [${good_expected} > >>>>> +]) > >>>>> + > >>>>> +#Repeat similar test for IPv6 > >>>> > >>>> Space between # and Repeat. > >>>> > >>>>> +flow_s_v6="\ > >>>>> + > >>>>> +eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86d > >>>>> +d, \ > >>>>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ > >>>>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ > >>>>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ > >>>>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" > >>> > >>> Nit: Line continuation ('\') is not necessary within strings. > >> > >> Right, I can fix all this on commit. Let me add my ACK below, and if > >> you have no other objections, I’ll commit? > > > > No objections from my side. > > > >> > >> Acked-by: Eelco Chaudron <echaudro@redhat.com> > >> > >>>>> + > >>>>> + > >>>> A single new line is enough here. > >>>> > >>>>> +good_frame_v6=$(ovs-ofctl compose-packet --bare "${flow_s_v6}") > >>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 > ${good_frame_v6}]) > >>>>> + > >>>>> +# Checksum should change to 0x59FD with ipv6_src changed to > >>>>> +fc00::100 # by the datapath while processing the packet. > >>>>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ > >>>>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') > >>>>> +good_expected_v6=$(ovs-ofctl compose-packet --bare > >>>>> +"${flow_expected_v6}") AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt > >>>>> +2>&1]) AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], > >>>>> +[${good_expected_v6} > >>>>> +]) > >>>>> + > >>>>> +OVS_VSWITCHD_STOP > >>>>> +AT_CLEANUP > >>>>> -- > >>>>> 2.34.1 > >>>> > >>
On 30 May 2024, at 14:46, Finn, Emma wrote: >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Wednesday, May 29, 2024 3:23 PM >> To: Finn, Emma <emma.finn@intel.com> >> Cc: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Van >> Haaren, Harry <harry.van.haaren@intel.com> >> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. >> >> >> >> On 29 May 2024, at 14:51, Ilya Maximets wrote: >> >>> On 5/29/24 11:01, Eelco Chaudron wrote: >>>> >>>> >>>> On 28 May 2024, at 16:49, Ilya Maximets wrote: >>>> >>>>> On 5/28/24 14:36, Eelco Chaudron wrote: >>>>>> >>>>>> >>>>>> On 24 May 2024, at 11:20, 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 check the checksum carry-bits issue with >>>>>>> actions autovalidator enabled. >> >> Hi Emma, >> >> I made the small changes, and did some more testing before I committed. >> However, there are more failures in the same area with or without your patch. >> I’m holding of committing this patch as it might be related. >> > > Hi Eelco, > > These tests are unrelated to this patch so I think we should go ahead and merge this. Ok, I’ll go ahead and apply it later today. >> The failing tests are (on latest main branch): >> >> 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED >> (ofproto.at:6668) > > I investigated this test and the SIMD implementation isn't handling traffic class field correctly. I'm on PTO for the next week but I will make a fix for this once I'm back. Thanks! >> 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED >> (nsh.at:816) >> > For this one it looks like the scalar is expecting an ipv4 checksum of 0x000 and the SIMD implementation has calculated an ipv4 checksum of 0xDF77. > This is more a logic question whether or not the checksum should be calculated for this? Thoughts? I need to look at the tests, but if it’s a UDP packet, and the original UDP checksum was 0, it should stay zero. >> Here are some details: >> >> 2024-05-29T14:18:53.923Z|00119|odp_execute_impl|ERR|Autovalidation >> of avx512 failed. Details: >> Packet: 0 >> Action : set(ipv6(tclass=0x2/0x3)) >> Good hex: >> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 20 >> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 >> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: >> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 >> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 >> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f 2024-05- >> 29T14:18:53.926Z|00120|unixctl|DBG|received request netdev- >> dummy/receive["p1","in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:0 >> 0:00:0c),eth_type(0x86dd),ipv6(src=2001:db8::1,dst=111:db8::6,proto=1,tcl >> ass=0,hlimit=64,frag=no),icmpv6(type=0,code=8)"], id=0 2024-05- >> 29T14:18:53.926Z|00121|unixctl|DBG|replying with success, id=0: "" >> 2024-05-29T14:18:53.926Z|00122|odp_execute_impl|ERR|Autovalidation >> of avx512 failed. Details: >> Packet: 0 >> Action : set(ipv6(tclass=0x40/0xfc)) >> Good hex: >> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 64 00 >> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 >> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: >> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 >> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 >> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f >> >> And >> >> 2024-05-29T14:18:54.503Z|00659|odp_execute_impl|ERR|Autovalidation >> of avx512 failed. Details: >> Packet: 0 >> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) >> Good hex: >> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 >> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 >> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 >> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 >> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: >> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 >> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 >> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 >> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 >> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 2024-05- >> 29T14:18:54.506Z|00660|unixctl|DBG|received request netdev- >> dummy/receive["n1","1e2ce92a669e3a6dd2099cab0800450000548a8340 >> 0040011aadc0a80a0ac0a80a1e0800b7170a4d0002fd509a5800000000de1 >> c020000000000101112131415161718191a1b1c1d1e1f20212223242526 >> 2728292a2b2c2d2e2f3031323334353637"], id=0 2024-05- >> 29T14:18:54.506Z|00661|unixctl|DBG|replying with success, id=0: "" >> 2024-05-29T14:18:54.506Z|00662|odp_execute_impl|ERR|Autovalidation >> of avx512 failed. Details: >> Packet: 0 >> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) >> Good hex: >> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 >> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 >> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 >> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c >> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: >> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 >> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 >> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 >> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c >> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 >> >> Etc. etc. >> >> >> Let me know if this requires a v5 of your patch, or is in a different area? >> >>>>>> Hi Emma, >>>>>> >>>>>> Thanks for sending out the v4. I have some small nits below, which I can >> fix during commit time. Assuming Ilya has no other simple to fix comments. >>>>>> >>>>>> Cheers, >>>>>> >>>>>> Eelco >>>>>> >>>>>>> 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 | 64 >> ++++++++++++++++++++++++++++++++++++++++ >>>>>>> 2 files changed, 69 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..260986ba9 100644 >>>>>>> --- a/tests/dpif-netdev.at >>>>>>> +++ b/tests/dpif-netdev.at >>>>>>> @@ -1091,3 +1091,67 @@ 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 Checksum]) >>>>>>> + >>>>>>> +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. >>>>>>> +]) >>>>>>> + >>>>>>> +# Add flows to trigger checksum calculation >>>>>> >>>>>> Comments should end with a dot(.). Also, not sure if ‘#’ is fine >>>>>> here, as we are moving to ‘dnl’, but this file has both (most are ‘#’). Ilya? >>>>> >>>>> Both are fine, 'dnl' is a bit cleaner, so if you want to swap those >>>>> on commit that's fine, but there is no point in new version just for >>>>> that. >>>>> >>>>> Note that while backporting the fix we'll need to substitute the >>>>> 'compose-packet' calls with their results, since bare packet compose >>>>> is not available pre 3.3. >>>>> >>>>>> >>>>>>> +AT_DATA([flows.txt], [ddl >>>>>>> + 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]) >>>>>>> + >>>>>>> +# Make sure checksum won't be offloaded AT_CHECK([ovs-vsctl set >>>>>>> +Interface p0 options:ol_ip_csum=false]) AT_CHECK([ovs-vsctl set >>>>>>> +Interface p0 options:ol_ip_csum_set_good=false]) >>>>>>> + >>>>>>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) >>>>>>> + >>>>>>> +# IPv4 packet with values that will trigger carry-over addition >>>>>>> +for checksum flow_s_v4="\ >>>>>>> + >>>>>>> >> +eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x080 >>>>>>> +0,\ >>>>>>> + >>>>>>> >> +nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,n >>>>>>> +w_frag=no,\ >>>>>>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" >>>>>>> + >>>>>>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") >>>>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) >>>>>>> + >>>>>>> +# Checksum should change to 0xAC33 with ip_src changed to >>>>>>> +10.1.1.1 # by the datapath while processing the packet. >>>>>>> +flow_expected=$(echo "${flow_s_v4}" | sed >>>>>>> +'s/229.167.36.90/10.1.1.1/g') good_expected=$(ovs-ofctl >>>>>>> +compose-packet --bare "${flow_expected}") AT_CHECK([ovs-pcap >>>>>>> +p1.pcap > p1.pcap.txt 2>&1]) AT_CHECK_UNQUOTED([tail -n 1 >>>>>>> +p1.pcap.txt], [0], [${good_expected} >>>>>>> +]) >>>>>>> + >>>>>>> +#Repeat similar test for IPv6 >>>>>> >>>>>> Space between # and Repeat. >>>>>> >>>>>>> +flow_s_v6="\ >>>>>>> + >>>>>>> +eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86d >>>>>>> +d, \ >>>>>>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ >>>>>>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ >>>>>>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ >>>>>>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" >>>>> >>>>> Nit: Line continuation ('\') is not necessary within strings. >>>> >>>> Right, I can fix all this on commit. Let me add my ACK below, and if >>>> you have no other objections, I’ll commit? >>> >>> No objections from my side. >>> >>>> >>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>>> >>>>>>> + >>>>>>> + >>>>>> A single new line is enough here. >>>>>> >>>>>>> +good_frame_v6=$(ovs-ofctl compose-packet --bare "${flow_s_v6}") >>>>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 >> ${good_frame_v6}]) >>>>>>> + >>>>>>> +# Checksum should change to 0x59FD with ipv6_src changed to >>>>>>> +fc00::100 # by the datapath while processing the packet. >>>>>>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ >>>>>>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') >>>>>>> +good_expected_v6=$(ovs-ofctl compose-packet --bare >>>>>>> +"${flow_expected_v6}") AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt >>>>>>> +2>&1]) AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], >>>>>>> +[${good_expected_v6} >>>>>>> +]) >>>>>>> + >>>>>>> +OVS_VSWITCHD_STOP >>>>>>> +AT_CLEANUP >>>>>>> -- >>>>>>> 2.34.1 >>>>>> >>>>
On 30 May 2024, at 15:28, Eelco Chaudron wrote: > On 30 May 2024, at 14:46, Finn, Emma wrote: > >>> -----Original Message----- >>> From: Eelco Chaudron <echaudro@redhat.com> >>> Sent: Wednesday, May 29, 2024 3:23 PM >>> To: Finn, Emma <emma.finn@intel.com> >>> Cc: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Van >>> Haaren, Harry <harry.van.haaren@intel.com> >>> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. >>> >>> >>> >>> On 29 May 2024, at 14:51, Ilya Maximets wrote: >>> >>>> On 5/29/24 11:01, Eelco Chaudron wrote: >>>>> >>>>> >>>>> On 28 May 2024, at 16:49, Ilya Maximets wrote: >>>>> >>>>>> On 5/28/24 14:36, Eelco Chaudron wrote: >>>>>>> >>>>>>> >>>>>>> On 24 May 2024, at 11:20, 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 check the checksum carry-bits issue with >>>>>>>> actions autovalidator enabled. >>> >>> Hi Emma, >>> >>> I made the small changes, and did some more testing before I committed. >>> However, there are more failures in the same area with or without your patch. >>> I’m holding of committing this patch as it might be related. >>> >> >> Hi Eelco, >> >> These tests are unrelated to this patch so I think we should go ahead and merge this. > > Ok, I’ll go ahead and apply it later today. > >>> The failing tests are (on latest main branch): >>> >>> 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED >>> (ofproto.at:6668) >> >> I investigated this test and the SIMD implementation isn't handling traffic class field correctly. I'm on PTO for the next week but I will make a fix for this once I'm back. > > Thanks! > >>> 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED >>> (nsh.at:816) >>> >> For this one it looks like the scalar is expecting an ipv4 checksum of 0x000 and the SIMD implementation has calculated an ipv4 checksum of 0xDF77. >> This is more a logic question whether or not the checksum should be calculated for this? Thoughts? > > I need to look at the tests, but if it’s a UDP packet, and the original UDP checksum was 0, it should stay zero. In addition, any idea why these tests do not fail in Intel’s upstream unit tests? Do they use different hardware? Copied in Michael, maybe he knows more about the setup/tests. //Eelco >>> Here are some details: >>> >>> 2024-05-29T14:18:53.923Z|00119|odp_execute_impl|ERR|Autovalidation >>> of avx512 failed. Details: >>> Packet: 0 >>> Action : set(ipv6(tclass=0x2/0x3)) >>> Good hex: >>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 20 >>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 >>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: >>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 >>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 >>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f 2024-05- >>> 29T14:18:53.926Z|00120|unixctl|DBG|received request netdev- >>> dummy/receive["p1","in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:0 >>> 0:00:0c),eth_type(0x86dd),ipv6(src=2001:db8::1,dst=111:db8::6,proto=1,tcl >>> ass=0,hlimit=64,frag=no),icmpv6(type=0,code=8)"], id=0 2024-05- >>> 29T14:18:53.926Z|00121|unixctl|DBG|replying with success, id=0: "" >>> 2024-05-29T14:18:53.926Z|00122|odp_execute_impl|ERR|Autovalidation >>> of avx512 failed. Details: >>> Packet: 0 >>> Action : set(ipv6(tclass=0x40/0xfc)) >>> Good hex: >>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 64 00 >>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 >>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: >>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 >>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 >>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f >>> >>> And >>> >>> 2024-05-29T14:18:54.503Z|00659|odp_execute_impl|ERR|Autovalidation >>> of avx512 failed. Details: >>> Packet: 0 >>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) >>> Good hex: >>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 >>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 >>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 >>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 >>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: >>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 >>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 >>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 >>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 >>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 2024-05- >>> 29T14:18:54.506Z|00660|unixctl|DBG|received request netdev- >>> dummy/receive["n1","1e2ce92a669e3a6dd2099cab0800450000548a8340 >>> 0040011aadc0a80a0ac0a80a1e0800b7170a4d0002fd509a5800000000de1 >>> c020000000000101112131415161718191a1b1c1d1e1f20212223242526 >>> 2728292a2b2c2d2e2f3031323334353637"], id=0 2024-05- >>> 29T14:18:54.506Z|00661|unixctl|DBG|replying with success, id=0: "" >>> 2024-05-29T14:18:54.506Z|00662|odp_execute_impl|ERR|Autovalidation >>> of avx512 failed. Details: >>> Packet: 0 >>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) >>> Good hex: >>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 >>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 >>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 >>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c >>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: >>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 >>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 >>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 >>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c >>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 >>> >>> Etc. etc. >>> >>> >>> Let me know if this requires a v5 of your patch, or is in a different area? >>> >>>>>>> Hi Emma, >>>>>>> >>>>>>> Thanks for sending out the v4. I have some small nits below, which I can >>> fix during commit time. Assuming Ilya has no other simple to fix comments. >>>>>>> >>>>>>> Cheers, >>>>>>> >>>>>>> Eelco >>>>>>> >>>>>>>> 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 | 64 >>> ++++++++++++++++++++++++++++++++++++++++ >>>>>>>> 2 files changed, 69 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..260986ba9 100644 >>>>>>>> --- a/tests/dpif-netdev.at >>>>>>>> +++ b/tests/dpif-netdev.at >>>>>>>> @@ -1091,3 +1091,67 @@ 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 Checksum]) >>>>>>>> + >>>>>>>> +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. >>>>>>>> +]) >>>>>>>> + >>>>>>>> +# Add flows to trigger checksum calculation >>>>>>> >>>>>>> Comments should end with a dot(.). Also, not sure if ‘#’ is fine >>>>>>> here, as we are moving to ‘dnl’, but this file has both (most are ‘#’). Ilya? >>>>>> >>>>>> Both are fine, 'dnl' is a bit cleaner, so if you want to swap those >>>>>> on commit that's fine, but there is no point in new version just for >>>>>> that. >>>>>> >>>>>> Note that while backporting the fix we'll need to substitute the >>>>>> 'compose-packet' calls with their results, since bare packet compose >>>>>> is not available pre 3.3. >>>>>> >>>>>>> >>>>>>>> +AT_DATA([flows.txt], [ddl >>>>>>>> + 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]) >>>>>>>> + >>>>>>>> +# Make sure checksum won't be offloaded AT_CHECK([ovs-vsctl set >>>>>>>> +Interface p0 options:ol_ip_csum=false]) AT_CHECK([ovs-vsctl set >>>>>>>> +Interface p0 options:ol_ip_csum_set_good=false]) >>>>>>>> + >>>>>>>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) >>>>>>>> + >>>>>>>> +# IPv4 packet with values that will trigger carry-over addition >>>>>>>> +for checksum flow_s_v4="\ >>>>>>>> + >>>>>>>> >>> +eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x080 >>>>>>>> +0,\ >>>>>>>> + >>>>>>>> >>> +nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,n >>>>>>>> +w_frag=no,\ >>>>>>>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" >>>>>>>> + >>>>>>>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") >>>>>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) >>>>>>>> + >>>>>>>> +# Checksum should change to 0xAC33 with ip_src changed to >>>>>>>> +10.1.1.1 # by the datapath while processing the packet. >>>>>>>> +flow_expected=$(echo "${flow_s_v4}" | sed >>>>>>>> +'s/229.167.36.90/10.1.1.1/g') good_expected=$(ovs-ofctl >>>>>>>> +compose-packet --bare "${flow_expected}") AT_CHECK([ovs-pcap >>>>>>>> +p1.pcap > p1.pcap.txt 2>&1]) AT_CHECK_UNQUOTED([tail -n 1 >>>>>>>> +p1.pcap.txt], [0], [${good_expected} >>>>>>>> +]) >>>>>>>> + >>>>>>>> +#Repeat similar test for IPv6 >>>>>>> >>>>>>> Space between # and Repeat. >>>>>>> >>>>>>>> +flow_s_v6="\ >>>>>>>> + >>>>>>>> +eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86d >>>>>>>> +d, \ >>>>>>>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ >>>>>>>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ >>>>>>>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ >>>>>>>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" >>>>>> >>>>>> Nit: Line continuation ('\') is not necessary within strings. >>>>> >>>>> Right, I can fix all this on commit. Let me add my ACK below, and if >>>>> you have no other objections, I’ll commit? >>>> >>>> No objections from my side. >>>> >>>>> >>>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>>>> >>>>>>>> + >>>>>>>> + >>>>>>> A single new line is enough here. >>>>>>> >>>>>>>> +good_frame_v6=$(ovs-ofctl compose-packet --bare "${flow_s_v6}") >>>>>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 >>> ${good_frame_v6}]) >>>>>>>> + >>>>>>>> +# Checksum should change to 0x59FD with ipv6_src changed to >>>>>>>> +fc00::100 # by the datapath while processing the packet. >>>>>>>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ >>>>>>>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') >>>>>>>> +good_expected_v6=$(ovs-ofctl compose-packet --bare >>>>>>>> +"${flow_expected_v6}") AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt >>>>>>>> +2>&1]) AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], >>>>>>>> +[${good_expected_v6} >>>>>>>> +]) >>>>>>>> + >>>>>>>> +OVS_VSWITCHD_STOP >>>>>>>> +AT_CLEANUP >>>>>>>> -- >>>>>>>> 2.34.1 >>>>>>> >>>>>
On 24 May 2024, at 11:20, 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 check the checksum carry-bits > issue with actions autovalidator enabled. > > Signed-off-by: Emma Finn <emma.finn@intel.com> > Reported-by: Eelco Chaudron <echaudro@redhat.com> Thanks Emma and others for the feedback on the patch. It has been applied upstream. Cheers, Eelco
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Thursday, May 30, 2024 2:44 PM > To: Finn, Emma <emma.finn@intel.com>; Phelan, Michael > <michael.phelan@intel.com> > Cc: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Van > Haaren, Harry <harry.van.haaren@intel.com> > Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. > > > > On 30 May 2024, at 15:28, Eelco Chaudron wrote: > > > On 30 May 2024, at 14:46, Finn, Emma wrote: > > > >>> -----Original Message----- > >>> From: Eelco Chaudron <echaudro@redhat.com<mailto:echaudro@redhat.com>> > >>> Sent: Wednesday, May 29, 2024 3:23 PM > >>> To: Finn, Emma <emma.finn@intel.com<mailto:emma.finn@intel.com>> > >>> Cc: Ilya Maximets <i.maximets@ovn.org<mailto:i.maximets@ovn.org>>; ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org>; Van > >>> Haaren, Harry <harry.van.haaren@intel.com<mailto:harry.van.haaren@intel.com>> > >>> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. > >>> > >>> > >>> > >>> On 29 May 2024, at 14:51, Ilya Maximets wrote: > >>> > >>>> On 5/29/24 11:01, Eelco Chaudron wrote: > >>>>> > >>>>> > >>>>> On 28 May 2024, at 16:49, Ilya Maximets wrote: > >>>>> > >>>>>> On 5/28/24 14:36, Eelco Chaudron wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 24 May 2024, at 11:20, 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 check the checksum carry-bits > >>>>>>>> issue with actions autovalidator enabled. > >>> > >>> Hi Emma, > >>> > >>> I made the small changes, and did some more testing before I committed. > >>> However, there are more failures in the same area with or without your > patch. > >>> I’m holding of committing this patch as it might be related. > >>> > >> > >> Hi Eelco, > >> > >> These tests are unrelated to this patch so I think we should go ahead and > merge this. > > > > Ok, I’ll go ahead and apply it later today. > > > >>> The failing tests are (on latest main branch): > >>> > >>> 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED > >>> (ofproto.at:6668) > >> > >> I investigated this test and the SIMD implementation isn't handling traffic > class field correctly. I'm on PTO for the next week but I will make a fix for this > once I'm back. > > > > Thanks! > > > >>> 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe > >>> FAILED > >>> (nsh.at:816) > >>> > >> For this one it looks like the scalar is expecting an ipv4 checksum of 0x000 > and the SIMD implementation has calculated an ipv4 checksum of 0xDF77. > >> This is more a logic question whether or not the checksum should be > calculated for this? Thoughts? > > > > I need to look at the tests, but if it’s a UDP packet, and the original UDP > checksum was 0, it should stay zero. > > > In addition, any idea why these tests do not fail in Intel’s upstream unit tests? > Do they use different hardware? Copied in Michael, maybe he knows more > about the setup/tests. > > //Eelco > I have investigated both unit test failures. 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED (ofproto.at:6668) For this one, the AVX implementation didn't handle setting the IPv6 traffic class field. 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED (nsh.at:816) For this one, the AVX implementation was missing a check for IPv4 checksum offload flag. I have 2 separate patches to fix these issues and will send shortly. As for the Intel unit test CI (ovsrobot/intel-ovs-compilation), make check is never run with any of the AVX autovalidators enabled. Table below shows the 4 builds and the unit tests ran after each build. Name Build Unit tests ACTIONS ./configure --enable-actions-default-autovalidator make check-dpdk make check-system-userspace DPCLS ./configure --enable-autovalidator make check-dpdk make check-system-userspace DPIF ./configure --enable-dpif-default-avx512 make check-dpdk make check-system-userspace MFEX ./configure --enable-mfex-default-autovalidator make check-dpdk make check-system-userspace > >>> Here are some details: > >>> > >>> 2024-05- > 29T14:18:53.923Z|00119|odp_execute_impl|ERR|Autovalidation > >>> of avx512 failed. Details: > >>> Packet: 0 > >>> Action : set(ipv6(tclass=0x2/0x3)) > >>> Good hex: > >>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 20 > >>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > >>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > >>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 > >>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > >>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > >>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > >>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: > >>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 > >>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > >>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > >>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 > >>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > >>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > >>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > >>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f 2024-05- > >>> 29T14:18:53.926Z|00120|unixctl|DBG|received request netdev- > >>> > dummy/receive["p1","in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:0 > >>> 0:0 > >>> 0:00:0c),eth_type(0x86dd),ipv6(src=2001:db8::1,dst=111:db8::6,proto= > >>> 1,tcl ass=0,hlimit=64,frag=no),icmpv6(type=0,code=8)"], id=0 > >>> 2024-05- 29T14:18:53.926Z|00121|unixctl|DBG|replying with success, > >>> id=0: "" > >>> 2024-05- > 29T14:18:53.926Z|00122|odp_execute_impl|ERR|Autovalidation > >>> of avx512 failed. Details: > >>> Packet: 0 > >>> Action : set(ipv6(tclass=0x40/0xfc)) Good hex: > >>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 64 00 > >>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > >>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > >>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 > >>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > >>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > >>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > >>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: > >>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 > >>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > >>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > >>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 > >>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > >>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > >>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > >>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f > >>> > >>> And > >>> > >>> 2024-05- > 29T14:18:54.503Z|00659|odp_execute_impl|ERR|Autovalidation > >>> of avx512 failed. Details: > >>> Packet: 0 > >>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) > >>> Good hex: > >>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > >>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 > >>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > >>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > >>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 > >>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 > >>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 > >>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > >>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > >>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: > >>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > >>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 > >>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > >>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > >>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 > >>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 > >>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 > >>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > >>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > >>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 2024-05- > >>> 29T14:18:54.506Z|00660|unixctl|DBG|received request netdev- > >>> > dummy/receive["n1","1e2ce92a669e3a6dd2099cab0800450000548a8340 > >>> > 0040011aadc0a80a0ac0a80a1e0800b7170a4d0002fd509a5800000000de1 > >>> > c020000000000101112131415161718191a1b1c1d1e1f20212223242526 > >>> 2728292a2b2c2d2e2f3031323334353637"], id=0 2024-05- > >>> 29T14:18:54.506Z|00661|unixctl|DBG|replying with success, id=0: "" > >>> 2024-05- > 29T14:18:54.506Z|00662|odp_execute_impl|ERR|Autovalidation > >>> of avx512 failed. Details: > >>> Packet: 0 > >>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) > >>> Good hex: > >>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > >>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 > >>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > >>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > >>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 > >>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 > >>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c > >>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > >>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > >>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: > >>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > >>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 > >>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > >>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > >>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 > >>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 > >>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c > >>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > >>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > >>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 > >>> > >>> Etc. etc. > >>> > >>> > >>> Let me know if this requires a v5 of your patch, or is in a different area? > >>> > >>>>>>> Hi Emma, > >>>>>>> > >>>>>>> Thanks for sending out the v4. I have some small nits below, > >>>>>>> which I can > >>> fix during commit time. Assuming Ilya has no other simple to fix > comments. > >>>>>>> > >>>>>>> Cheers, > >>>>>>> > >>>>>>> Eelco > >>>>>>> > >>>>>>>> Signed-off-by: Emma Finn <emma.finn@intel.com<mailto:emma.finn@intel.com>> > >>>>>>>> Reported-by: Eelco Chaudron <echaudro@redhat.com<mailto:echaudro@redhat.com>> > >>>>>>>> --- > >>>>>>>> lib/odp-execute-avx512.c | 5 ++++ > >>>>>>>> tests/dpif-netdev.at | 64 > >>> ++++++++++++++++++++++++++++++++++++++++ > >>>>>>>> 2 files changed, 69 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..260986ba9 100644 > >>>>>>>> --- a/tests/dpif-netdev.at > >>>>>>>> +++ b/tests/dpif-netdev.at > >>>>>>>> @@ -1091,3 +1091,67 @@ 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 Checksum]) > >>>>>>>> + > >>>>>>>> +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. > >>>>>>>> +]) > >>>>>>>> + > >>>>>>>> +# Add flows to trigger checksum calculation > >>>>>>> > >>>>>>> Comments should end with a dot(.). Also, not sure if ‘#’ is fine > >>>>>>> here, as we are moving to ‘dnl’, but this file has both (most are ‘#’). > Ilya? > >>>>>> > >>>>>> Both are fine, 'dnl' is a bit cleaner, so if you want to swap > >>>>>> those on commit that's fine, but there is no point in new version > >>>>>> just for that. > >>>>>> > >>>>>> Note that while backporting the fix we'll need to substitute the > >>>>>> 'compose-packet' calls with their results, since bare packet > >>>>>> compose is not available pre 3.3. > >>>>>> > >>>>>>> > >>>>>>>> +AT_DATA([flows.txt], [ddl > >>>>>>>> + 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]) > >>>>>>>> + > >>>>>>>> +# Make sure checksum won't be offloaded AT_CHECK([ovs-vsctl > >>>>>>>> +set Interface p0 options:ol_ip_csum=false]) > >>>>>>>> +AT_CHECK([ovs-vsctl set Interface p0 > >>>>>>>> +options:ol_ip_csum_set_good=false]) > >>>>>>>> + > >>>>>>>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) > >>>>>>>> + > >>>>>>>> +# IPv4 packet with values that will trigger carry-over > >>>>>>>> +addition for checksum flow_s_v4="\ > >>>>>>>> + > >>>>>>>> > >>> +eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x080 > >>>>>>>> +0,\ > >>>>>>>> + > >>>>>>>> > >>> > +nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,n > >>>>>>>> +w_frag=no,\ > >>>>>>>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" > >>>>>>>> + > >>>>>>>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") > >>>>>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) > >>>>>>>> + > >>>>>>>> +# Checksum should change to 0xAC33 with ip_src changed to > >>>>>>>> +10.1.1.1 # by the datapath while processing the packet. > >>>>>>>> +flow_expected=$(echo "${flow_s_v4}" | sed > >>>>>>>> +'s/229.167.36.90/10.1.1.1/g') good_expected=$(ovs-ofctl > >>>>>>>> +compose-packet --bare "${flow_expected}") AT_CHECK([ovs-pcap > >>>>>>>> +p1.pcap > p1.pcap.txt 2>&1]) AT_CHECK_UNQUOTED([tail -n 1 > >>>>>>>> +p1.pcap.txt], [0], [${good_expected} > >>>>>>>> +]) > >>>>>>>> + > >>>>>>>> +#Repeat similar test for IPv6 > >>>>>>> > >>>>>>> Space between # and Repeat. > >>>>>>> > >>>>>>>> +flow_s_v6="\ > >>>>>>>> + > >>>>>>>> +eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x > >>>>>>>> +86d > >>>>>>>> +d, \ > >>>>>>>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ > >>>>>>>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ > >>>>>>>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ > >>>>>>>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" > >>>>>> > >>>>>> Nit: Line continuation ('\') is not necessary within strings. > >>>>> > >>>>> Right, I can fix all this on commit. Let me add my ACK below, and > >>>>> if you have no other objections, I’ll commit? > >>>> > >>>> No objections from my side. > >>>> > >>>>> > >>>>> Acked-by: Eelco Chaudron <echaudro@redhat.com<mailto:echaudro@redhat.com>> > >>>>> > >>>>>>>> + > >>>>>>>> + > >>>>>>> A single new line is enough here. > >>>>>>> > >>>>>>>> +good_frame_v6=$(ovs-ofctl compose-packet --bare > >>>>>>>> +"${flow_s_v6}") AT_CHECK([ovs-appctl netdev-dummy/receive p0 > >>> ${good_frame_v6}]) > >>>>>>>> + > >>>>>>>> +# Checksum should change to 0x59FD with ipv6_src changed to > >>>>>>>> +fc00::100 # by the datapath while processing the packet. > >>>>>>>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ > >>>>>>>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') > >>>>>>>> +good_expected_v6=$(ovs-ofctl compose-packet --bare > >>>>>>>> +"${flow_expected_v6}") AT_CHECK([ovs-pcap p1.pcap > > >>>>>>>> +p1.pcap.txt > >>>>>>>> +2>&1]) AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], > >>>>>>>> +[${good_expected_v6} > >>>>>>>> +]) > >>>>>>>> + > >>>>>>>> +OVS_VSWITCHD_STOP > >>>>>>>> +AT_CLEANUP > >>>>>>>> -- > >>>>>>>> 2.34.1 > >>>>>>> > >>>>>
On 12 Jun 2024, at 12:42, Finn, Emma wrote: >> -----Original Message----- > >> From: Eelco Chaudron <echaudro@redhat.com> > >> Sent: Thursday, May 30, 2024 2:44 PM > >> To: Finn, Emma <emma.finn@intel.com>; Phelan, Michael > >> <michael.phelan@intel.com> > >> Cc: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Van > >> Haaren, Harry <harry.van.haaren@intel.com> > >> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. > >> > >> > >> > >> On 30 May 2024, at 15:28, Eelco Chaudron wrote: > >> > >>> On 30 May 2024, at 14:46, Finn, Emma wrote: > >>> > >>>>> -----Original Message----- > >>>>> From: Eelco Chaudron <echaudro@redhat.com<mailto:echaudro@redhat.com>> > >>>>> Sent: Wednesday, May 29, 2024 3:23 PM > >>>>> To: Finn, Emma <emma.finn@intel.com<mailto:emma.finn@intel.com>> > >>>>> Cc: Ilya Maximets <i.maximets@ovn.org<mailto:i.maximets@ovn.org>> ; ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org> ; Van > >>>>> Haaren, Harry <harry.van.haaren@intel.com<mailto:harry.van.haaren@intel.com>> > >>>>> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. > >>>>> > >>>>> > >>>>> > >>>>> On 29 May 2024, at 14:51, Ilya Maximets wrote: > >>>>> > >>>>>> On 5/29/24 11:01, Eelco Chaudron wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 28 May 2024, at 16:49, Ilya Maximets wrote: > >>>>>>> > >>>>>>>> On 5/28/24 14:36, Eelco Chaudron wrote: > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On 24 May 2024, at 11:20, 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 check the checksum carry-bits > >>>>>>>>>> issue with actions autovalidator enabled. > >>>>> > >>>>> Hi Emma, > >>>>> > >>>>> I made the small changes, and did some more testing before I committed. > >>>>> However, there are more failures in the same area with or without your > >> patch. > >>>>> I’m holding of committing this patch as it might be related. > >>>>> > >>>> > >>>> Hi Eelco, > >>>> > >>>> These tests are unrelated to this patch so I think we should go ahead and > >> merge this. > >>> > >>> Ok, I’ll go ahead and apply it later today. > >>> > >>>>> The failing tests are (on latest main branch): > >>>>> > >>>>> 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED > >>>>> (ofproto.at:6668) > >>>> > >>>> I investigated this test and the SIMD implementation isn't handling traffic > >> class field correctly. I'm on PTO for the next week but I will make a fix for this > >> once I'm back. > >>> > >>> Thanks! > >>> > >>>>> 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe > >>>>> FAILED > >>>>> (nsh.at:816) > >>>>> > >>>> For this one it looks like the scalar is expecting an ipv4 checksum of 0x000 > >> and the SIMD implementation has calculated an ipv4 checksum of 0xDF77. > >>>> This is more a logic question whether or not the checksum should be > >> calculated for this? Thoughts? > >>> > >>> I need to look at the tests, but if it’s a UDP packet, and the original UDP > >> checksum was 0, it should stay zero. > >> > >> > >> In addition, any idea why these tests do not fail in Intel’s upstream unit tests? > >> Do they use different hardware? Copied in Michael, maybe he knows more > >> about the setup/tests. > >> > >> //Eelco > >> > > > > I have investigated both unit test failures. > > 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED (ofproto.at:6668) > > For this one, the AVX implementation didn't handle setting the IPv6 traffic class field. > > > > 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED (nsh.at:816) > > For this one, the AVX implementation was missing a check for IPv4 checksum offload flag. > > I have 2 separate patches to fix these issues and will send shortly. Thanks Emma, I’ll review them next week, as I’m out at a conference (and a lot of internal meetings). > As for the Intel unit test CI (ovsrobot/intel-ovs-compilation), make check is never run with > > any of the AVX autovalidators enabled. Table below shows the 4 builds and the unit tests ran > > after each build. I guess it would be good to add the “make check” to the runs below. Michael would you be able to set this up? Thanks, Eelco > Name > > Build > > Unit tests > > ACTIONS > > ./configure --enable-actions-default-autovalidator > > make check-dpdk > > make check-system-userspace > > DPCLS > > ./configure --enable-autovalidator > > make check-dpdk > > make check-system-userspace > > DPIF > > ./configure --enable-dpif-default-avx512 > > make check-dpdk > > make check-system-userspace > > MFEX > > ./configure --enable-mfex-default-autovalidator > > make check-dpdk > > make check-system-userspace > > > > > >>>>> Here are some details: > >>>>> > >>>>> 2024-05- > >> 29T14:18:53.923Z|00119|odp_execute_impl|ERR|Autovalidation > >>>>> of avx512 failed. Details: > >>>>> Packet: 0 > >>>>> Action : set(ipv6(tclass=0x2/0x3)) > >>>>> Good hex: > >>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 20 > >>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > >>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > >>>>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 > >>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > >>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > >>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > >>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: > >>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 > >>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > >>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > >>>>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 > >>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > >>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > >>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > >>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f 2024-05- > >>>>> 29T14:18:53.926Z|00120|unixctl|DBG|received request netdev- > >>>>> > >> dummy/receive["p1","in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:0 > >>>>> 0:0 > >>>>> 0:00:0c),eth_type(0x86dd),ipv6(src=2001:db8::1,dst=111:db8::6,proto= > >>>>> 1,tcl ass=0,hlimit=64,frag=no),icmpv6(type=0,code=8)"], id=0 > >>>>> 2024-05- 29T14:18:53.926Z|00121|unixctl|DBG|replying with success, > >>>>> id=0: "" > >>>>> 2024-05- > >> 29T14:18:53.926Z|00122|odp_execute_impl|ERR|Autovalidation > >>>>> of avx512 failed. Details: > >>>>> Packet: 0 > >>>>> Action : set(ipv6(tclass=0x40/0xfc)) Good hex: > >>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 64 00 > >>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > >>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > >>>>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 > >>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > >>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > >>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > >>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: > >>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 > >>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > >>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > >>>>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 > >>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > >>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > >>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > >>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f > >>>>> > >>>>> And > >>>>> > >>>>> 2024-05- > >> 29T14:18:54.503Z|00659|odp_execute_impl|ERR|Autovalidation > >>>>> of avx512 failed. Details: > >>>>> Packet: 0 > >>>>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) > >>>>> Good hex: > >>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > >>>>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 > >>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > >>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > >>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 > >>>>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 > >>>>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 > >>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > >>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > >>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: > >>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > >>>>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 > >>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > >>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > >>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 > >>>>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 > >>>>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 > >>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > >>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > >>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 2024-05- > >>>>> 29T14:18:54.506Z|00660|unixctl|DBG|received request netdev- > >>>>> > >> dummy/receive["n1","1e2ce92a669e3a6dd2099cab0800450000548a8340 > >>>>> > >> 0040011aadc0a80a0ac0a80a1e0800b7170a4d0002fd509a5800000000de1 > >>>>> > >> c020000000000101112131415161718191a1b1c1d1e1f20212223242526 > >>>>> 2728292a2b2c2d2e2f3031323334353637"], id=0 2024-05- > >>>>> 29T14:18:54.506Z|00661|unixctl|DBG|replying with success, id=0: "" > >>>>> 2024-05- > >> 29T14:18:54.506Z|00662|odp_execute_impl|ERR|Autovalidation > >>>>> of avx512 failed. Details: > >>>>> Packet: 0 > >>>>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) > >>>>> Good hex: > >>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > >>>>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 > >>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > >>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > >>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 > >>>>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 > >>>>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c > >>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > >>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > >>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: > >>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > >>>>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 > >>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > >>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > >>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 > >>>>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 > >>>>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c > >>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > >>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > >>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 > >>>>> > >>>>> Etc. etc. > >>>>> > >>>>> > >>>>> Let me know if this requires a v5 of your patch, or is in a different area? > >>>>> > >>>>>>>>> Hi Emma, > >>>>>>>>> > >>>>>>>>> Thanks for sending out the v4. I have some small nits below, > >>>>>>>>> which I can > >>>>> fix during commit time. Assuming Ilya has no other simple to fix > >> comments. > >>>>>>>>> > >>>>>>>>> Cheers, > >>>>>>>>> > >>>>>>>>> Eelco > >>>>>>>>> > >>>>>>>>>> Signed-off-by: Emma Finn <emma.finn@intel.com<mailto:emma.finn@intel.com>> > >>>>>>>>>> Reported-by: Eelco Chaudron <echaudro@redhat.com<mailto:echaudro@redhat.com>> > >>>>>>>>>> --- > >>>>>>>>>> lib/odp-execute-avx512.c | 5 ++++ > >>>>>>>>>> tests/dpif-netdev.at | 64 > >>>>> ++++++++++++++++++++++++++++++++++++++++ > >>>>>>>>>> 2 files changed, 69 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..260986ba9 100644 > >>>>>>>>>> --- a/tests/dpif-netdev.at > >>>>>>>>>> +++ b/tests/dpif-netdev.at > >>>>>>>>>> @@ -1091,3 +1091,67 @@ 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 Checksum]) > >>>>>>>>>> + > >>>>>>>>>> +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. > >>>>>>>>>> +]) > >>>>>>>>>> + > >>>>>>>>>> +# Add flows to trigger checksum calculation > >>>>>>>>> > >>>>>>>>> Comments should end with a dot(.). Also, not sure if ‘#’ is fine > >>>>>>>>> here, as we are moving to ‘dnl’, but this file has both (most are ‘#’). > >> Ilya? > >>>>>>>> > >>>>>>>> Both are fine, 'dnl' is a bit cleaner, so if you want to swap > >>>>>>>> those on commit that's fine, but there is no point in new version > >>>>>>>> just for that. > >>>>>>>> > >>>>>>>> Note that while backporting the fix we'll need to substitute the > >>>>>>>> 'compose-packet' calls with their results, since bare packet > >>>>>>>> compose is not available pre 3.3. > >>>>>>>> > >>>>>>>>> > >>>>>>>>>> +AT_DATA([flows.txt], [ddl > >>>>>>>>>> + 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]) > >>>>>>>>>> + > >>>>>>>>>> +# Make sure checksum won't be offloaded AT_CHECK([ovs-vsctl > >>>>>>>>>> +set Interface p0 options:ol_ip_csum=false]) > >>>>>>>>>> +AT_CHECK([ovs-vsctl set Interface p0 > >>>>>>>>>> +options:ol_ip_csum_set_good=false]) > >>>>>>>>>> + > >>>>>>>>>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) > >>>>>>>>>> + > >>>>>>>>>> +# IPv4 packet with values that will trigger carry-over > >>>>>>>>>> +addition for checksum flow_s_v4="\ > >>>>>>>>>> + > >>>>>>>>>> > >>>>> +eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x080 > >>>>>>>>>> +0,\ > >>>>>>>>>> + > >>>>>>>>>> > >>>>> > >> +nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,n > >>>>>>>>>> +w_frag=no,\ > >>>>>>>>>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" > >>>>>>>>>> + > >>>>>>>>>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") > >>>>>>>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) > >>>>>>>>>> + > >>>>>>>>>> +# Checksum should change to 0xAC33 with ip_src changed to > >>>>>>>>>> +10.1.1.1 # by the datapath while processing the packet. > >>>>>>>>>> +flow_expected=$(echo "${flow_s_v4}" | sed > >>>>>>>>>> +'s/229.167.36.90/10.1.1.1/g') good_expected=$(ovs-ofctl > >>>>>>>>>> +compose-packet --bare "${flow_expected}") AT_CHECK([ovs-pcap > >>>>>>>>>> +p1.pcap > p1.pcap.txt 2>&1]) AT_CHECK_UNQUOTED([tail -n 1 > >>>>>>>>>> +p1.pcap.txt], [0], [${good_expected} > >>>>>>>>>> +]) > >>>>>>>>>> + > >>>>>>>>>> +#Repeat similar test for IPv6 > >>>>>>>>> > >>>>>>>>> Space between # and Repeat. > >>>>>>>>> > >>>>>>>>>> +flow_s_v6="\ > >>>>>>>>>> + > >>>>>>>>>> +eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x > >>>>>>>>>> +86d > >>>>>>>>>> +d, \ > >>>>>>>>>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ > >>>>>>>>>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ > >>>>>>>>>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ > >>>>>>>>>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" > >>>>>>>> > >>>>>>>> Nit: Line continuation ('\') is not necessary within strings. > >>>>>>> > >>>>>>> Right, I can fix all this on commit. Let me add my ACK below, and > >>>>>>> if you have no other objections, I’ll commit? > >>>>>> > >>>>>> No objections from my side. > >>>>>> > >>>>>>> > >>>>>>> Acked-by: Eelco Chaudron <echaudro@redhat.com<mailto:echaudro@redhat.com>> > >>>>>>> > >>>>>>>>>> + > >>>>>>>>>> + > >>>>>>>>> A single new line is enough here. > >>>>>>>>> > >>>>>>>>>> +good_frame_v6=$(ovs-ofctl compose-packet --bare > >>>>>>>>>> +"${flow_s_v6}") AT_CHECK([ovs-appctl netdev-dummy/receive p0 > >>>>> ${good_frame_v6}]) > >>>>>>>>>> + > >>>>>>>>>> +# Checksum should change to 0x59FD with ipv6_src changed to > >>>>>>>>>> +fc00::100 # by the datapath while processing the packet. > >>>>>>>>>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ > >>>>>>>>>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') > >>>>>>>>>> +good_expected_v6=$(ovs-ofctl compose-packet --bare > >>>>>>>>>> +"${flow_expected_v6}") AT_CHECK([ovs-pcap p1.pcap > > >>>>>>>>>> +p1.pcap.txt > >>>>>>>>>> +2>&1]) AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], > >>>>>>>>>> +[${good_expected_v6} > >>>>>>>>>> +]) > >>>>>>>>>> + > >>>>>>>>>> +OVS_VSWITCHD_STOP > >>>>>>>>>> +AT_CLEANUP > >>>>>>>>>> -- > >>>>>>>>>> 2.34.1 > >>>>>>>>> > >>>>>>>
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Thursday, June 13, 2024 12:45 PM > To: Finn, Emma <emma.finn@intel.com>; Phelan, Michael > <michael.phelan@intel.com> > Cc: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Van > Haaren, Harry <harry.van.haaren@intel.com> > Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. > > > > On 12 Jun 2024, at 12:42, Finn, Emma wrote: > > >> -----Original Message----- > > > >> From: Eelco Chaudron <echaudro@redhat.com> > > > >> Sent: Thursday, May 30, 2024 2:44 PM > > > >> To: Finn, Emma <emma.finn@intel.com>; Phelan, Michael > > > >> <michael.phelan@intel.com> > > > >> Cc: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Van > > > >> Haaren, Harry <harry.van.haaren@intel.com> > > > >> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. > > > >> > > > >> > > > >> > > > >> On 30 May 2024, at 15:28, Eelco Chaudron wrote: > > > >> > > > >>> On 30 May 2024, at 14:46, Finn, Emma wrote: > > > >>> > > > >>>>> -----Original Message----- > > > >>>>> From: Eelco Chaudron > <echaudro@redhat.com<mailto:echaudro@redhat.com>> > > > >>>>> Sent: Wednesday, May 29, 2024 3:23 PM > > > >>>>> To: Finn, Emma > <emma.finn@intel.com<mailto:emma.finn@intel.com>> > > > >>>>> Cc: Ilya Maximets <i.maximets@ovn.org<mailto:i.maximets@ovn.org>> > ; ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org> ; Van > > > >>>>> Haaren, Harry > <harry.van.haaren@intel.com<mailto:harry.van.haaren@intel.com>> > > > >>>>> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. > > > >>>>> > > > >>>>> > > > >>>>> > > > >>>>> On 29 May 2024, at 14:51, Ilya Maximets wrote: > > > >>>>> > > > >>>>>> On 5/29/24 11:01, Eelco Chaudron wrote: > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> On 28 May 2024, at 16:49, Ilya Maximets wrote: > > > >>>>>>> > > > >>>>>>>> On 5/28/24 14:36, Eelco Chaudron wrote: > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> On 24 May 2024, at 11:20, 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 check the checksum carry-bits > > > >>>>>>>>>> issue with actions autovalidator enabled. > > > >>>>> > > > >>>>> Hi Emma, > > > >>>>> > > > >>>>> I made the small changes, and did some more testing before I > committed. > > > >>>>> However, there are more failures in the same area with or without your > > > >> patch. > > > >>>>> I’m holding of committing this patch as it might be related. > > > >>>>> > > > >>>> > > > >>>> Hi Eelco, > > > >>>> > > > >>>> These tests are unrelated to this patch so I think we should go ahead and > > > >> merge this. > > > >>> > > > >>> Ok, I’ll go ahead and apply it later today. > > > >>> > > > >>>>> The failing tests are (on latest main branch): > > > >>>>> > > > >>>>> 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED > > > >>>>> (ofproto.at:6668) > > > >>>> > > > >>>> I investigated this test and the SIMD implementation isn't handling traffic > > > >> class field correctly. I'm on PTO for the next week but I will make a fix for this > > > >> once I'm back. > > > >>> > > > >>> Thanks! > > > >>> > > > >>>>> 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe > > > >>>>> FAILED > > > >>>>> (nsh.at:816) > > > >>>>> > > > >>>> For this one it looks like the scalar is expecting an ipv4 checksum of 0x000 > > > >> and the SIMD implementation has calculated an ipv4 checksum of 0xDF77. > > > >>>> This is more a logic question whether or not the checksum should be > > > >> calculated for this? Thoughts? > > > >>> > > > >>> I need to look at the tests, but if it’s a UDP packet, and the original UDP > > > >> checksum was 0, it should stay zero. > > > >> > > > >> > > > >> In addition, any idea why these tests do not fail in Intel’s upstream unit > tests? > > > >> Do they use different hardware? Copied in Michael, maybe he knows more > > > >> about the setup/tests. > > > >> > > > >> //Eelco > > > >> > > > > > > > > I have investigated both unit test failures. > > > > 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED > (ofproto.at:6668) > > > > For this one, the AVX implementation didn't handle setting the IPv6 traffic > class field. > > > > > > > > 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED > (nsh.at:816) > > > > For this one, the AVX implementation was missing a check for IPv4 checksum > offload flag. > > > > I have 2 separate patches to fix these issues and will send shortly. > > Thanks Emma, I’ll review them next week, as I’m out at a conference (and a lot > of internal meetings). > > > As for the Intel unit test CI (ovsrobot/intel-ovs-compilation), make check is > never run with > > > > any of the AVX autovalidators enabled. Table below shows the 4 builds and > the unit tests ran > > > > after each build. > > I guess it would be good to add the “make check” to the runs below. Michael > would you be able to set this up? Hi Eelco, Yes, I can add make check to all of the runs on Intel CI. I will set that up now. Thanks, Michael. > > Thanks, > > Eelco > > > Name > > > > Build > > > > Unit tests > > > > ACTIONS > > > > ./configure --enable-actions-default-autovalidator > > > > make check-dpdk > > > > make check-system-userspace > > > > DPCLS > > > > ./configure --enable-autovalidator > > > > make check-dpdk > > > > make check-system-userspace > > > > DPIF > > > > ./configure --enable-dpif-default-avx512 > > > > make check-dpdk > > > > make check-system-userspace > > > > MFEX > > > > ./configure --enable-mfex-default-autovalidator > > > > make check-dpdk > > > > make check-system-userspace > > > > > > > > > > > >>>>> Here are some details: > > > >>>>> > > > >>>>> 2024-05- > > > >> 29T14:18:53.923Z|00119|odp_execute_impl|ERR|Autovalidation > > > >>>>> of avx512 failed. Details: > > > >>>>> Packet: 0 > > > >>>>> Action : set(ipv6(tclass=0x2/0x3)) > > > >>>>> Good hex: > > > >>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 20 > > > >>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > > > >>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > > > >>>>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 > > > >>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > > > >>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > > > >>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > > > >>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: > > > >>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 > > > >>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > > > >>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > > > >>>>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 > > > >>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > > > >>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > > > >>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > > > >>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f 2024-05- > > > >>>>> 29T14:18:53.926Z|00120|unixctl|DBG|received request netdev- > > > >>>>> > > > >> dummy/receive["p1","in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:0 > > > >>>>> 0:0 > > > >>>>> > 0:00:0c),eth_type(0x86dd),ipv6(src=2001:db8::1,dst=111:db8::6,proto= > > > >>>>> 1,tcl ass=0,hlimit=64,frag=no),icmpv6(type=0,code=8)"], id=0 > > > >>>>> 2024-05- 29T14:18:53.926Z|00121|unixctl|DBG|replying with > success, > > > >>>>> id=0: "" > > > >>>>> 2024-05- > > > >> 29T14:18:53.926Z|00122|odp_execute_impl|ERR|Autovalidation > > > >>>>> of avx512 failed. Details: > > > >>>>> Packet: 0 > > > >>>>> Action : set(ipv6(tclass=0x40/0xfc)) Good hex: > > > >>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 64 00 > > > >>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > > > >>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > > > >>>>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 > > > >>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > > > >>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > > > >>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > > > >>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: > > > >>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 > > > >>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 > > > >>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 > > > >>>>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 > > > >>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 > > > >>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 > > > >>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 > > > >>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f > > > >>>>> > > > >>>>> And > > > >>>>> > > > >>>>> 2024-05- > > > >> 29T14:18:54.503Z|00659|odp_execute_impl|ERR|Autovalidation > > > >>>>> of avx512 failed. Details: > > > >>>>> Packet: 0 > > > >>>>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) > > > >>>>> Good hex: > > > >>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > > > >>>>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 > > > >>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > > > >>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > > > >>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 > > > >>>>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 > > > >>>>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 > > > >>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > > > >>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > > > >>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: > > > >>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > > > >>>>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 > > > >>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > > > >>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > > > >>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 > > > >>>>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 > > > >>>>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 > > > >>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > > > >>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > > > >>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 2024-05- > > > >>>>> 29T14:18:54.506Z|00660|unixctl|DBG|received request netdev- > > > >>>>> > > > >> > dummy/receive["n1","1e2ce92a669e3a6dd2099cab0800450000548a8340 > > > >>>>> > > > >> > 0040011aadc0a80a0ac0a80a1e0800b7170a4d0002fd509a5800000000de1 > > > >>>>> > > > >> > c020000000000101112131415161718191a1b1c1d1e1f20212223242526 > > > >>>>> 2728292a2b2c2d2e2f3031323334353637"], id=0 2024-05- > > > >>>>> 29T14:18:54.506Z|00661|unixctl|DBG|replying with success, id=0: "" > > > >>>>> 2024-05- > > > >> 29T14:18:54.506Z|00662|odp_execute_impl|ERR|Autovalidation > > > >>>>> of avx512 failed. Details: > > > >>>>> Packet: 0 > > > >>>>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) > > > >>>>> Good hex: > > > >>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > > > >>>>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 > > > >>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > > > >>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > > > >>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 > > > >>>>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 > > > >>>>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c > > > >>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > > > >>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > > > >>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: > > > >>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 > > > >>>>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 > > > >>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 > > > >>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 > > > >>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 > > > >>>>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 > > > >>>>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c > > > >>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 > > > >>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 > > > >>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 > > > >>>>> > > > >>>>> Etc. etc. > > > >>>>> > > > >>>>> > > > >>>>> Let me know if this requires a v5 of your patch, or is in a different area? > > > >>>>> > > > >>>>>>>>> Hi Emma, > > > >>>>>>>>> > > > >>>>>>>>> Thanks for sending out the v4. I have some small nits below, > > > >>>>>>>>> which I can > > > >>>>> fix during commit time. Assuming Ilya has no other simple to fix > > > >> comments. > > > >>>>>>>>> > > > >>>>>>>>> Cheers, > > > >>>>>>>>> > > > >>>>>>>>> Eelco > > > >>>>>>>>> > > > >>>>>>>>>> Signed-off-by: Emma Finn > <emma.finn@intel.com<mailto:emma.finn@intel.com>> > > > >>>>>>>>>> Reported-by: Eelco Chaudron > <echaudro@redhat.com<mailto:echaudro@redhat.com>> > > > >>>>>>>>>> --- > > > >>>>>>>>>> lib/odp-execute-avx512.c | 5 ++++ > > > >>>>>>>>>> tests/dpif-netdev.at | 64 > > > >>>>> ++++++++++++++++++++++++++++++++++++++++ > > > >>>>>>>>>> 2 files changed, 69 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..260986ba9 100644 > > > >>>>>>>>>> --- a/tests/dpif-netdev.at > > > >>>>>>>>>> +++ b/tests/dpif-netdev.at > > > >>>>>>>>>> @@ -1091,3 +1091,67 @@ 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 Checksum]) > > > >>>>>>>>>> + > > > >>>>>>>>>> +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. > > > >>>>>>>>>> +]) > > > >>>>>>>>>> + > > > >>>>>>>>>> +# Add flows to trigger checksum calculation > > > >>>>>>>>> > > > >>>>>>>>> Comments should end with a dot(.). Also, not sure if ‘#’ is fine > > > >>>>>>>>> here, as we are moving to ‘dnl’, but this file has both (most are ‘#’). > > > >> Ilya? > > > >>>>>>>> > > > >>>>>>>> Both are fine, 'dnl' is a bit cleaner, so if you want to swap > > > >>>>>>>> those on commit that's fine, but there is no point in new version > > > >>>>>>>> just for that. > > > >>>>>>>> > > > >>>>>>>> Note that while backporting the fix we'll need to substitute the > > > >>>>>>>> 'compose-packet' calls with their results, since bare packet > > > >>>>>>>> compose is not available pre 3.3. > > > >>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>>> +AT_DATA([flows.txt], [ddl > > > >>>>>>>>>> + 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]) > > > >>>>>>>>>> + > > > >>>>>>>>>> +# Make sure checksum won't be offloaded AT_CHECK([ovs-vsctl > > > >>>>>>>>>> +set Interface p0 options:ol_ip_csum=false]) > > > >>>>>>>>>> +AT_CHECK([ovs-vsctl set Interface p0 > > > >>>>>>>>>> +options:ol_ip_csum_set_good=false]) > > > >>>>>>>>>> + > > > >>>>>>>>>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) > > > >>>>>>>>>> + > > > >>>>>>>>>> +# IPv4 packet with values that will trigger carry-over > > > >>>>>>>>>> +addition for checksum flow_s_v4="\ > > > >>>>>>>>>> + > > > >>>>>>>>>> > > > >>>>> > +eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x080 > > > >>>>>>>>>> +0,\ > > > >>>>>>>>>> + > > > >>>>>>>>>> > > > >>>>> > > > >> > +nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,n > > > >>>>>>>>>> +w_frag=no,\ > > > >>>>>>>>>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" > > > >>>>>>>>>> + > > > >>>>>>>>>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") > > > >>>>>>>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 > ${good_frame}]) > > > >>>>>>>>>> + > > > >>>>>>>>>> +# Checksum should change to 0xAC33 with ip_src changed to > > > >>>>>>>>>> +10.1.1.1 # by the datapath while processing the packet. > > > >>>>>>>>>> +flow_expected=$(echo "${flow_s_v4}" | sed > > > >>>>>>>>>> +'s/229.167.36.90/10.1.1.1/g') good_expected=$(ovs-ofctl > > > >>>>>>>>>> +compose-packet --bare "${flow_expected}") AT_CHECK([ovs- > pcap > > > >>>>>>>>>> +p1.pcap > p1.pcap.txt 2>&1]) AT_CHECK_UNQUOTED([tail -n 1 > > > >>>>>>>>>> +p1.pcap.txt], [0], [${good_expected} > > > >>>>>>>>>> +]) > > > >>>>>>>>>> + > > > >>>>>>>>>> +#Repeat similar test for IPv6 > > > >>>>>>>>> > > > >>>>>>>>> Space between # and Repeat. > > > >>>>>>>>> > > > >>>>>>>>>> +flow_s_v6="\ > > > >>>>>>>>>> + > > > >>>>>>>>>> > +eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x > > > >>>>>>>>>> +86d > > > >>>>>>>>>> +d, \ > > > >>>>>>>>>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ > > > >>>>>>>>>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ > > > >>>>>>>>>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ > > > >>>>>>>>>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" > > > >>>>>>>> > > > >>>>>>>> Nit: Line continuation ('\') is not necessary within strings. > > > >>>>>>> > > > >>>>>>> Right, I can fix all this on commit. Let me add my ACK below, and > > > >>>>>>> if you have no other objections, I’ll commit? > > > >>>>>> > > > >>>>>> No objections from my side. > > > >>>>>> > > > >>>>>>> > > > >>>>>>> Acked-by: Eelco Chaudron > <echaudro@redhat.com<mailto:echaudro@redhat.com>> > > > >>>>>>> > > > >>>>>>>>>> + > > > >>>>>>>>>> + > > > >>>>>>>>> A single new line is enough here. > > > >>>>>>>>> > > > >>>>>>>>>> +good_frame_v6=$(ovs-ofctl compose-packet --bare > > > >>>>>>>>>> +"${flow_s_v6}") AT_CHECK([ovs-appctl netdev-dummy/receive > p0 > > > >>>>> ${good_frame_v6}]) > > > >>>>>>>>>> + > > > >>>>>>>>>> +# Checksum should change to 0x59FD with ipv6_src changed to > > > >>>>>>>>>> +fc00::100 # by the datapath while processing the packet. > > > >>>>>>>>>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ > > > >>>>>>>>>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') > > > >>>>>>>>>> +good_expected_v6=$(ovs-ofctl compose-packet --bare > > > >>>>>>>>>> +"${flow_expected_v6}") AT_CHECK([ovs-pcap p1.pcap > > > > >>>>>>>>>> +p1.pcap.txt > > > >>>>>>>>>> +2>&1]) AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], > > > >>>>>>>>>> +[${good_expected_v6} > > > >>>>>>>>>> +]) > > > >>>>>>>>>> + > > > >>>>>>>>>> +OVS_VSWITCHD_STOP > > > >>>>>>>>>> +AT_CLEANUP > > > >>>>>>>>>> -- > > > >>>>>>>>>> 2.34.1 > > > >>>>>>>>> > > > >>>>>>>
> Op 14 jun 2024 om 10:13 heeft Phelan, Michael <michael.phelan@intel.com> het volgende geschreven: > > >> >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Thursday, June 13, 2024 12:45 PM >> To: Finn, Emma <emma.finn@intel.com>; Phelan, Michael >> <michael.phelan@intel.com> >> Cc: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Van >> Haaren, Harry <harry.van.haaren@intel.com> >> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. >> >> >> >> On 12 Jun 2024, at 12:42, Finn, Emma wrote: >> >>>> -----Original Message----- >>> >>>> From: Eelco Chaudron <echaudro@redhat.com> >>> >>>> Sent: Thursday, May 30, 2024 2:44 PM >>> >>>> To: Finn, Emma <emma.finn@intel.com>; Phelan, Michael >>> >>>> <michael.phelan@intel.com> >>> >>>> Cc: Ilya Maximets <i.maximets@ovn.org>; ovs-dev@openvswitch.org; Van >>> >>>> Haaren, Harry <harry.van.haaren@intel.com> >>> >>>> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. >>> >>>> >>> >>>> >>> >>>> >>> >>>>> On 30 May 2024, at 15:28, Eelco Chaudron wrote: >>> >>>> >>> >>>>>> On 30 May 2024, at 14:46, Finn, Emma wrote: >>> >>>>> >>> >>>>>>> -----Original Message----- >>> >>>>>>> From: Eelco Chaudron >> <echaudro@redhat.com<mailto:echaudro@redhat.com>> >>> >>>>>>> Sent: Wednesday, May 29, 2024 3:23 PM >>> >>>>>>> To: Finn, Emma >> <emma.finn@intel.com<mailto:emma.finn@intel.com>> >>> >>>>>>> Cc: Ilya Maximets <i.maximets@ovn.org<mailto:i.maximets@ovn.org>> >> ; ovs-dev@openvswitch.org<mailto:ovs-dev@openvswitch.org> ; Van >>> >>>>>>> Haaren, Harry >> <harry.van.haaren@intel.com<mailto:harry.van.haaren@intel.com>> >>> >>>>>>> Subject: Re: [v4] odp-execute: Fix AVX checksum calculation. >>> >>>>>>> >>> >>>>>>> >>> >>>>>>> >>> >>>>>>>> On 29 May 2024, at 14:51, Ilya Maximets wrote: >>> >>>>>>> >>> >>>>>>>>> On 5/29/24 11:01, Eelco Chaudron wrote: >>> >>>>>>>>> >>> >>>>>>>>> >>> >>>>>>>>>> On 28 May 2024, at 16:49, Ilya Maximets wrote: >>> >>>>>>>>> >>> >>>>>>>>>>> On 5/28/24 14:36, Eelco Chaudron wrote: >>> >>>>>>>>>>> >>> >>>>>>>>>>> >>> >>>>>>>>>>>> On 24 May 2024, at 11:20, 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 check the checksum carry-bits >>> >>>>>>>>>>>> issue with actions autovalidator enabled. >>> >>>>>>> >>> >>>>>>> Hi Emma, >>> >>>>>>> >>> >>>>>>> I made the small changes, and did some more testing before I >> committed. >>> >>>>>>> However, there are more failures in the same area with or without your >>> >>>> patch. >>> >>>>>>> I’m holding of committing this patch as it might be related. >>> >>>>>>> >>> >>>>>> >>> >>>>>> Hi Eelco, >>> >>>>>> >>> >>>>>> These tests are unrelated to this patch so I think we should go ahead and >>> >>>> merge this. >>> >>>>> >>> >>>>> Ok, I’ll go ahead and apply it later today. >>> >>>>> >>> >>>>>>> The failing tests are (on latest main branch): >>> >>>>>>> >>> >>>>>>> 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED >>> >>>>>>> (ofproto.at:6668) >>> >>>>>> >>> >>>>>> I investigated this test and the SIMD implementation isn't handling traffic >>> >>>> class field correctly. I'm on PTO for the next week but I will make a fix for this >>> >>>> once I'm back. >>> >>>>> >>> >>>>> Thanks! >>> >>>>> >>> >>>>>>> 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe >>> >>>>>>> FAILED >>> >>>>>>> (nsh.at:816) >>> >>>>>>> >>> >>>>>> For this one it looks like the scalar is expecting an ipv4 checksum of 0x000 >>> >>>> and the SIMD implementation has calculated an ipv4 checksum of 0xDF77. >>> >>>>>> This is more a logic question whether or not the checksum should be >>> >>>> calculated for this? Thoughts? >>> >>>>> >>> >>>>> I need to look at the tests, but if it’s a UDP packet, and the original UDP >>> >>>> checksum was 0, it should stay zero. >>> >>>> >>> >>>> >>> >>>> In addition, any idea why these tests do not fail in Intel’s upstream unit >> tests? >>> >>>> Do they use different hardware? Copied in Michael, maybe he knows more >>> >>>> about the setup/tests. >>> >>>> >>> >>>> //Eelco >>> >>>> >>> >>> >>> >>> I have investigated both unit test failures. >>> >>> 1064: ofproto - implicit mask of ipv6 proto with HOPOPT field FAILED >> (ofproto.at:6668) >>> >>> For this one, the AVX implementation didn't handle setting the IPv6 traffic >> class field. >>> >>> >>> >>> 2615: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED >> (nsh.at:816) >>> >>> For this one, the AVX implementation was missing a check for IPv4 checksum >> offload flag. >>> >>> I have 2 separate patches to fix these issues and will send shortly. >> >> Thanks Emma, I’ll review them next week, as I’m out at a conference (and a lot >> of internal meetings). >> >>> As for the Intel unit test CI (ovsrobot/intel-ovs-compilation), make check is >> never run with >>> >>> any of the AVX autovalidators enabled. Table below shows the 4 builds and >> the unit tests ran >>> >>> after each build. >> >> I guess it would be good to add the “make check” to the runs below. Michael >> would you be able to set this up? > Hi Eelco, > Yes, I can add make check to all of the runs on Intel CI. I will set that up now. Thanks Michael for adding this. You might want to wait until the patches are in as it will fail without them. Cheers, Eelco > Thanks, > Michael. >> >> Thanks, >> >> Eelco >> >>> Name >>> >>> Build >>> >>> Unit tests >>> >>> ACTIONS >>> >>> ./configure --enable-actions-default-autovalidator >>> >>> make check-dpdk >>> >>> make check-system-userspace >>> >>> DPCLS >>> >>> ./configure --enable-autovalidator >>> >>> make check-dpdk >>> >>> make check-system-userspace >>> >>> DPIF >>> >>> ./configure --enable-dpif-default-avx512 >>> >>> make check-dpdk >>> >>> make check-system-userspace >>> >>> MFEX >>> >>> ./configure --enable-mfex-default-autovalidator >>> >>> make check-dpdk >>> >>> make check-system-userspace >>> >>> >>> >>> >>> >>>>>>> Here are some details: >>> >>>>>>> >>> >>>>>>> 2024-05- >>> >>>> 29T14:18:53.923Z|00119|odp_execute_impl|ERR|Autovalidation >>> >>>>>>> of avx512 failed. Details: >>> >>>>>>> Packet: 0 >>> >>>>>>> Action : set(ipv6(tclass=0x2/0x3)) >>> >>>>>>> Good hex: >>> >>>>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 20 >>> >>>>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >>> >>>>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >>> >>>>>>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 >>> >>>>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >>> >>>>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >>> >>>>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >>> >>>>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: >>> >>>>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 >>> >>>>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >>> >>>>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >>> >>>>>>> 00000030 00 00 00 00 00 05 00 00-1b fc 00 00 00 00 00 01 >>> >>>>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >>> >>>>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >>> >>>>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >>> >>>>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f 2024-05- >>> >>>>>>> 29T14:18:53.926Z|00120|unixctl|DBG|received request netdev- >>> >>>>>>> >>> >>>> dummy/receive["p1","in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:0 >>> >>>>>>> 0:0 >>> >>>>>>> >> 0:00:0c),eth_type(0x86dd),ipv6(src=2001:db8::1,dst=111:db8::6,proto= >>> >>>>>>> 1,tcl ass=0,hlimit=64,frag=no),icmpv6(type=0,code=8)"], id=0 >>> >>>>>>> 2024-05- 29T14:18:53.926Z|00121|unixctl|DBG|replying with >> success, >>> >>>>>>> id=0: "" >>> >>>>>>> 2024-05- >>> >>>> 29T14:18:53.926Z|00122|odp_execute_impl|ERR|Autovalidation >>> >>>>>>> of avx512 failed. Details: >>> >>>>>>> Packet: 0 >>> >>>>>>> Action : set(ipv6(tclass=0x40/0xfc)) Good hex: >>> >>>>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 64 00 >>> >>>>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >>> >>>>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >>> >>>>>>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 >>> >>>>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >>> >>>>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >>> >>>>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >>> >>>>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f Test hex: >>> >>>>>>> 00000000 50 54 00 00 00 0c 50 54-00 00 00 0b 86 dd 60 00 >>> >>>>>>> 00000010 00 00 00 48 01 40 20 01-0d b8 00 00 00 00 00 00 >>> >>>>>>> 00000020 00 00 00 00 00 01 01 11-0d b8 00 00 00 00 00 00 >>> >>>>>>> 00000030 00 00 00 00 00 06 00 00-1b fc 00 00 00 00 00 01 >>> >>>>>>> 00000040 02 03 04 05 06 07 08 09-0a 0b 0c 0d 0e 0f 10 11 >>> >>>>>>> 00000050 12 13 14 15 16 17 18 19-1a 1b 1c 1d 1e 1f 20 21 >>> >>>>>>> 00000060 22 23 24 25 26 27 28 29-2a 2b 2c 2d 2e 2f 30 31 >>> >>>>>>> 00000070 32 33 34 35 36 37 38 39-3a 3b 3c 3d 3e 3f >>> >>>>>>> >>> >>>>>>> And >>> >>>>>>> >>> >>>>>>> 2024-05- >>> >>>> 29T14:18:54.503Z|00659|odp_execute_impl|ERR|Autovalidation >>> >>>>>>> of avx512 failed. Details: >>> >>>>>>> Packet: 0 >>> >>>>>>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) >>> >>>>>>> Good hex: >>> >>>>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >>> >>>>>>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 >>> >>>>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >>> >>>>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >>> >>>>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 >>> >>>>>>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 >>> >>>>>>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 >>> >>>>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >>> >>>>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >>> >>>>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: >>> >>>>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >>> >>>>>>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 >>> >>>>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >>> >>>>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >>> >>>>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 53 >>> >>>>>>> 00000050 40 00 40 01 1a dd c0 a8-0a 0a c0 a8 0a 1e 08 00 >>> >>>>>>> 00000060 6f 20 0a 4d 00 01 fc 50-9a 58 00 00 00 00 27 15 >>> >>>>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >>> >>>>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >>> >>>>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 2024-05- >>> >>>>>>> 29T14:18:54.506Z|00660|unixctl|DBG|received request netdev- >>> >>>>>>> >>> >>>> >> dummy/receive["n1","1e2ce92a669e3a6dd2099cab0800450000548a8340 >>> >>>>>>> >>> >>>> >> 0040011aadc0a80a0ac0a80a1e0800b7170a4d0002fd509a5800000000de1 >>> >>>>>>> >>> >>>> >> c020000000000101112131415161718191a1b1c1d1e1f20212223242526 >>> >>>>>>> 2728292a2b2c2d2e2f3031323334353637"], id=0 2024-05- >>> >>>>>>> 29T14:18:54.506Z|00661|unixctl|DBG|replying with success, id=0: "" >>> >>>>>>> 2024-05- >>> >>>> 29T14:18:54.506Z|00662|odp_execute_impl|ERR|Autovalidation >>> >>>>>>> of avx512 failed. Details: >>> >>>>>>> Packet: 0 >>> >>>>>>> Action : set(ipv4(src=30.0.0.1,dst=30.0.0.3)) >>> >>>>>>> Good hex: >>> >>>>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >>> >>>>>>> 00000010 00 90 00 00 40 00 40 11-00 00 1e 00 00 01 1e 00 >>> >>>>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >>> >>>>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >>> >>>>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 >>> >>>>>>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 >>> >>>>>>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c >>> >>>>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >>> >>>>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >>> >>>>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 Test hex: >>> >>>>>>> 00000000 aa 55 00 00 00 03 aa 55-00 00 00 01 08 00 45 00 >>> >>>>>>> 00000010 00 90 00 00 40 00 40 11-d7 ff 1e 00 00 01 1e 00 >>> >>>>>>> 00000020 00 03 e8 20 12 b5 00 7c-00 00 0c 00 00 04 00 00 >>> >>>>>>> 00000030 00 00 0f c6 01 01 00 30-00 ff 00 00 00 00 00 00 >>> >>>>>>> 00000040 00 00 00 00 00 00 00 00-00 00 45 00 00 54 8a 83 >>> >>>>>>> 00000050 40 00 40 01 1a ad c0 a8-0a 0a c0 a8 0a 1e 08 00 >>> >>>>>>> 00000060 b7 17 0a 4d 00 02 fd 50-9a 58 00 00 00 00 de 1c >>> >>>>>>> 00000070 02 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 >>> >>>>>>> 00000080 1a 1b 1c 1d 1e 1f 20 21-22 23 24 25 26 27 28 29 >>> >>>>>>> 00000090 2a 2b 2c 2d 2e 2f 30 31-32 33 34 35 36 37 >>> >>>>>>> >>> >>>>>>> Etc. etc. >>> >>>>>>> >>> >>>>>>> >>> >>>>>>> Let me know if this requires a v5 of your patch, or is in a different area? >>> >>>>>>> >>> >>>>>>>>>>> Hi Emma, >>> >>>>>>>>>>> >>> >>>>>>>>>>> Thanks for sending out the v4. I have some small nits below, >>> >>>>>>>>>>> which I can >>> >>>>>>> fix during commit time. Assuming Ilya has no other simple to fix >>> >>>> comments. >>> >>>>>>>>>>> >>> >>>>>>>>>>> Cheers, >>> >>>>>>>>>>> >>> >>>>>>>>>>> Eelco >>> >>>>>>>>>>> >>> >>>>>>>>>>>> Signed-off-by: Emma Finn >> <emma.finn@intel.com<mailto:emma.finn@intel.com>> >>> >>>>>>>>>>>> Reported-by: Eelco Chaudron >> <echaudro@redhat.com<mailto:echaudro@redhat.com>> >>> >>>>>>>>>>>> --- >>> >>>>>>>>>>>> lib/odp-execute-avx512.c | 5 ++++ >>> >>>>>>>>>>>> tests/dpif-netdev.at | 64 >>> >>>>>>> ++++++++++++++++++++++++++++++++++++++++ >>> >>>>>>>>>>>> 2 files changed, 69 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..260986ba9 100644 >>> >>>>>>>>>>>> --- a/tests/dpif-netdev.at >>> >>>>>>>>>>>> +++ b/tests/dpif-netdev.at >>> >>>>>>>>>>>> @@ -1091,3 +1091,67 @@ 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 Checksum]) >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +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. >>> >>>>>>>>>>>> +]) >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +# Add flows to trigger checksum calculation >>> >>>>>>>>>>> >>> >>>>>>>>>>> Comments should end with a dot(.). Also, not sure if ‘#’ is fine >>> >>>>>>>>>>> here, as we are moving to ‘dnl’, but this file has both (most are ‘#’). >>> >>>> Ilya? >>> >>>>>>>>>> >>> >>>>>>>>>> Both are fine, 'dnl' is a bit cleaner, so if you want to swap >>> >>>>>>>>>> those on commit that's fine, but there is no point in new version >>> >>>>>>>>>> just for that. >>> >>>>>>>>>> >>> >>>>>>>>>> Note that while backporting the fix we'll need to substitute the >>> >>>>>>>>>> 'compose-packet' calls with their results, since bare packet >>> >>>>>>>>>> compose is not available pre 3.3. >>> >>>>>>>>>> >>> >>>>>>>>>>> >>> >>>>>>>>>>>> +AT_DATA([flows.txt], [ddl >>> >>>>>>>>>>>> + 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]) >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +# Make sure checksum won't be offloaded AT_CHECK([ovs-vsctl >>> >>>>>>>>>>>> +set Interface p0 options:ol_ip_csum=false]) >>> >>>>>>>>>>>> +AT_CHECK([ovs-vsctl set Interface p0 >>> >>>>>>>>>>>> +options:ol_ip_csum_set_good=false]) >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +# IPv4 packet with values that will trigger carry-over >>> >>>>>>>>>>>> +addition for checksum flow_s_v4="\ >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> >>> >>>>>>> >> +eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x080 >>> >>>>>>>>>>>> +0,\ >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> >>> >>>>>>> >>> >>>> >> +nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,n >>> >>>>>>>>>>>> +w_frag=no,\ >>> >>>>>>>>>>>> + tp_src=54392,tp_dst=5201,tcp_flags=ack" >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") >>> >>>>>>>>>>>> +AT_CHECK([ovs-appctl netdev-dummy/receive p0 >> ${good_frame}]) >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +# Checksum should change to 0xAC33 with ip_src changed to >>> >>>>>>>>>>>> +10.1.1.1 # by the datapath while processing the packet. >>> >>>>>>>>>>>> +flow_expected=$(echo "${flow_s_v4}" | sed >>> >>>>>>>>>>>> +'s/229.167.36.90/10.1.1.1/g') good_expected=$(ovs-ofctl >>> >>>>>>>>>>>> +compose-packet --bare "${flow_expected}") AT_CHECK([ovs- >> pcap >>> >>>>>>>>>>>> +p1.pcap > p1.pcap.txt 2>&1]) AT_CHECK_UNQUOTED([tail -n 1 >>> >>>>>>>>>>>> +p1.pcap.txt], [0], [${good_expected} >>> >>>>>>>>>>>> +]) >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +#Repeat similar test for IPv6 >>> >>>>>>>>>>> >>> >>>>>>>>>>> Space between # and Repeat. >>> >>>>>>>>>>> >>> >>>>>>>>>>>> +flow_s_v6="\ >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> >> +eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x >>> >>>>>>>>>>>> +86d >>> >>>>>>>>>>>> +d, \ >>> >>>>>>>>>>>> + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ >>> >>>>>>>>>>>> + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ >>> >>>>>>>>>>>> + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ >>> >>>>>>>>>>>> + tp_src=20405,tp_dst=20662,tcp_flags=ack" >>> >>>>>>>>>> >>> >>>>>>>>>> Nit: Line continuation ('\') is not necessary within strings. >>> >>>>>>>>> >>> >>>>>>>>> Right, I can fix all this on commit. Let me add my ACK below, and >>> >>>>>>>>> if you have no other objections, I’ll commit? >>> >>>>>>>> >>> >>>>>>>> No objections from my side. >>> >>>>>>>> >>> >>>>>>>>> >>> >>>>>>>>> Acked-by: Eelco Chaudron >> <echaudro@redhat.com<mailto:echaudro@redhat.com>> >>> >>>>>>>>> >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> + >>> >>>>>>>>>>> A single new line is enough here. >>> >>>>>>>>>>> >>> >>>>>>>>>>>> +good_frame_v6=$(ovs-ofctl compose-packet --bare >>> >>>>>>>>>>>> +"${flow_s_v6}") AT_CHECK([ovs-appctl netdev-dummy/receive >> p0 >>> >>>>>>> ${good_frame_v6}]) >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +# Checksum should change to 0x59FD with ipv6_src changed to >>> >>>>>>>>>>>> +fc00::100 # by the datapath while processing the packet. >>> >>>>>>>>>>>> +flow_expected_v6=$(echo "${flow_s_v6}" | \ >>> >>>>>>>>>>>> + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') >>> >>>>>>>>>>>> +good_expected_v6=$(ovs-ofctl compose-packet --bare >>> >>>>>>>>>>>> +"${flow_expected_v6}") AT_CHECK([ovs-pcap p1.pcap > >>> >>>>>>>>>>>> +p1.pcap.txt >>> >>>>>>>>>>>> +2>&1]) AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], >>> >>>>>>>>>>>> +[${good_expected_v6} >>> >>>>>>>>>>>> +]) >>> >>>>>>>>>>>> + >>> >>>>>>>>>>>> +OVS_VSWITCHD_STOP >>> >>>>>>>>>>>> +AT_CLEANUP >>> >>>>>>>>>>>> -- >>> >>>>>>>>>>>> 2.34.1 >>> >>>>>>>>>>> >>> >>>>>>>>> >
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..260986ba9 100644 --- a/tests/dpif-netdev.at +++ b/tests/dpif-netdev.at @@ -1091,3 +1091,67 @@ 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 Checksum]) + +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. +]) + +# Add flows to trigger checksum calculation +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]) + +# Make sure checksum won't be offloaded +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum=false]) +AT_CHECK([ovs-vsctl set Interface p0 options:ol_ip_csum_set_good=false]) + +AT_CHECK([ovs-vsctl set Interface p1 options:pcap=p1.pcap]) + +# IPv4 packet with values that will trigger carry-over addition for checksum +flow_s_v4="\ + eth_src=47:42:86:08:17:50,eth_dst=3e:55:b5:9e:3a:fb,dl_type=0x0800,\ + nw_src=229.167.36.90,nw_dst=130.161.64.186,nw_proto=6,nw_ttl=64,nw_frag=no,\ + tp_src=54392,tp_dst=5201,tcp_flags=ack" + +good_frame=$(ovs-ofctl compose-packet --bare "${flow_s_v4}") +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame}]) + +# Checksum should change to 0xAC33 with ip_src changed to 10.1.1.1 +# by the datapath while processing the packet. +flow_expected=$(echo "${flow_s_v4}" | sed 's/229.167.36.90/10.1.1.1/g') +good_expected=$(ovs-ofctl compose-packet --bare "${flow_expected}") +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected} +]) + +#Repeat similar test for IPv6 +flow_s_v6="\ + eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x86dd, \ + ipv6_src=2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3, \ + ipv6_dst=7287:10dd:2fb9:41d5:3eb2:2c7a:11b0:6258, \ + ipv6_label=0x51ac,nw_proto=6,nw_ttl=142,nw_frag=no, \ + tp_src=20405,tp_dst=20662,tcp_flags=ack" + + +good_frame_v6=$(ovs-ofctl compose-packet --bare "${flow_s_v6}") +AT_CHECK([ovs-appctl netdev-dummy/receive p0 ${good_frame_v6}]) + +# Checksum should change to 0x59FD with ipv6_src changed to fc00::100 +# by the datapath while processing the packet. +flow_expected_v6=$(echo "${flow_s_v6}" | \ + sed 's/2f8a:2076:3926:9e7:2d47:4bc9:9c7:17f3/fc00::100/g') +good_expected_v6=$(ovs-ofctl compose-packet --bare "${flow_expected_v6}") +AT_CHECK([ovs-pcap p1.pcap > p1.pcap.txt 2>&1]) +AT_CHECK_UNQUOTED([tail -n 1 p1.pcap.txt], [0], [${good_expected_v6} +]) + +OVS_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 check the checksum carry-bits issue with actions autovalidator enabled. 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 | 64 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+)