Re: [PATCH v2] arch/powerpc/include/asm/barrier.h: redefine rmb and wmb to lwsync

On 2023-02-22 20:16:10, Paul E. McKenney wrote:

On Thu, Feb 23, 2023 at 09:31:48AM +0530, Kautuk Consul wrote:

quoted

On 2023-02-22 09:47:19, Paul E. McKenney wrote:

quoted

On Wed, Feb 22, 2023 at 02:33:44PM +0530, Kautuk Consul wrote:

quoted

A link from ibm.com states:
"Ensures that all instructions preceding the call to __lwsync
 complete before any subsequent store instructions can be executed
 on the processor that executed the function. Also, it ensures that
 all load instructions preceding the call to __lwsync complete before
 any subsequent load instructions can be executed on the processor
 that executed the function. This allows you to synchronize between
 multiple processors with minimal performance impact, as __lwsync
 does not wait for confirmation from each processor."

Thats why smp_rmb() and smp_wmb() are defined to lwsync.
But this same understanding applies to parallel pipeline
execution on each PowerPC processor.
So, use the lwsync instruction for rmb() and wmb() on the PPC
architectures that support it.

Signed-off-by: Kautuk Consul <redacted>
---
 arch/powerpc/include/asm/barrier.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index b95b666f0374..e088dacc0ee8 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h

@@ -36,8 +36,15 @@
  * heavy-weight sync, so smp_wmb() can be a lighter-weight eieio.
  */
 #define __mb()   __asm__ __volatile__ ("sync" : : : "memory")
+
+/* The sub-arch has lwsync. */
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC_E500MC)
+#define __rmb() __asm__ __volatile__ ("lwsync" : : : "memory")
+#define __wmb() __asm__ __volatile__ ("lwsync" : : : "memory")

Hmmm...

Does the lwsync instruction now order both cached and uncached accesses?
Or have there been changes so that smp_rmb() and smp_wmb() get this
definition, while rmb() and wmb() still get the sync instruction?
(Not seeing this, but I could easily be missing something.)

quoted

Upfront I don't see any documentation that states that lwsync
distinguishes between cached and uncached accesses.
That's why I requested the mailing list for test results with
kernel load testing.

I suggest giving the reference manual a very careful read.  I wish I
could be more helpful, but I found that a very long time ago, and no
longer recall exactly where it was stated.

Will do that as soon as I get an opprotunity.

But maybe Michael Ellerman has a pointer?

Sure. Maybe the cached and uncached accesses in these instructions should be
spelt out more clearly for newer people like me. :-)
Thanks for your time, Paul.

							Thanx, Paul

quoted

quoted

quoted

+#else
 #define __rmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define __wmb()  __asm__ __volatile__ ("sync" : : : "memory")
+#endif
 
 /* The sub-arch has lwsync */
 #if defined(CONFIG_PPC64) || defined(CONFIG_PPC_E500MC)
-- 
2.31.1