[PATCH v2] powerpc: Remove eieio() in PowerPC IO functions

[PATCH v2] powerpc: Remove eieio() in PowerPC IO functions

[Julian Vetter]

Remove the eieio() calls in IO functions for PowerPC. While other
architectures permit prefetching, combining, and reordering, the eieio()
calls on PowerPC prevent such optimizations. This has only historical
reasons, because in the early arch/powerpc64 code also readl() had these
eieio() calls, but later went through several iterations which
subsequently removed them. While the _memcpy_fromio and ins{b,l,w}
functions stayed with the old implementation.

Signed-off-by: Julian Vetter <julian@outer-limits.org>
---
Changes for v2:
- Removed eieio() calls also from other functions in io.c
- Rephrased commit message
---
 arch/powerpc/kernel/io.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/powerpc/kernel/io.c b/arch/powerpc/kernel/io.c
index 6af535905984..d643ec4a201d 100644
--- a/arch/powerpc/kernel/io.c
+++ b/arch/powerpc/kernel/io.c
@@ -34,7 +34,6 @@ void _insb(const volatile u8 __iomem *port, void *buf, long count)
 	asm volatile("sync");
 	do {
 		tmp = *(const volatile u8 __force *)port;
-		eieio();
 		*tbuf++ = tmp;
 	} while (--count != 0);
 	asm volatile("twi 0,%0,0; isync" : : "r" (tmp));
@@ -65,7 +64,6 @@ void _insw_ns(const volatile u16 __iomem *port, void *buf, long count)
 	asm volatile("sync");
 	do {
 		tmp = *(const volatile u16 __force *)port;
-		eieio();
 		*tbuf++ = tmp;
 	} while (--count != 0);
 	asm volatile("twi 0,%0,0; isync" : : "r" (tmp));
@@ -96,7 +94,6 @@ void _insl_ns(const volatile u32 __iomem *port, void *buf, long count)
 	asm volatile("sync");
 	do {
 		tmp = *(const volatile u32 __force *)port;
-		eieio();
 		*tbuf++ = tmp;
 	} while (--count != 0);
 	asm volatile("twi 0,%0,0; isync" : : "r" (tmp));
@@ -155,21 +152,18 @@ void _memcpy_fromio(void *dest, const volatile void __iomem *src,
 	__asm__ __volatile__ ("sync" : : : "memory");
 	while(n && (!IO_CHECK_ALIGN(vsrc, 4) || !IO_CHECK_ALIGN(dest, 4))) {
 		*((u8 *)dest) = *((volatile u8 *)vsrc);
-		eieio();
 		vsrc++;
 		dest++;
 		n--;
 	}
 	while(n >= 4) {
 		*((u32 *)dest) = *((volatile u32 *)vsrc);
-		eieio();
 		vsrc += 4;
 		dest += 4;
 		n -= 4;
 	}
 	while(n) {
 		*((u8 *)dest) = *((volatile u8 *)vsrc);
-		eieio();
 		vsrc++;
 		dest++;
 		n--;
-- 
2.34.1

Re: [PATCH v2] powerpc: Remove eieio() in PowerPC IO functions

[Segher Boessenkool]

On Wed, Jan 29, 2025 at 10:45:10AM +0100, Julian Vetter wrote:
> Remove the eieio() calls in IO functions for PowerPC. While other
> architectures permit prefetching, combining, and reordering, the eieio()
> calls on PowerPC prevent such optimizations.

Yes, and it is crucial to prevent combining, it is part of the semantics
of these functions.  This is a much bigger problem on PowerPC than on
architectures which optimise memory accesses much less.  So most other
archs can get away with it much easier (but it is still completely wrong
there).

You are keeping the trap;isync things, which a) have a way bigger
performance impact, and b) are merely a debugging aid (if some i/o
access kills the system, it will be clear where that came from).  And
that isn't even the biggest thing of course, there is a heavyweight
sync in there as well.  Is there any benefit to this patch, or is it
only sabotage?


Segher

Re: [PATCH v2] powerpc: Remove eieio() in PowerPC IO functions

[Julian Vetter]

On 29.01.25 12:14, Segher Boessenkool wrote:
> On Wed, Jan 29, 2025 at 10:45:10AM +0100, Julian Vetter wrote:
>> Remove the eieio() calls in IO functions for PowerPC. While other
>> architectures permit prefetching, combining, and reordering, the eieio()
>> calls on PowerPC prevent such optimizations.
> 
> Yes, and it is crucial to prevent combining, it is part of the semantics
> of these functions.  This is a much bigger problem on PowerPC than on
> architectures which optimise memory accesses much less.  So most other
> archs can get away with it much easier (but it is still completely wrong
> there).
> 
> You are keeping the trap;isync things, which a) have a way bigger
> performance impact, and b) are merely a debugging aid (if some i/o
> access kills the system, it will be clear where that came from).  And
> that isn't even the biggest thing of course, there is a heavyweight
> sync in there as well.  Is there any benefit to this patch, or is it
> only sabotage?
>

Hello Segher,
thank you for your explanation. Yes, indeed, it was a bit rude to just 
send this patch out of the blue. I should have explained what the 
purpose of this patch was in the first place. I would like to align 
"most" arch specific implementations of memcpy_fromio, memcpy_toio, and 
memset_io, so they can all use the one from lib/iomem_copy.c instead of 
having all their own. So, I wanted to first bring the implementation on, 
e.g., powerpc closer to the "generic" one. Because I had the impression 
that it's very similar, except this eieio() after every read.
But if it's really mandatory to have these eieio() instructions, then 
the alternative be to either keep the powerpc implementation or add a 
generic '#define __io_mbr' (or different name) that is called after 
every read and resolves to eieio() on powerpc, and maybe to nothing or 
something else on other architectures.

Julian

> 
> Segher

Re: [PATCH v2] powerpc: Remove eieio() in PowerPC IO functions

[Arnd Bergmann]

On Wed, Jan 29, 2025, at 12:14, Segher Boessenkool wrote:
> On Wed, Jan 29, 2025 at 10:45:10AM +0100, Julian Vetter wrote:
>> Remove the eieio() calls in IO functions for PowerPC. While other
>> architectures permit prefetching, combining, and reordering, the eieio()
>> calls on PowerPC prevent such optimizations.
>
> Yes, and it is crucial to prevent combining, it is part of the semantics
> of these functions.  This is a much bigger problem on PowerPC than on
> architectures which optimise memory accesses much less.  So most other
> archs can get away with it much easier (but it is still completely wrong
> there).

Unfortunately it's not well documented what the actual behavior is
supposed to be across architectures, so part of the work that Julian
is doing is to make them behave consistently. My impression is that
we actually do want combining (and reordering) here in memcpy_fromio,
unless there are specific drivers that depend on the non-combining
behavior. My earlier observations towards this are:

- memcpy_fromio() is almost always used on RAM behind a bus, not MMIO
  registers.

- there has been a push towards using instruction sequences on arm64
  make sure we get the most (write) combining for the I/O string functions
  to get better performance

- There is only an eieio in powerpc memcpy_fromio() but no barrier
  inside the memcpy_toio() or memset_io() loops. If it was necessary
  to prevent combining, this would likely be for both load and store
  loops here.

- I tried to trace the history of memcpy_fromio() and found that
  it was originally just a loop around readl(), which at the time
  was eieio() and a pointer dereference and byteswap. The readl()
  definition has changed many times after that, but memcpy_fromio()
  never changed again, which makes it most likely that this was
  just forgotten in the conversion rather than intentional.

If you can think of callers of memcpy_fromio() that depend on the
eieio(), we probably need to fix other architectures as well and
go back to Julian's original idea of adding a new barrier into the
common code and have an architecture specific definition for that
barrier.

For the insb()/insw()/insl() case, I think you are correct that
the eieio is required on powerpc and likely others as well, since
the CPU combining multiple reads on a single address into a single
one would clearly break the concept.
On Arm and other architectures that prevent combining of MMIO
reads based on page table flags, we don't need a barrier here,
but there is a good chance that these barriers are in fact
missing on some alpha and some mips implementations of
readsl() etc: The alpha ioread32_rep() and insl() have barriers
inside, but it also uses the generic readsl() which does not.

> You are keeping the trap;isync things, which a) have a way bigger
> performance impact, and b) are merely a debugging aid (if some i/o
> access kills the system, it will be clear where that came from).  And
> that isn't even the biggest thing of course, there is a heavyweight
> sync in there as well.

The barriers around memcpy_toio()/memcpy_fromio() are a separate
question that we should address as well. I somehow thought that
other architectures had the same barriers as readl/writel around
the string functions, but it does seem like it's only powerpc
at this point.

Intuitively I would have expected that a memcpy_toio() etc
has the same barriers as a writel() around it for consistency, on
the other hand it's only powerpc that has these, and if the
functions are indeed only meant to work on RAM behind MMIO,
they would not not have to serialize against DMA the same way
that writel does because there are no side-effects.

I'm still looking for more insights on that, but if we can
agree that memcpy_{from,to}io don't need the outer barriers,
it seems best to address this at the same time as the internal
eieio() in memcpy_fromio(), provided we agree on removing
those as well.

      Arnd