Re: [PATCH 4/6] arm64/io: Provide a WC friendly __iowriteXX_copy()

[Jason Gunthorpe <jgg@nvidia.com>]

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:
> > On Tue, Feb 27, 2024 at 10:37:18AM +0000, Catalin Marinas wrote:
> > > On Tue, Feb 20, 2024 at 09:17:08PM -0400, Jason Gunthorpe wrote:
> > > > +/*
> > > > + * 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?

> > 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