On 29 Nov 2022, at 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. > 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? > Have you been able to see a failure with autovalidator ? > >>> +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(). > >>> +{ >>> + const struct ovs_key_ipv6 *key, *mask; >>> + struct dp_packet *packet; >>> + >>> + a = nl_attr_get(a); >>> + key = nl_attr_get(a); >>> + mask = odp_get_key_mask(a, struct ovs_key_ipv6); >>> + >>> + /* Read the content of the key and mask in the respective registers. We >>> + * only load the size of the actual structure, which is only 40 bytes. >>> */ >>> + __m512i v_key = _mm512_maskz_loadu_epi64(0x1F, (void *) key); >>> + __m512i v_mask = _mm512_maskz_loadu_epi64(0x1F, (void *) mask); >>> + >>> + /* This shuffle mask v_shuffle, is to shuffle key and mask to match the >>> + * ip6_hdr structure layout. */ >>> + static const uint8_t ip_shuffle_mask[64] = { >>> + 0x20, 0x21, 0x22, 0x23, 0xFF, 0xFF, 0x24, 0x26, >>> + 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, >>> + 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, >>> + 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, >>> + 0x18, 0x19, 0x1A, 0x1B, 0x1C, 0x1D, 0x1E, 0x1F, >>> + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0XFF, 0xFF, 0xFF, >>> + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, >>> + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0XFF, 0xFF >>> + }; >>> + >>> + __m512i v_shuffle = _mm512_loadu_si512((void *) ip_shuffle_mask); >>> + >>> + /* This shuffle is required for key and mask to match the layout of the >>> + * ip6_hdr struct. */ >>> + __m512i v_key_shuf = _mm512_permutexvar_epi8(v_shuffle, v_key); >>> + __m512i v_mask_shuf = _mm512_permutexvar_epi8(v_shuffle, >> v_mask); >>> + >>> + /* Set the v_zero register to all zero's. */ >>> + const __m128i v_zeros = _mm_setzero_si128(); >>> + >>> + /* Set the v_all_ones register to all one's. */ >>> + const __m128i v_all_ones = _mm_cmpeq_epi16(v_zeros, v_zeros); >>> + >>> + /* Load ip6 src and dst masks respectively into 128-bit wide >>> registers. */ >>> + __m128i v_src = _mm_loadu_si128((void *) &mask->ipv6_src); >>> + __m128i v_dst = _mm_loadu_si128((void *) &mask->ipv6_dst); >>> + >>> + /* Perform a bitwise OR between src and dst registers. */ >>> + __m128i v_or = _mm_or_si128(v_src, v_dst); >>> + >>> + /* Will return true if any bit has been set in v_or, else it will >>> return >>> + * false. */ >>> + bool do_checksum = !_mm_test_all_zeros(v_or, v_all_ones); >>> + >>> + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { >>> + struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(packet); >>> + >>> + /* Load the 40 bytes of the IPv6 header. */ >>> + __m512i v_packet = _mm512_maskz_loadu_epi64(0x1F, (void *) >>> + nh); >>> + >>> + /* AND the v_pkt_mask to the packet data (v_packet). */ >>> + __m512i v_pkt_masked = _mm512_andnot_si512(v_mask_shuf, >>> + v_packet); >>> + >>> + /* OR the new addresses (v_key_shuf) with the masked packet >> addresses >>> + * (v_pkt_masked). */ >>> + __m512i v_new_hdr = _mm512_or_si512(v_key_shuf, >>> + v_pkt_masked); >>> + >>> + /* If ip6_src or ip6_dst has been modified, L4 checksum needs to >>> + * be updated. */ >>> + if (do_checksum) { >>> + uint8_t proto = nh->ip6_nxt; >>> + uint16_t delta_checksum = >> avx512_ipv6_addr_csum_delta(v_packet, >>> + >>> + v_new_hdr); >>> + >>> + if (proto == IPPROTO_UDP) { >>> + struct udp_header *uh = dp_packet_l4(packet); >>> + >>> + if (uh->udp_csum) { >>> + uint16_t old_udp_checksum = ~uh->udp_csum; >>> + uint32_t udp_checksum = old_udp_checksum + >>> + delta_checksum; >>> + >>> + udp_checksum = csum_finish(udp_checksum); >>> + >>> + if (!udp_checksum) { >>> + udp_checksum = htons(0xffff); >>> + } >>> + >>> + uh->udp_csum = udp_checksum; >>> + } >>> + } else if (proto == IPPROTO_TCP) { >>> + struct tcp_header *th = dp_packet_l4(packet); >>> + uint16_t old_tcp_checksum = ~th->tcp_csum; >>> + uint32_t tcp_checksum = old_tcp_checksum + >>> + delta_checksum; >>> + >>> + tcp_checksum = csum_finish(tcp_checksum); >>> + th->tcp_csum = tcp_checksum; >>> + } else if (proto == IPPROTO_ICMPV6) { >>> + struct icmp6_header *icmp = dp_packet_l4(packet); >>> + uint16_t old_icmp6_checksum = ~icmp->icmp6_cksum; >>> + uint32_t icmp6_checksum = old_icmp6_checksum + >>> + delta_checksum; >>> + >>> + icmp6_checksum = csum_finish(icmp6_checksum); >>> + icmp->icmp6_cksum = icmp6_checksum; >>> + } >>> + } >>> + /* Write back the modified IPv6 addresses. */ >>> + _mm512_mask_storeu_epi64((void *) nh, 0x1F, v_new_hdr); >> >> Overindented. >> > Sure, will fix this. > >>> + } >>> +} >>> +#endif /* HAVE_AVX512VBMI */ >>> + >>> static void >>> action_avx512_set_masked(struct dp_packet_batch *batch, const struct >>> nlattr *a) { @@ -514,6 +711,13 @@ action_avx512_init(struct >>> odp_execute_action_impl *self OVS_UNUSED) >>> impl_set_masked_funcs[OVS_KEY_ATTR_ETHERNET] = >> action_avx512_eth_set_addrs; >>> impl_set_masked_funcs[OVS_KEY_ATTR_IPV4] = >>> action_avx512_ipv4_set_addrs; >>> >>> +#if HAVE_AVX512VBMI >>> + if (action_avx512vbmi_isa_probe()) { >>> + impl_set_masked_funcs[OVS_KEY_ATTR_IPV6] = >>> + action_avx512_ipv6_set_addrs; >>> + } >>> +#endif >>> + >>> return 0; >>> } >>> >>> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c >>> index f80ae5a23..8b86b1e4f 100644 >>> --- a/lib/odp-execute-private.c >>> +++ b/lib/odp-execute-private.c >>> @@ -60,6 +60,23 @@ action_avx512_isa_probe(void) >>> >>> #endif >>> >>> +#if ACTION_IMPL_AVX512_CHECK && HAVE_AVX512VBMI bool >>> +action_avx512vbmi_isa_probe(void) >>> +{ >>> + if (cpu_has_isa(OVS_CPU_ISA_X86_AVX512VBMI)) { >>> + return true; >>> + } >>> + return false; >> >> Hmm, should this be just: >> return cpu_has_isa(OVS_CPU_ISA_X86_AVX512VBMI); >> ? > > Eelco, if you're okay with this change (as you asked for this function to be > changed in v4)? > I can update to the above. Yes sound good to me, guess I just didn’t see this nicer solution ;) > <SNIP> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
