<snip> > >>>> > >>>> 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. > >>
Thanks for the explanation. I have added a check for extension headers to mimic the scalar behaviour in the next version. > >> 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. > <snip> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
