On 12/5/22 12:53, Finn, Emma wrote:
>
>
>> -----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.
Thanks!
>
>> 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
Hmm, OK. Please, add a space between the cast and the inversion then,
as Eelco suggested in his diff for v4.
>
>>> +
>>> + 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.
I think, the point of autovalidator is to catch differences that
we can miss while working on a change. So, unless it's checking
everything, it doesn't make a lot of sense.
I'd suggest to expand the autovalidator check in the ipv4 bug fix
patch and make it compare the whole metadata.
Since the metadata gets directly copied with memcpy by the
autovalidator, we may get away with just memcmp... ? Unless we
want some more detailed report.
Make sure to use ds_put_sparse_hex_dump() instead of regular hex
dump while dumping big chunks of metadata though. It is huge and
mostly contains zeroes, there is no point in printing it out in
full.
>
>> 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
