On Thu, Feb 29, 2024 at 10:24:42AM +0000, Catalin Marinas wrote:

On Wed, Feb 28, 2024 at 07:06:16PM -0400, Jason Gunthorpe wrote:

quoted

On Tue, Feb 27, 2024 at 10:37:18AM +0000, Catalin Marinas wrote:

quoted

On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:

quoted

+/*
+ * This generates a memcpy that works on a from/to address which is aligned to
+ * bits. Count is in terms of the number of bits sized quantities to copy. It
+ * optimizes to use the STR groupings when possible so that it is WC friendly.
+ */
+#define memcpy_toio_aligned(to, from, count, bits)                        \
+	({                                                                \
+		volatile u##bits __iomem *_to = to;                       \
+		const u##bits *_from = from;                              \
+		size_t _count = count;                                    \
+		const u##bits *_end_from = _from + ALIGN_DOWN(_count, 8); \
+                                                                          \
+		for (; _from < _end_from; _from += 8, _to += 8)           \
+			__const_memcpy_toio_aligned##bits(_to, _from, 8); \
+		if ((_count % 8) >= 4) {                                  \
+			__const_memcpy_toio_aligned##bits(_to, _from, 4); \
+			_from += 4;                                       \
+			_to += 4;                                         \
+		}                                                         \
+		if ((_count % 4) >= 2) {                                  \
+			__const_memcpy_toio_aligned##bits(_to, _from, 2); \
+			_from += 2;                                       \
+			_to += 2;                                         \
+		}                                                         \
+		if (_count % 2)                                           \
+			__const_memcpy_toio_aligned##bits(_to, _from, 1); \
+	})

Do we actually need all this if count is not constant? If it's not
performance critical anywhere, I'd rather copy the generic
implementation, it's easier to read.

Which generic version?

The current __iowriteXX_copy() in lib/iomap_copy.c (copy them over or
add some preprocessor reuse the generic functions).

That just loops over 64 bit quantities - we know that doesn't work?

quoted

The point is to maximize WC effects with non-constant values, so I
think we do need something like this. ie we can't just fall back to
looping over 64 bit stores one at a time.

If that's a case you are also targeting and have seen it in practice,
that's fine. But I had the impression that you are mostly after the
constant count case which is already addressed by the other part of this
patch. For the non-constant case, we have a DGH only at the end of
whatever buffer was copied rather than after every 64-byte increments
you'd get for a count of 8.

mlx5 uses only the constant case. From my looking most places were
using the constant path.

However, from an API perspective, we know we need these runs of stores
for the CPU to work properly so it doesn't make any sense that the
same function called with a constant length would have good WC and the
very same function called with a variable length would have bad WC. I
would expect them to behave the same.

This is what the above does, if you pass in non-constant 64 or 32 you
get the same instruction sequence out of line as constant 64 or 32
length generates in-line. I think it is important to work like this
for basic sanity.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel