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 <[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=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 <[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
