On 29 Nov 2022, at 17:35, Phelan, Michael wrote:
>> -----Original Message-----
>> From: Ilya Maximets <[email protected]>
>> Sent: Tuesday 29 November 2022 15:07
>> To: Finn, Emma <[email protected]>; [email protected]; Phelan,
>> Michael <[email protected]>
>> Cc: [email protected]; Eelco Chaudron <[email protected]>; Van
>> Haaren, Harry <[email protected]>; Stokes, Ian
>> <[email protected]>
>> Subject: Re: [ovs-dev] [v5] odp-execute: Add ISA implementation of
>> set_masked IPv6 action
>>
>> On 11/29/22 15:09, Finn, Emma wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ilya Maximets <[email protected]>
>>>> Sent: Friday 25 November 2022 17:22
>>>> To: Finn, Emma <[email protected]>; [email protected]
>>>> Cc: [email protected]; Eelco Chaudron <[email protected]>
>>>> Subject: Re: [ovs-dev] [v5] odp-execute: Add ISA implementation of
>>>> set_masked IPv6 action
>>>>
>>>> On 11/25/22 17:23, Emma Finn wrote:
>>>>> This commit adds support for the AVX512 implementation of the
>>>>> ipv6_set_addrs action as well as an AVX512 implementation of
>>>>> updating the L4 checksums.
>>>>>
>>>>> Signed-off-by: Emma Finn <[email protected]>
>>>>>
>>>>> ---
>>>>> v5:
>>>>> - Fixed load for ip6 src and dst mask for checksum check.
>>>>> v4:
>>>>> - Reworked and moved check for checksum outside loop.
>>>>> - Code cleanup based on review from Eelco.
>>>>> v3:
>>>>> - Added a runtime check for AVX512 vbmi.
>>>>> v2:
>>>>> - Added check for availbility of s6_addr32 field of struct in6_addr.
>>>>> - Fixed network headers for freebsd builds.
>>>>> ---
>>>>> ---
>>>>> lib/odp-execute-avx512.c | 204
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>> lib/odp-execute-private.c | 17 ++++
>>>>> lib/odp-execute-private.h | 1 +
>>>>> 3 files changed, 222 insertions(+)
>>>>
>>>> Hi, Emma. Thanks for the patch!
>>>> I didn't review the actual AVX512 code, but I have a couple of
>>>> questions and nits inline.
>>>>
>>>
>>> Thanks Ilya.
>>> My replies are inline below.
>>>
>>> <SNIP>
>>>>> +
>>>>> +/* This function performs the same operation on each packet in the
>>>>> +batch as
>>>>> + * the scalar odp_set_ipv6() function. */
>>>>
>>>> I'm not sure if that statement is correct. If you'll look at the
>>>> odp_set_ipv6() implementation and precisely at the
>>>> packet_set_ipv6() implementation, there is a check for the routing
>>>> extension header combined with the check for the fragmentation header
>>>> (packet_rh_present) to prevent writing into L4 fields that do not
>>>> exist or, in case of routing header being present, checksum should
>>>> not be updated for the destination address.
>>>>
>>>> Could you point me to the AVX512 code that is responsible for that check?
>>>>
>>> I think the AVX code is handling this case the same as scalar and also I
>> cannot reproduce a failure with the autovalidator.
>>> If I am following the scalar code correctly, you're right. If there is a
>>> routing
>> extension header present, for the dst address no checksum will happen.
>>> But similarly for src address, a checksum won't happen.
>>> As packet_update_csum128() will only do a checksum if ip6_nxt is UPD,TCP
>> or ICMPv6. Which won't be the case if any extension header is present.
>>
>> Not really, the 'proto' argument in this function is one of the results of
>> packet_rh_present() that iterates over all the extension headers and takes
>> the protocol number from the last one. So, extension headers are jumped
>> over this way.
>>
>>> Similarly in the AVX code, l4 checksum will only happen if ip6_nxt is
>> UPD,TCP or ICMPv6, i.e no extension header is present.
>>> So I think this case is covered if I'm not missing any corner cases?
>>
>> I didn't read the AVX code carefully enough to confirm that, but it is not
>> really
>> a correct behavior as extension headers should generally be just ignored
>> except for fragmentation header and the routing header. So, the logic is:
>>
>> - If the fragmentation header is present and it is a 'later'
>> fragment - skip the checksum as there is no L4 header in
>> the packet. For the 'first' fragment the checksum should
>> be re-calculated.
>>
>> - If the routing header with non-zero segments_left is present
>> then update of the destination address should not be reflected
>> in the checksum. Update of the source address should still
>> trigger the checksum update. This is because the original
>> packet checksum is calculated with the destination address
>> taken from the last segment of the routing header.
>>
>> - In all other cases, extension headers should be just ignored
>> and the checksum should be updated.
>>
>> I'm not sure if that logic is covering all the cases, but that is what
>> scalar code is
>> doing.
>>
>>> Have you been able to see a failure with autovalidator ?
>>
>> Yes, there is a failure on a system test:
>>
>> 9. system-traffic.at:229: testing datapath - ping6 between two ports with
>> header modify ...
>>
>> 2022-11-28T17:27:13.067Z|00107|dpif_lookup_avx512_gather|INFO|Using
>> non-specialized AVX512 lookup for subtable (4,5) and possibly oth ers.
>> 2022-11-28T17:27:13.389Z|00108|odp_execute_impl|ERR|Autovalidation of
>> avx512 failed. Details:
>> Packet: 0
>> Action : set(ipv6(dst=fc00::2))
>> Good hex:
>> 00000000 e4 11 22 33 44 54 e4 11-22 33 44 55 86 dd 60 06
>> 00000010 01 5d 05 b0 2c 40 fc 00-00 00 00 00 00 00 00 00
>> 00000020 00 00 00 00 00 01 fc 00-00 00 00 00 00 00 00 00
>> 00000030 00 00 00 00 00 02 3a 00-00 01 81 05 2f 0d 80 00
>> 00000040 0b bc 39 39 00 01 71 ef-84 63 00 00 00 00 13 ed
>> 00000050 05 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 <...> Test hex:
>> 00000000 e4 11 22 33 44 54 e4 11-22 33 44 55 86 dd 60 06
>> 00000010 01 5d 05 b0 2c 40 fc 00-00 00 00 00 00 00 00 00
>> 00000020 00 00 00 00 00 01 fc 00-00 00 00 00 00 00 00 00
>> 00000030 00 00 00 00 00 02 3a 00-00 01 81 05 2f 0d 80 00
>> 00000040 0b bb 39 39 00 01 71 ef-84 63 00 00 00 00 13 ed
>> 00000050 05 00 00 00 00 00 10 11-12 13 14 15 16 17 18 19 <...>
>>
>>
>> This is a fragmented ICMPv6 packet. The first fragment.
>>
>> I'm also wondering why CI didn't catch that...
>>
>> There might be 2 reasons:
>>
>> 1. Actions autovalidator is not enabled in CI, or 2. CI system doesn't have
>> avx512vbmi.
>>
>> Michael, could you check that?
>
> Hi Ilya,
> The CI system does have avx512vbmi, however, the actions autovalidator is
> never enabled for any of the tests.
>
> I could add a test to configure with the actions autovalidator if you think
> this would be a good value add for the CI?
I would suggest doing a run with and without all the avx512 auto validators
enabled at compile time.
>>
>>
>>>
>>>>> +static void
>>>>> +__attribute__((__target__("avx512vbmi")))
>>>>> +action_avx512_ipv6_set_addrs(struct dp_packet_batch *batch,
>>>>> + const struct nlattr *a)
>>>>
>>>> Name of a function is a bit confusing. Doesn't it also set tclass, proto,
>>>> etc.
>> ?
>>>>
>>> It does. Would something like action_avx512_set_ipv6() be better?
>>> As the scalar function is called packet_set_ipv6().
>>
>> Yes, that looks better. Thanks!
>>
>> Best regards, Ilya Maximets.
> Thanks,
> Michael.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
