> -----Original Message-----
> From: Ilya Maximets <[email protected]>
> Sent: Friday 2 December 2022 14:22
> To: Finn, Emma <[email protected]>; [email protected]
> Cc: [email protected]; Van Haaren, Harry <[email protected]>;
> [email protected]; Stokes, Ian <[email protected]>
> Subject: Re: [v6] odp-execute: Add ISA implementation of set_masked IPv6
> action
>
> On 11/30/22 16:57, 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]>
>
> Hi. Thanks for the updated version!
> Could you also provide some performance numbers in the commit message?
> Performance related patches should typically have some.
>
Yes, I will add some relative performance numbers when I send out the next
version.
> Some comments inline. There is also a bug in ipv4 implementation.
>
> >
> > ---
> > v6:
> > - Added check for ipv6 extension headers.
> > 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.
> > ---
> > ---
<snip>
> > +static inline uint16_t ALWAYS_INLINE
> > +__attribute__((__target__("avx512vbmi")))
> > +avx512_ipv6_addr_csum_delta(__m512i old_header, __m512i
> new_header) {
> > + uint16_t old_delta = avx512_ipv6_sum_header(old_header);
> > + uint16_t new_delta = avx512_ipv6_sum_header(new_header);
> > + uint32_t csum_delta = ((uint16_t)~old_delta) + new_delta;
>
> Is the cast necessary here? The 'old_delta' is uint16_t. The bit inversion
> should not change the type, right?
>
Yes cast is necessary here.
Bit inversion doesn't change type but the addition with result being saved to a
32-bit does. Without cast, delta is incorrect
> > +
> > + return ~csum_finish(csum_delta);
>
> One too many spaces after 'return'.
>
> > +}
> > +
> > +/* This function performs the same operation on each packet in the
> > +batch as
> > + * the scalar odp_set_ipv6() function. */ static void
> > +__attribute__((__target__("avx512vbmi")))
> > +action_avx512_set_ipv6(struct dp_packet_batch *batch, const struct
> > +nlattr *a) {
> > + 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
>
> These are overindented. Should be moved 4 spaces to the left.
>
> > + };
> > +
> > + __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. */
> > + uint8_t proto = 0;
> > + bool rh_present;
> > +
> > + rh_present = packet_rh_present(packet, &proto, &do_checksum);
>
> Hmm, the 'do_checksum' is global for all packets in a batch.
> packet_rh_present() will overwrite the value.
>
> > +
> > + if (do_checksum) {
> > + uint16_t delta_checksum;
> > + __m512i v_new_hdr_for_cksum = v_new_hdr;
>
> Reverse x-mass tree.
>
> > +
> > + /* In case of routing header being present, checksum should
> > not be
> > + * updated for the destination address. */
> > + if (rh_present) {
> > + v_new_hdr_for_cksum = _mm512_mask_blend_epi64(0x18,
> v_new_hdr,
> > + v_packet);
> > + }
> > +
> > + delta_checksum = avx512_ipv6_addr_csum_delta(v_packet,
> > +
> > + v_new_hdr_for_cksum);
> > +
> > + 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);
>
> I think, there supposed to be a pkt_metadata_init_conn(&packet->md) call,
> otherwise we may misuse outdated conntrack connection later.
>
> Hmm, action_avx512_ipv4_set_addrs() seems to have the same problem in
> it.
>
Sure, I will send a separate patch to address the ipv4 bug.
> Comparision of the packet metadata should, probably, be added to the
> actions autovalidator.
>
Good Idea. Would just comparing packet->md.conn cover this issue for now?
Then I can add a comment that if future actions opts modify md, this check will
need to be expanded.
> You can verify that conntrack is broken by running:
>
> $ make check-system-userspace TESTSUITEFLAGS='-k negative'
>
> But adding the line 'ovs-appctl odp-execute/action-impl-set avx512' to the
> 'conntrack - negative test for recirculation optimization' test beforehand.
>
> The test doesn't fail with just autovalidator, because autovalidator doesn't
> compare packet metadata, i.e. the packet->md.conn field, and packets that
> were handled by generic scalar implementation are actually used for later
> procesing, not ones changed by the avx512 implementation.
>
<SNIP>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
