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?
>
>>> +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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
