RE: [RFC v2] non-temporal memcpy

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>
> 
> 22/07/2022 11:44, Morten Brørup пишет:
> >> From: Konstantin Ananyev [mailto:konstantin.v.ananyev@yandex.ru]
> >> Sent: Friday, 22 July 2022 01.20
> >>
> >> Hi Morten,
> >>
> >>> This RFC proposes a set of functions optimized for non-temporal
> >> memory copy.
> >>>
> >>> At this stage, I am asking for feedback on the concept.
> >>>
> >>> Applications sometimes data to another memory location, which is
> >>> only
> >> used
> >>> much later.
> >>> In this case, it is inefficient to pollute the data cache with the
> >> copied
> >>> data.
> >>>
> >>> An example use case (originating from a real life application):
> >>> Copying filtered packets, or the first part of them, into a capture
> >> buffer
> >>> for offline analysis.
> >>>
> >>> The purpose of these functions is to achieve a performance gain by
> >> not
> >>> polluting the cache when copying data.
> >>> Although the throughput may be improved by further optimization, I
> >>> do
> >> not
> >>> consider througput optimization relevant initially.
> >>>
> >>> The x86 non-temporal load instructions have 16 byte alignment
> >>> requirements [1], while ARM non-temporal load instructions are
> >> available with
> >>> 4 byte alignment requirements [2].
> >>> Both platforms offer non-temporal store instructions with 4 byte
> >> alignment
> >>> requirements.
> >>>
> >>> In addition to the primary function without any alignment
> >> requirements, we
> >>> also provide functions for respectivly 16 and 4 byte aligned access
> >> for
> >>> performance purposes.
> >>>
> >>> The function names resemble standard C library function names, but
> >> their
> >>> signatures are intentionally different. No need to drag legacy into
> >> it.
> >>>
> >>> NB: Don't comment on spaces for indentation; a patch will follow
> >>> DPDK
> >> coding
> >>> style and use TAB.
> >>
> >>
> >> I think there were discussions in other direction - remove
> >> rte_memcpy() completely and use memcpy() instead...
> >
> > Yes, the highly optimized rte_memcpy() implementation of memcpy() has
> become obsolete, now that modern compilers provide an efficient memcpy()
> implementation.
> >
> > It's an excellent reference, because we should learn from it, and avoid
> introducing similar mistakes with non-temporal memcpy.
> >
> >> But if we have a good use case for that, then I am positive in
> >> principle.
> >
> > The standard C library doesn't offer non-temporal memcpy(), so we need to
> implement it ourselves.
> >
> >> Though I think we need a clear use-case within dpdk for it to
> >> demonstrate perfomance gain.
> >
> > The performance gain is to avoid polluting the data cache. DPDK example
> applications, like l3fwd, are probably too primitive to measure any benefit in this
> regard.
> >
> >> Probably copying packets within pdump lib, or examples/dma. or ...
> >
> > Good point - the new functions should be used somewhere within DPDK. For
> this purpose, I will look into modifying rte_pktmbuf_copy(), which is used by
> pdump_copy(), to use non-temporal copying of the packet data.
> >
> >> Another thought - do we really need a separate inline function for
> >> each flavour?
> >> Might be just one non-inline rte_memcpy_nt(dst, src, size, flags),
> >> where flags could be combination of NT_SRC, NT_DST, and keep
> >> alignment detection/decisions to particular implementation?
> >
> > Thank you for the feedback, Konstantin.
> >
> > My answer to this suggestion gets a little longwinded...
> >
> > Looking at the DPDK pcapng library, it copies a 4 byte aligned metadata
> structure sized 28 byte. So it can do with 4 byte aligned functions.
> >
> > Our application can capture packets starting at the IP header, which is offset
> by 14 byte (Ethernet header size) from the packet buffer, so it requires 2 byte
> alignment. And thus, requiring 4 byte alignment is not acceptable.
> >
> > Our application uses 16 byte alignment in the capture buffer area, and can
> benefit from 16 byte aligned functions. Furthermore, x86 processors require 16
> byte alignment for non-temporal load instructions, so I think a 16 byte aligned
> non-temporal memcpy function should be offered.
> 
> 
> Yes, x86 needs 16B alignment for NT load/stores But that's supposed to be arch
> specific limitation, that we probably want to hide, no?
> Inside the function can check alignment of both src and dst and decide should it
> use NT load/store instructions or just do normal copy.
IMO, the normal copy should not be done by this API under any conditions. Why not let the application call memcpy/rte_memcpy when the NT copy is not applicable? It helps the programmer to understand and debug the issues much easier.

> 
> 
> > While working on these funtions, I experimented with an rte_memcpy_nt()
> taking flags, which is also my personal preference, but haven't succeed yet.
> Especially when copying a 16 byte aligned structure of only 16 byte, the
> overhead of the function call + comparing the flags + the copy loop overhead is
> significant, compared to inline code consisting of only one pair of "movntdqa
> (%rsi),%xmm0; movntdq %xmm0,(%rdi)" instructions.
> >
> > Remember that a non-inlined rte_memcpy_nt() will be called with very
> varying size, due to the typical mix of small and big packets, so branch prediction
> will not help.
> >
> > This RFC does not yet show the rte_memcpy_nt() function handling unaligned
> load/store, but it is more complex than the aligned functions. So I think the
> aligned variants are warranted - for performance reasons.
> >
> > Some of the need for exposing individual functions for different alignment
> stems from the compiler being unable to determine the alignment of the source
> and destination pointers at build time. So we need to help the compiler with this
> at build time, and thus the need for inlining the function. If we expose a bunch
> of small inline functions or a big inline function with flags seems to be a matter
> of taste.
> >
> > Thinking about it, you are probably right that exposing a single function with
> flags is better for documentation purposes and easier for other architectures to
> implement. But it still needs to be inline, for the reasons described above.
> 
> 
> Ok, my initial thought was that main use-case for it would be copying of big
> chunks of data, but from your description it might not be the case.
> Yes, for just 16/32B copy function call overhead might be way too high...
> As another alternative - would memcpy_nt_bulk() help somehow?
> It can do copying for the several src/dst pairs at once and that might help to
> amortize cost of function call.
> 
> 
> >
> >>
> >>
> >>> [1] https://www.intel.com/content/www/us/en/docs/intrinsics-
> >> guide/index.html#text=_mm_stream_load
> >>> [2] https://developer.arm.com/documentation/100076/0100/A64-
> >> Instruction-Set-Reference/A64-Floating-point-Instructions/LDNP--SIMD-
> >> and-FP-
> >>>
> >>> V2:
> >>> - Only copy from non-temporal source to non-temporal destination.
> >>>     I.e. remove the two variants with only source and/or destination
> >> being
> >>>     non-temporal.
> >>> - Do not require alignment.
> >>>     Instead, offer additional 4 and 16 byte aligned functions for
> >> performance
> >>>     purposes.
> >>> - Implemented two of the functions for x86.
> >>> - Remove memset function.
> >>>
> >>> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> >>> ---
> >>>
> >>> /**
> >>>    * @warning
> >>>    * @b EXPERIMENTAL: this API may change without prior notice.
> >>>    *
> >>>    * Copy data from non-temporal source to non-temporal destination.
> >>>    *
> >>>    * @param dst
> >>>    *   Pointer to the non-temporal destination of the data.
> >>>    *   Should be 4 byte aligned, for optimal performance.
> >>>    * @param src
> >>>    *   Pointer to the non-temporal source data.
> >>>    *   No alignment requirements.
> >>>    * @param len
> >>>    *   Number of bytes to copy.
> >>>    *   Should be be divisible by 4, for optimal performance.
> >>>    */
> >>> __rte_experimental
> >>> static __rte_always_inline
> >>> __attribute__((__nonnull__(1, 2), __access__(write_only, 1, 3),
> >> __access__(read_only, 2, 3)))
> >>> void rte_memcpy_nt(void * __rte_restrict dst, const void *
> >> __rte_restrict src, size_t len)
> >>> /* Implementation T.B.D. */
> >>>
> >>> /**
> >>>    * @warning
> >>>    * @b EXPERIMENTAL: this API may change without prior notice.
> >>>    *
> >>>    * Copy data in blocks of 16 byte from aligned non-temporal source
> >>>    * to aligned non-temporal destination.
> >>>    *
> >>>    * @param dst
> >>>    *   Pointer to the non-temporal destination of the data.
> >>>    *   Must be 16 byte aligned.
> >>>    * @param src
> >>>    *   Pointer to the non-temporal source data.
> >>>    *   Must be 16 byte aligned.
> >>>    * @param len
> >>>    *   Number of bytes to copy.
> >>>    *   Must be divisible by 16.
> >>>    */
> >>> __rte_experimental
> >>> static __rte_always_inline
> >>> __attribute__((__nonnull__(1, 2), __access__(write_only, 1, 3),
> >> __access__(read_only, 2, 3)))
> >>> void rte_memcpy_nt16a(void * __rte_restrict dst, const void *
> >> __rte_restrict src, size_t len)
> >>> {
> >>>       const void * const  end = RTE_PTR_ADD(src, len);
> >>>
> >>>       RTE_ASSERT(rte_is_aligned(dst, sizeof(__m128i)));
> >>>       RTE_ASSERT(rte_is_aligned(src, sizeof(__m128i)));
> >>>       RTE_ASSERT(rte_is_aligned(len, sizeof(__m128i)));
> >>>
> >>>       /* Copy large portion of data. */
> >>>       while (RTE_PTR_DIFF(end, src) >= 4 * sizeof(__m128i)) {
> >>>           register __m128i    xmm0, xmm1, xmm2, xmm3;
> >>>
> >>> /* Note: Workaround for _mm_stream_load_si128() not taking a const
> >> pointer as parameter. */
> >>> #pragma GCC diagnostic push
> >>> #pragma GCC diagnostic ignored "-Wdiscarded-qualifiers"
> >>>           xmm0 = _mm_stream_load_si128(RTE_PTR_ADD(src, 0 *
> >> sizeof(__m128i)));
> >>>           xmm1 = _mm_stream_load_si128(RTE_PTR_ADD(src, 1 *
> >> sizeof(__m128i)));
> >>>           xmm2 = _mm_stream_load_si128(RTE_PTR_ADD(src, 2 *
> >> sizeof(__m128i)));
> >>>           xmm3 = _mm_stream_load_si128(RTE_PTR_ADD(src, 3 *
> >> sizeof(__m128i)));
> >>> #pragma GCC diagnostic pop
> >>>           _mm_stream_si128(RTE_PTR_ADD(dst, 0 * sizeof(__m128i)),
> >> xmm0);
> >>>           _mm_stream_si128(RTE_PTR_ADD(dst, 1 * sizeof(__m128i)),
> >> xmm1);
> >>>           _mm_stream_si128(RTE_PTR_ADD(dst, 2 * sizeof(__m128i)),
> >> xmm2);
> >>>           _mm_stream_si128(RTE_PTR_ADD(dst, 3 * sizeof(__m128i)),
> >> xmm3);
> >>>           src = RTE_PTR_ADD(src, 4 * sizeof(__m128i));
> >>>           dst = RTE_PTR_ADD(dst, 4 * sizeof(__m128i));
> >>>       }
> >>>
> >>>       /* Copy remaining data. */
> >>>       while (src != end) {
> >>>           register __m128i    xmm;
> >>>
> >>> /* Note: Workaround for _mm_stream_load_si128() not taking a const
> >> pointer as parameter. */
> >>> #pragma GCC diagnostic push
> >>> #pragma GCC diagnostic ignored "-Wdiscarded-qualifiers"
> >>>           xmm = _mm_stream_load_si128(src); #pragma GCC diagnostic
> >>> pop
> >>>           _mm_stream_si128(dst, xmm);
> >>>           src = RTE_PTR_ADD(src, sizeof(__m128i));
> >>>           dst = RTE_PTR_ADD(dst, sizeof(__m128i));
> >>>       }
> >>> }
> >>>
> >>> /**
> >>>    * @warning
> >>>    * @b EXPERIMENTAL: this API may change without prior notice.
> >>>    *
> >>>    * Copy data in blocks of 4 byte from aligned non-temporal source
> >>>    * to aligned non-temporal destination.
> >>>    *
> >>>    * @param dst
> >>>    *   Pointer to the non-temporal destination of the data.
> >>>    *   Must be 4 byte aligned.
> >>>    * @param src
> >>>    *   Pointer to the non-temporal source data.
> >>>    *   Must be 4 byte aligned.
> >>>    * @param len
> >>>    *   Number of bytes to copy.
> >>>    *   Must be divisible by 4.
> >>>    */
> >>> __rte_experimental
> >>> static __rte_always_inline
> >>> __attribute__((__nonnull__(1, 2), __access__(write_only, 1, 3),
> >> __access__(read_only, 2, 3)))
> >>> void rte_memcpy_nt4a(void * __rte_restrict dst, const void *
> >> __rte_restrict src, size_t len)
> >>> {
> >>>       int32_t             buf[sizeof(__m128i) / sizeof(int32_t)]
> >> __rte_aligned(sizeof(__m128i));
> >>>       /** Address of source data, rounded down to achieve alignment.
> >> */
> >>>       const void *        srca = RTE_PTR_ALIGN_FLOOR(src,
> >> sizeof(__m128i));
> >>>       /** Address of end of source data, rounded down to achieve
> >> alignment. */
> >>>       const void * const  srcenda =
> >> RTE_PTR_ALIGN_FLOOR(RTE_PTR_ADD(src, len), sizeof(__m128i));
> >>>       const int           offset =  RTE_PTR_DIFF(src, srca) /
> >> sizeof(int32_t);
> >>>       register __m128i    xmm0;
> >>>
> >>>       RTE_ASSERT(rte_is_aligned(dst, sizeof(int32_t)));
> >>>       RTE_ASSERT(rte_is_aligned(src, sizeof(int32_t)));
> >>>       RTE_ASSERT(rte_is_aligned(len, sizeof(int32_t)));
> >>>
> >>>       if (unlikely(len == 0)) return;
> >>>
> >>>       /* Copy first, non-__m128i aligned, part of source data. */
> >>>       if (offset) {
> >>> /* Note: Workaround for _mm_stream_load_si128() not taking a const
> >> pointer as parameter. */
> >>> #pragma GCC diagnostic push
> >>> #pragma GCC diagnostic ignored "-Wdiscarded-qualifiers"
> >>>           xmm0 = _mm_stream_load_si128(srca);
> >>>           _mm_store_si128((void *)buf, xmm0); #pragma GCC diagnostic
> >>> pop
> >>>           switch (offset) {
> >>>               case 1:
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 0 *
> >> sizeof(int32_t)), buf[1]);
> >>>                   if (unlikely(len == 1 * sizeof(int32_t))) return;
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 1 *
> >> sizeof(int32_t)), buf[2]);
> >>>                   if (unlikely(len == 2 * sizeof(int32_t))) return;
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 2 *
> >> sizeof(int32_t)), buf[3]);
> >>>                   break;
> >>>               case 2:
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 0 *
> >> sizeof(int32_t)), buf[2]);
> >>>                   if (unlikely(len == 1 * sizeof(int32_t))) return;
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 1 *
> >> sizeof(int32_t)), buf[3]);
> >>>                   break;
> >>>               case 3:
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 0 *
> >> sizeof(int32_t)), buf[3]);
> >>>                   break;
> >>>           }
> >>>           srca = RTE_PTR_ADD(srca, (4 - offset) * sizeof(int32_t));
> >>>           dst = RTE_PTR_ADD(dst, (4 - offset) * sizeof(int32_t));
> >>>       }
> >>>
> >>>       /* Copy middle, __m128i aligned, part of source data. */
> >>>       while (srca != srcenda) {
> >>> /* Note: Workaround for _mm_stream_load_si128() not taking a const
> >> pointer as parameter. */
> >>> #pragma GCC diagnostic push
> >>> #pragma GCC diagnostic ignored "-Wdiscarded-qualifiers"
> >>>           xmm0 = _mm_stream_load_si128(srca); #pragma GCC diagnostic
> >>> pop
> >>>           _mm_store_si128((void *)buf, xmm0);
> >>>           _mm_stream_si32(RTE_PTR_ADD(dst, 0 * sizeof(int32_t)),
> >> buf[0]);
> >>>           _mm_stream_si32(RTE_PTR_ADD(dst, 1 * sizeof(int32_t)),
> >> buf[1]);
> >>>           _mm_stream_si32(RTE_PTR_ADD(dst, 2 * sizeof(int32_t)),
> >> buf[2]);
> >>>           _mm_stream_si32(RTE_PTR_ADD(dst, 3 * sizeof(int32_t)),
> >> buf[3]);
> >>>           srca = RTE_PTR_ADD(srca, sizeof(__m128i));
> >>>           dst = RTE_PTR_ADD(dst, 4 * sizeof(int32_t));
> >>>       }
> >>>
> >>>       /* Copy last, non-__m128i aligned, part of source data. */
> >>>       if (RTE_PTR_DIFF(srca, src) != 4) {
> >>> /* Note: Workaround for _mm_stream_load_si128() not taking a const
> >> pointer as parameter. */
> >>> #pragma GCC diagnostic push
> >>> #pragma GCC diagnostic ignored "-Wdiscarded-qualifiers"
> >>>           xmm0 = _mm_stream_load_si128(srca);
> >>>           _mm_store_si128((void *)buf, xmm0); #pragma GCC diagnostic
> >>> pop
> >>>           switch (offset) {
> >>>               case 1:
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 0 *
> >> sizeof(int32_t)), buf[0]);
> >>>                   break;
> >>>               case 2:
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 0 *
> >> sizeof(int32_t)), buf[0]);
> >>>                   if (unlikely(RTE_PTR_DIFF(srca, src) == 1 *
> >> sizeof(int32_t))) return;
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 1 *
> >> sizeof(int32_t)), buf[1]);
> >>>                   break;
> >>>               case 3:
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 0 *
> >> sizeof(int32_t)), buf[0]);
> >>>                   if (unlikely(RTE_PTR_DIFF(srca, src) == 1 *
> >> sizeof(int32_t))) return;
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 1 *
> >> sizeof(int32_t)), buf[1]);
> >>>                   if (unlikely(RTE_PTR_DIFF(srca, src) == 2 *
> >> sizeof(int32_t))) return;
> >>>                   _mm_stream_si32(RTE_PTR_ADD(dst, 2 *
> >> sizeof(int32_t)), buf[2]);
> >>>                   break;
> >>>           }
> >>>       }
> >>> }
> >>>
> >>
> >