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.f...@intel.com>
>>>> Reported-by: Eelco Chaudron <echau...@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 <echau...@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
>>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
