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 <[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.
>> +
>> +
> 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
