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 <[email protected]> >>> Sent: Wednesday, May 29, 2024 3:23 PM >>> To: Finn, Emma <[email protected]> >>> Cc: Ilya Maximets <[email protected]>; [email protected]; Van >>> Haaren, Harry <[email protected]> >>> 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 <[email protected]> >>>>>>>> Reported-by: Eelco Chaudron <[email protected]> >>>>>>>> --- >>>>>>>> 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 <[email protected]> >>>>> >>>>>>>> + >>>>>>>> + >>>>>>> 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 >>>>>>> >>>>> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
