> -----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?
>
>
> >
> >>> +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
