Skip to contentSkip to conversation

Add support for pldw instruction on v7 MP cores

PATCH Superseded Will Deacon 5 participants 30 messages Jul 23, 2013 – Jul 26, 2013

Subsystems (3)
Arm (32-bit) Locking primitives Atomic infrastructure

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Will Deacon

author
#0 lore raw

cover Add support for pldw instruction on v7 MP cores

Hello,

This patch series adds support for the pldw instruction implemented on ARMv7 SMP processors. The instruction is used to implement the prefetchw macro, which is then used around barrier-less ldrex/strex sections (i.e. the prefetch isn't blocked either side by memory barriers) to increase the chances of the strex succeeding initially.

On my TC2 board (A7s + A15s), I see a ~1.3% boost in hackbench scores on top of my barriers patch series.

All feedback welcome,

Will

Will Deacon (6):
ARM: prefetch: remove redundant "cc" clobber
ARM: smp_on_up: move inline asm ALT_SMP patching macro out of
spinlock.h
ARM: prefetch: add support for prefetchw using pldw on SMP ARMv7+ CPUs
ARM: locks: prefetch the destination word for write prior to strex
ARM: atomics: prefetch the destination word for write prior to strex
ARM: bitops: prefetch the destination word for write prior to strex

 arch/arm/include/asm/atomic.h    |  7 +++++++
 arch/arm/include/asm/processor.h | 33 +++++++++++++++++++++++++--------
 arch/arm/include/asm/spinlock.h  | 24 ++++++++++++------------
 arch/arm/include/asm/unified.h   |  4 ++++
 arch/arm/lib/bitops.h            |  5 +++++
 5 files changed, 53 insertions(+), 20 deletions(-)
Signature · 1 line
1.8.2.2
author
#1 lore raw

PATCH 1/6 ARM: prefetch: remove redundant "cc" clobber

The pld instruction does not affect the condition flags, so don't bother clobbering them.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/processor.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
arch/arm/include/asm/processor.h +1 -3
index 06e7d50..91cfe08 100644
@@ -101,9 +101,7 @@ static inline void prefetch(const void *ptr)
 {
 	__asm__ __volatile__(
 		"pld\t%a0"
-		:
-		: "p" (ptr)
-		: "cc");
+		:: "p" (ptr));
 }
 
 #define ARCH_HAS_PREFETCHW
Signature · 1 line
1.8.2.2
#2 lore raw

PATCH 1/6 ARM: prefetch: remove redundant "cc" clobber

Quoted 4 lines · Will Deacon

The pld instruction does not affect the condition flags, so don't bother clobbering them.

Signed-off-by Will Deacon <[email protected]>
Acked-by Nicolas Pitre <[email protected]>

You know the legacy reason why the cc clobber was there, right? It certainly doesn't apply anymore.

Quoted 28 lines
---
 arch/arm/include/asm/processor.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
arch/arm/include/asm/processor.h +1 -3
index 06e7d50..91cfe08 100644
@@ -101,9 +101,7 @@ static inline void prefetch(const void *ptr)
 {
 	__asm__ __volatile__(
 		"pld\t%a0"
-		:
-		: "p" (ptr)
-		: "cc");
+		:: "p" (ptr));
 }
 
 #define ARCH_HAS_PREFETCHW
Signature · 7 lines
1.8.2.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author
#3 lore raw

PATCH 1/6 ARM: prefetch: remove redundant "cc" clobber

Hi Nicolas,

Quoted 8 lines · Nicolas Pitre
Quoted 4 lines · Will Deacon

The pld instruction does not affect the condition flags, so don't bother clobbering them.

Signed-off-by Will Deacon <[email protected]>
Acked-by Nicolas Pitre <[email protected]>

Thanks for reviewing the series, much appreciated.

Quoted 2 lines

You know the legacy reason why the cc clobber was there, right? It certainly doesn't apply anymore.

I was under the impression that it was due to ancient GCC behaviour and the "cc" was there to prevent re-ordering. For interest, what are the specifics?

Cheers,

Will

#4 lore raw

PATCH 1/6 ARM: prefetch: remove redundant "cc" clobber

Quoted 12 lines · Will Deacon

Hi Nicolas,

Quoted 8 lines · Nicolas Pitre
Quoted 4 lines · Will Deacon

The pld instruction does not affect the condition flags, so don't bother clobbering them.

Signed-off-by Will Deacon <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Quoted 7 lines

Thanks for reviewing the series, much appreciated.

Quoted 2 lines

You know the legacy reason why the cc clobber was there, right? It certainly doesn't apply anymore.

I was under the impression that it was due to ancient GCC behaviour and the "cc" was there to prevent re-ordering. For interest, what are the specifics?

In the old days, you had to take care of the predicate for conditional instructions in your inline asm code. So for example, if you had:

	if (foo)
		inline asm ("mov %0, #1" : "=r" (bar));

That wouldn't work most of the time because even if the if condition was false, gcc would not branch over the inline asm code but expect you to take care of the condition code. That means the above needed to be written as:

	if (foo)
		inline asm ("mov%? %0, #1" : "=r" (bar));

Because having %? added all over the place was cumbersome, the alternative was to add "cc" to the clobber list so gcc would then branch across the inline asm when the condition is false.

But of course many people forgot about those subtle details quite often, and then gcc was changed so inline asm was made non predicated.

Nicolas

author
#5 lore raw

PATCH 1/6 ARM: prefetch: remove redundant "cc" clobber

Quoted 22 lines · Nicolas Pitre
Quoted 6 lines · Will Deacon
Quoted 2 lines · Nicolas Pitre

You know the legacy reason why the cc clobber was there, right? It certainly doesn't apply anymore.

I was under the impression that it was due to ancient GCC behaviour and the "cc" was there to prevent re-ordering. For interest, what are the specifics?

In the old days, you had to take care of the predicate for conditional instructions in your inline asm code. So for example, if you had:

	if (foo)
		inline asm ("mov %0, #1" : "=r" (bar));

That wouldn't work most of the time because even if the if condition was false, gcc would not branch over the inline asm code but expect you to take care of the condition code. That means the above needed to be written as:

	if (foo)
		inline asm ("mov%? %0, #1" : "=r" (bar));
Quoted 6 lines

Because having %? added all over the place was cumbersome, the alternative was to add "cc" to the clobber list so gcc would then branch across the inline asm when the condition is false.

But of course many people forgot about those subtle details quite often, and then gcc was changed so inline asm was made non predicated.

Holy sh*t....

Next time I complain to the tools guys about inline asm, I'll count my blessings so they don't try and resurrect anything as horrendous as what you've just described.

Thanks for the insight!

Will

#6 lore raw

PATCH 1/6 ARM: prefetch: remove redundant "cc" clobber

Quoted 27 lines · Will Deacon
Quoted 26 lines · Nicolas Pitre
Quoted 6 lines · Will Deacon
Quoted 2 lines · Nicolas Pitre

You know the legacy reason why the cc clobber was there, right? It certainly doesn't apply anymore.

I was under the impression that it was due to ancient GCC behaviour and the "cc" was there to prevent re-ordering. For interest, what are the specifics?

In the old days, you had to take care of the predicate for conditional instructions in your inline asm code. So for example, if you had:

	if (foo)
		inline asm ("mov %0, #1" : "=r" (bar));

That wouldn't work most of the time because even if the if condition was false, gcc would not branch over the inline asm code but expect you to take care of the condition code. That means the above needed to be written as:

	if (foo)
		inline asm ("mov%? %0, #1" : "=r" (bar));

Because having %? added all over the place was cumbersome, the alternative was to add "cc" to the clobber list so gcc would then branch across the inline asm when the condition is false.

Quoted 8 lines
Quoted 2 lines

But of course many people forgot about those subtle details quite often, and then gcc was changed so inline asm was made non predicated.

Holy sh*t....

Next time I complain to the tools guys about inline asm, I'll count my blessings so they don't try and resurrect anything as horrendous as what you've just described.

And the result of its removal is all the horrid macro crap in asm/tlbflush.h to work around the lack of this feature - otherwise we end up with seven instances of:

	tst
	bne	1f
	mcr

1:

for every call to a TLB function. GCC _could_ have detected the lack of %? and the lack of "cc" clobber and either warned or implicitly branched around the block. There was no need to completely remove the facility.

So now, instead, we have to do lots of preprocessor junk to work out what instructions are always executed, which are never executed, and which might be executed. All that additional complexity thanks to the removal of this brilliant feature.

That was a truely sad day when that happened.

#7 lore raw

PATCH 1/6 ARM: prefetch: remove redundant "cc" clobber

Quoted 41 lines · Russell King - ARM Linux
Quoted 27 lines · Will Deacon
Quoted 26 lines · Nicolas Pitre
Quoted 6 lines · Will Deacon
Quoted 2 lines · Nicolas Pitre

You know the legacy reason why the cc clobber was there, right? It certainly doesn't apply anymore.

I was under the impression that it was due to ancient GCC behaviour and the "cc" was there to prevent re-ordering. For interest, what are the specifics?

In the old days, you had to take care of the predicate for conditional instructions in your inline asm code. So for example, if you had:

	if (foo)
		inline asm ("mov %0, #1" : "=r" (bar));

That wouldn't work most of the time because even if the if condition was false, gcc would not branch over the inline asm code but expect you to take care of the condition code. That means the above needed to be written as:

	if (foo)
		inline asm ("mov%? %0, #1" : "=r" (bar));

Because having %? added all over the place was cumbersome, the alternative was to add "cc" to the clobber list so gcc would then branch across the inline asm when the condition is false.

Quoted 8 lines
Quoted 2 lines

But of course many people forgot about those subtle details quite often, and then gcc was changed so inline asm was made non predicated.

Holy sh*t....

Next time I complain to the tools guys about inline asm, I'll count my blessings so they don't try and resurrect anything as horrendous as what you've just described.

And the result of its removal is all the horrid macro crap in asm/tlbflush.h to work around the lack of this feature - otherwise we end up with seven instances of:

Quoted 8 lines
	tst
	bne	1f
	mcr

1:

for every call to a TLB function. GCC _could_ have detected the lack of %? and the lack of "cc" clobber and either warned or implicitly branched around the block. There was no need to completely remove the facility.

I agree. gcc could have considered an inline asm block predicate safe whenever it contains a %?. I even suggested this at the time.

Nicolas

author
#8 lore raw

PATCH 2/6 ARM: smp_on_up: move inline asm ALT_SMP patching macro out of spinlock.h

In reply to Will Deacon

Patching UP/SMP alternatives inside inline assembly blocks is useful outside of the spinlock implementation, where it is used for sev and wfe.

This patch lifts the macro into processor.h and gives it a scarier name to (a) avoid conflicts in the global namespace and (b) to try and deter its usage unless you "know what you're doing". The W macro for generating wide instructions when targetting Thumb-2 is also made available under the name WASM, to reduce the potential for conflicts with other headers.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/processor.h | 12 ++++++++++++
 arch/arm/include/asm/spinlock.h  | 15 ++++-----------
 arch/arm/include/asm/unified.h   |  4 ++++
 3 files changed, 20 insertions(+), 11 deletions(-)
arch/arm/include/asm/processor.h +12 -0
index 91cfe08..cbdb130 100644
@@ -22,6 +22,7 @@
 #include <asm/hw_breakpoint.h>
 #include <asm/ptrace.h>
 #include <asm/types.h>
+#include <asm/unified.h>
 
 #ifdef __KERNEL__
 #define STACK_TOP	((current->personality & ADDR_LIMIT_32BIT) ? \

@@ -91,6 +92,17 @@ unsigned long get_wchan(struct task_struct *p);
 #define KSTK_EIP(tsk)	task_pt_regs(tsk)->ARM_pc
 #define KSTK_ESP(tsk)	task_pt_regs(tsk)->ARM_sp
 
+#ifdef CONFIG_SMP
+#define __ALT_SMP_ASM(smp, up)						\
+	"9998:	" smp "\n"						\
+	"	.pushsection \".alt.smp.init\", \"a\"\n"		\
+	"	.long	9998b\n"					\
+	"	" up "\n"						\
+	"	.popsection\n"
+#else
+#define __ALT_SMP_ASM(smp, up)	up
+#endif
+
 /*
  * Prefetching support - only ARMv5.
  */
arch/arm/include/asm/spinlock.h +4 -11
index f8b8965..0de7bec 100644
@@ -11,15 +11,7 @@
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
  * extensions, so when running on UP, we have to patch these instructions away.
  */
-#define ALT_SMP(smp, up)					\
-	"9998:	" smp "\n"					\
-	"	.pushsection \".alt.smp.init\", \"a\"\n"	\
-	"	.long	9998b\n"				\
-	"	" up "\n"					\
-	"	.popsection\n"
-
 #ifdef CONFIG_THUMB2_KERNEL
-#define SEV		ALT_SMP("sev.w", "nop.w")
 /*
  * For Thumb-2, special care is needed to ensure that the conditional WFE
  * instruction really does assemble to exactly 4 bytes (as required by

@@ -31,17 +23,18 @@
  * the assembler won't change IT instructions which are explicitly present
  * in the input.
  */
-#define WFE(cond)	ALT_SMP(		\
+#define WFE(cond)	__ALT_SMP_ASM(		\
 	"it " cond "\n\t"			\
 	"wfe" cond ".n",			\
 						\
 	"nop.w"					\
 )
 #else
-#define SEV		ALT_SMP("sev", "nop")
-#define WFE(cond)	ALT_SMP("wfe" cond, "nop")
+#define WFE(cond)	__ALT_SMP_ASM("wfe" cond, "nop")
 #endif
 
+#define SEV		__ALT_SMP_ASM(WASM(sev), WASM(nop))
+
 static inline void dsb_sev(void)
 {
 #if __LINUX_ARM_ARCH__ >= 7
arch/arm/include/asm/unified.h +4 -0
index f5989f4..b88beab 100644
@@ -38,6 +38,8 @@
 #ifdef __ASSEMBLY__
 #define W(instr)	instr.w
 #define BSYM(sym)	sym + 1
+#else
+#define WASM(instr)	#instr ".w"
 #endif
 
 #else	/* !CONFIG_THUMB2_KERNEL */

@@ -50,6 +52,8 @@
 #ifdef __ASSEMBLY__
 #define W(instr)	instr
 #define BSYM(sym)	sym
+#else
+#define WASM(instr)	#instr
 #endif
 
 #endif	/* CONFIG_THUMB2_KERNEL */
Signature · 1 line
1.8.2.2
#9 lore raw

PATCH 2/6 ARM: smp_on_up: move inline asm ALT_SMP patching macro out of spinlock.h

Quoted 10 lines · Will Deacon

Patching UP/SMP alternatives inside inline assembly blocks is useful outside of the spinlock implementation, where it is used for sev and wfe.

This patch lifts the macro into processor.h and gives it a scarier name to (a) avoid conflicts in the global namespace and (b) to try and deter its usage unless you "know what you're doing". The W macro for generating wide instructions when targetting Thumb-2 is also made available under the name WASM, to reduce the potential for conflicts with other headers.

Signed-off-by Will Deacon <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Quoted 109 lines
---
 arch/arm/include/asm/processor.h | 12 ++++++++++++
 arch/arm/include/asm/spinlock.h  | 15 ++++-----------
 arch/arm/include/asm/unified.h   |  4 ++++
 3 files changed, 20 insertions(+), 11 deletions(-)
arch/arm/include/asm/processor.h +12 -0
index 91cfe08..cbdb130 100644
@@ -22,6 +22,7 @@
 #include <asm/hw_breakpoint.h>
 #include <asm/ptrace.h>
 #include <asm/types.h>
+#include <asm/unified.h>
 
 #ifdef __KERNEL__
 #define STACK_TOP	((current->personality & ADDR_LIMIT_32BIT) ? \

@@ -91,6 +92,17 @@ unsigned long get_wchan(struct task_struct *p);
 #define KSTK_EIP(tsk)	task_pt_regs(tsk)->ARM_pc
 #define KSTK_ESP(tsk)	task_pt_regs(tsk)->ARM_sp
 
+#ifdef CONFIG_SMP
+#define __ALT_SMP_ASM(smp, up)						\
+	"9998:	" smp "\n"						\
+	"	.pushsection \".alt.smp.init\", \"a\"\n"		\
+	"	.long	9998b\n"					\
+	"	" up "\n"						\
+	"	.popsection\n"
+#else
+#define __ALT_SMP_ASM(smp, up)	up
+#endif
+
 /*
  * Prefetching support - only ARMv5.
  */
arch/arm/include/asm/spinlock.h +4 -11
index f8b8965..0de7bec 100644
@@ -11,15 +11,7 @@
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
  * extensions, so when running on UP, we have to patch these instructions away.
  */
-#define ALT_SMP(smp, up)					\
-	"9998:	" smp "\n"					\
-	"	.pushsection \".alt.smp.init\", \"a\"\n"	\
-	"	.long	9998b\n"				\
-	"	" up "\n"					\
-	"	.popsection\n"
-
 #ifdef CONFIG_THUMB2_KERNEL
-#define SEV		ALT_SMP("sev.w", "nop.w")
 /*
  * For Thumb-2, special care is needed to ensure that the conditional WFE
  * instruction really does assemble to exactly 4 bytes (as required by

@@ -31,17 +23,18 @@
  * the assembler won't change IT instructions which are explicitly present
  * in the input.
  */
-#define WFE(cond)	ALT_SMP(		\
+#define WFE(cond)	__ALT_SMP_ASM(		\
 	"it " cond "\n\t"			\
 	"wfe" cond ".n",			\
 						\
 	"nop.w"					\
 )
 #else
-#define SEV		ALT_SMP("sev", "nop")
-#define WFE(cond)	ALT_SMP("wfe" cond, "nop")
+#define WFE(cond)	__ALT_SMP_ASM("wfe" cond, "nop")
 #endif
 
+#define SEV		__ALT_SMP_ASM(WASM(sev), WASM(nop))
+
 static inline void dsb_sev(void)
 {
 #if __LINUX_ARM_ARCH__ >= 7
arch/arm/include/asm/unified.h +4 -0
index f5989f4..b88beab 100644
@@ -38,6 +38,8 @@
 #ifdef __ASSEMBLY__
 #define W(instr)	instr.w
 #define BSYM(sym)	sym + 1
+#else
+#define WASM(instr)	#instr ".w"
 #endif
 
 #else	/* !CONFIG_THUMB2_KERNEL */

@@ -50,6 +52,8 @@
 #ifdef __ASSEMBLY__
 #define W(instr)	instr
 #define BSYM(sym)	sym
+#else
+#define WASM(instr)	#instr
 #endif
 
 #endif	/* CONFIG_THUMB2_KERNEL */
Signature · 7 lines
1.8.2.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author
#10 lore raw

PATCH 3/6 ARM: prefetch: add support for prefetchw using pldw on SMP ARMv7+ CPUs

In reply to Will Deacon

SMP ARMv7 CPUs implement the pldw instruction, which allows them to prefetch data cachelines in an exclusive state.

This patch defines the prefetchw macro using pldw for CPUs that support it.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/processor.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
arch/arm/include/asm/processor.h +12 -5
index cbdb130..d571697 100644
@@ -116,12 +116,19 @@ static inline void prefetch(const void *ptr)
 		:: "p" (ptr));
 }
 
+#if __LINUX_ARM_ARCH__ >= 7 && defined(CONFIG_SMP)
 #define ARCH_HAS_PREFETCHW
-#define prefetchw(ptr)	prefetch(ptr)
-
-#define ARCH_HAS_SPINLOCK_PREFETCH
-#define spin_lock_prefetch(x) do { } while (0)
-
+static inline void prefetchw(const void *ptr)
+{
+	__asm__ __volatile__(
+		".arch_extension	mp\n"
+	__ALT_SMP_ASM(
+		WASM(pldw)		"\t%a0",
+		WASM(pld)		"\t%a0"
+	)
+		:: "p" (ptr));
+}
+#endif
 #endif
 
 #define HAVE_ARCH_PICK_MMAP_LAYOUT
Signature · 1 line
1.8.2.2
#11 lore raw

PATCH 3/6 ARM: prefetch: add support for prefetchw using pldw on SMP ARMv7+ CPUs

Quoted 7 lines · Will Deacon

SMP ARMv7 CPUs implement the pldw instruction, which allows them to prefetch data cachelines in an exclusive state.

This patch defines the prefetchw macro using pldw for CPUs that support it.

Signed-off-by Will Deacon <[email protected]>

Minor nit below. Otherwise:

Acked-by Nicolas Pitre <[email protected]>
Quoted 9 lines
---
 arch/arm/include/asm/processor.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
arch/arm/include/asm/processor.h +0 -0
index cbdb130..d571697 100644

arch/arm/include/asm/processor.h static inline void prefetch(const void *ptr) · line 124

Quoted 10 lines
arch/arm/include/asm/processor.h +1 -5
@@ -116,12 +116,19 @@ static inline void prefetch(const void *ptr)
 		:: "p" (ptr));
 }
 
+#if __LINUX_ARM_ARCH__ >= 7 && defined(CONFIG_SMP)
 #define ARCH_HAS_PREFETCHW
-#define prefetchw(ptr)	prefetch(ptr)
-
-#define ARCH_HAS_SPINLOCK_PREFETCH
-#define spin_lock_prefetch(x) do { } while (0)
-

What about keeping the above definitions when __LINUX_ARM_ARCH__ >= 7 && defined(CONFIG_SMP) is not true?

arch/arm/include/asm/processor.h static inline void prefetch(const void *ptr) · line 128

Quoted 8 lines
arch/arm/include/asm/processor.h +8 -0
+static inline void prefetchw(const void *ptr)
+{
+	__asm__ __volatile__(
+		".arch_extension	mp\n"
+	__ALT_SMP_ASM(
+		WASM(pldw)		"\t%a0",
+		WASM(pld)		"\t%a0"
+	)

The paren indentation looks odd. If you shift the whole __ALT_SMP_ASM block right then it is also less likely to look like a sequential execution of pldw followed by pld to the casual reader.

Quoted 15 lines
arch/arm/include/asm/processor.h +3 -0
+		:: "p" (ptr));
+}
+#endif
 #endif
 
 #define HAVE_ARCH_PICK_MMAP_LAYOUT
Signature · 7 lines
1.8.2.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author
#12 lore raw

PATCH 3/6 ARM: prefetch: add support for prefetchw using pldw on SMP ARMv7+ CPUs

Quoted 26 lines · Nicolas Pitre
Quoted 7 lines · Will Deacon

SMP ARMv7 CPUs implement the pldw instruction, which allows them to prefetch data cachelines in an exclusive state.

This patch defines the prefetchw macro using pldw for CPUs that support it.

Signed-off-by Will Deacon <[email protected]>

Minor nit below. Otherwise:

Acked-by Nicolas Pitre <[email protected]>
Quoted 12 lines
---
 arch/arm/include/asm/processor.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
arch/arm/include/asm/processor.h +0 -0
index cbdb130..d571697 100644
@@ -116,12 +116,19 @@ static inline void prefetch(const void *ptr)
 		:: "p" (ptr));
 }
Quoted 10 lines

arch/arm/include/asm/processor.h static inline void prefetch(const void *ptr) · line 124

Quoted 7 lines
arch/arm/include/asm/processor.h +1 -5
+#if __LINUX_ARM_ARCH__ >= 7 && defined(CONFIG_SMP)
 #define ARCH_HAS_PREFETCHW
-#define prefetchw(ptr)	prefetch(ptr)
-
-#define ARCH_HAS_SPINLOCK_PREFETCH
-#define spin_lock_prefetch(x) do { } while (0)
-

What about keeping the above definitions when __LINUX_ARM_ARCH__ >= 7 && defined(CONFIG_SMP) is not true?

In the case that we don't have prefetchw, then spin_lock_prefetch would end up being defined as __builtin_prefetch(x,1) by linux/prefetch.h. GCC will almost certainly spit out `pld' for that (it currently ignores the second argument...), which should be better than doing nothing. The cacheline may not be in the ideal state (shared rather than exclusive), but at least it's likely to be in the cache rather than sitting out in memory.

Unfortunately, spin_lock_prefetch is only used in one place, so changes to its implementation don't have any measurable difference on kernel performance.

Quoted 4 lines
Quoted 4 lines
arch/arm/include/asm/processor.h +4 -0
+static inline void prefetchw(const void *ptr)
+{
+	__asm__ __volatile__(
+		".arch_extension	mp\n"
Quoted 8 lines

arch/arm/include/asm/processor.h static inline void prefetch(const void *ptr) · line 128

Quoted 4 lines
arch/arm/include/asm/processor.h +4 -0
+	__ALT_SMP_ASM(
+		WASM(pldw)		"\t%a0",
+		WASM(pld)		"\t%a0"
+	)

The paren indentation looks odd. If you shift the whole __ALT_SMP_ASM block right then it is also less likely to look like a sequential execution of pldw followed by pld to the casual reader.

Sure, I'll fix that up.

Thanks again for your feedback,

Will

author
#13 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

In reply to Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our {spin,read,write}_[try]lock implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/spinlock.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm/include/asm/spinlock.h +8 -1
index 0de7bec..3e1cc9d 100644
@@ -5,7 +5,7 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#include <asm/processor.h>
+#include <linux/prefetch.h>
 
 /*
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K

@@ -70,6 +70,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	u32 newval;
 	arch_spinlock_t lockval;
 
+	prefetchw((const void *)&lock->slock);
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%3]\n"
 "	add	%1, %0, %4\n"

@@ -93,6 +94,7 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
 	unsigned long contended, res;
 	u32 slock;
 
+	prefetchw((const void *)&lock->slock);
 	do {
 		__asm__ __volatile__(
 		"	ldrex	%0, [%3]\n"

@@ -145,6 +147,7 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
 {
 	unsigned long tmp;
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"

@@ -163,6 +166,7 @@ static inline int arch_write_trylock(arch_rwlock_t *rw)
 {
 	unsigned long tmp;
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"

@@ -211,6 +215,7 @@ static inline void arch_read_lock(arch_rwlock_t *rw)
 {
 	unsigned long tmp, tmp2;
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%2]\n"
 "	adds	%0, %0, #1\n"

@@ -231,6 +236,7 @@ static inline void arch_read_unlock(arch_rwlock_t *rw)
 
 	smp_mb();
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%2]\n"
 "	sub	%0, %0, #1\n"

@@ -249,6 +255,7 @@ static inline int arch_read_trylock(arch_rwlock_t *rw)
 {
 	unsigned long tmp, tmp2 = 1;
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "	ldrex	%0, [%2]\n"
 "	adds	%0, %0, #1\n"
Signature · 1 line
1.8.2.2
#14 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Quoted 27 lines · Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our {spin,read,write}_[try]lock implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/spinlock.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm/include/asm/spinlock.h +1 -1
index 0de7bec..3e1cc9d 100644
@@ -5,7 +5,7 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#include <asm/processor.h>
+#include <linux/prefetch.h>
 
 /*
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K

arch/arm/include/asm/spinlock.h static inline void arch_spin_lock(arch_spinlock_t *lock) · line 73

Quoted 4 lines
arch/arm/include/asm/spinlock.h +1 -0
@@ -70,6 +70,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	u32 newval;
 	arch_spinlock_t lockval;
 
+	prefetchw((const void *)&lock->slock);

Couldn't that cast be carried in the definition of prefetchw() instead?

Other than that:

Acked-by Nicolas Pitre <[email protected]>
Quoted 60 lines
arch/arm/include/asm/spinlock.h +6 -0
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%3]\n"
 "	add	%1, %0, %4\n"

@@ -93,6 +94,7 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
 	unsigned long contended, res;
 	u32 slock;
 
+	prefetchw((const void *)&lock->slock);
 	do {
 		__asm__ __volatile__(
 		"	ldrex	%0, [%3]\n"

@@ -145,6 +147,7 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
 {
 	unsigned long tmp;
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"

@@ -163,6 +166,7 @@ static inline int arch_write_trylock(arch_rwlock_t *rw)
 {
 	unsigned long tmp;
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"

@@ -211,6 +215,7 @@ static inline void arch_read_lock(arch_rwlock_t *rw)
 {
 	unsigned long tmp, tmp2;
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%2]\n"
 "	adds	%0, %0, #1\n"

@@ -231,6 +236,7 @@ static inline void arch_read_unlock(arch_rwlock_t *rw)
 
 	smp_mb();
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%2]\n"
 "	sub	%0, %0, #1\n"

@@ -249,6 +255,7 @@ static inline int arch_read_trylock(arch_rwlock_t *rw)
 {
 	unsigned long tmp, tmp2 = 1;
 
+	prefetchw((const void *)&rw->lock);
 	__asm__ __volatile__(
 "	ldrex	%0, [%2]\n"
 "	adds	%0, %0, #1\n"
Signature · 7 lines
1.8.2.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author
#15 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Quoted 32 lines · Nicolas Pitre
Quoted 30 lines · Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our {spin,read,write}_[try]lock implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/spinlock.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm/include/asm/spinlock.h +1 -1
index 0de7bec..3e1cc9d 100644
@@ -5,7 +5,7 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#include <asm/processor.h>
+#include <linux/prefetch.h>
 
 /*
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K

@@ -70,6 +70,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	u32 newval;
 	arch_spinlock_t lockval;
Quoted 3 lines

arch/arm/include/asm/spinlock.h static inline void arch_spin_lock(arch_spinlock_t *lock) · line 73

Quoted 1 line
arch/arm/include/asm/spinlock.h +1 -0
+	prefetchw((const void *)&lock->slock);

Couldn't that cast be carried in the definition of prefetchw() instead?

I think that would mean implementing prefetchw as a macro rather than an inline function, since the core code expects to pass a const pointer and GCC gets angry if the type signatures don't match.

I'll have a go at doing that for v2.

Cheers,

Will

#16 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Quoted 33 lines · Will Deacon
Quoted 32 lines · Nicolas Pitre
Quoted 30 lines · Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our {spin,read,write}_[try]lock implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/spinlock.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm/include/asm/spinlock.h +1 -1
index 0de7bec..3e1cc9d 100644
@@ -5,7 +5,7 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#include <asm/processor.h>
+#include <linux/prefetch.h>
 
 /*
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K

@@ -70,6 +70,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	u32 newval;
 	arch_spinlock_t lockval;
Quoted 5 lines
Quoted 2 lines

arch/arm/include/asm/spinlock.h static inline void arch_spin_lock(arch_spinlock_t *lock) · line 73

Quoted 1 line
arch/arm/include/asm/spinlock.h +1 -0
+	prefetchw((const void *)&lock->slock);

Couldn't that cast be carried in the definition of prefetchw() instead?

I think that would mean implementing prefetchw as a macro rather than an inline function, since the core code expects to pass a const pointer and GCC gets angry if the type signatures don't match.

Maybe I'm wrong, but can't you just remove the casts and leave the function as static inline? const void * is pretty much telling the compiler to turn off type checking.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
#17 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Quoted 34 lines · Stephen Boyd
Quoted 33 lines · Will Deacon
Quoted 32 lines · Nicolas Pitre
Quoted 30 lines · Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our {spin,read,write}_[try]lock implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/spinlock.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm/include/asm/spinlock.h +1 -1
index 0de7bec..3e1cc9d 100644
@@ -5,7 +5,7 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#include <asm/processor.h>
+#include <linux/prefetch.h>
 
 /*
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K

@@ -70,6 +70,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	u32 newval;
 	arch_spinlock_t lockval;
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines

arch/arm/include/asm/spinlock.h static inline void arch_spin_lock(arch_spinlock_t *lock) · line 73

Quoted 1 line
arch/arm/include/asm/spinlock.h +1 -0
+	prefetchw((const void *)&lock->slock);

Couldn't that cast be carried in the definition of prefetchw() instead?

I think that would mean implementing prefetchw as a macro rather than an inline function, since the core code expects to pass a const pointer and GCC gets angry if the type signatures don't match.

Maybe I'm wrong, but can't you just remove the casts and leave the function as static inline? const void * is pretty much telling the compiler to turn off type checking.

Oh joy. Why is rwlock's lock member marked volatile?

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
author
#18 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Quoted 40 lines · Stephen Boyd
Quoted 39 lines · Stephen Boyd
Quoted 38 lines · Will Deacon
Quoted 26 lines · Nicolas Pitre
Quoted 24 lines · Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our {spin,read,write}_[try]lock implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/spinlock.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm/include/asm/spinlock.h +1 -1
index 0de7bec..3e1cc9d 100644
@@ -5,7 +5,7 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#include <asm/processor.h>
+#include <linux/prefetch.h>
Quoted 8 lines

arch/arm/include/asm/spinlock.h static inline void arch_spin_lock(arch_spinlock_t *lock) · line 73

Quoted 7 lines
arch/arm/include/asm/spinlock.h +1 -0
 /*
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K

@@ -70,6 +70,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	u32 newval;
 	arch_spinlock_t lockval;
 
+	prefetchw((const void *)&lock->slock);

Couldn't that cast be carried in the definition of prefetchw() instead?

I think that would mean implementing prefetchw as a macro rather than an inline function, since the core code expects to pass a const pointer and GCC gets angry if the type signatures don't match.

Quoted 6 lines
Quoted 4 lines

Maybe I'm wrong, but can't you just remove the casts and leave the function as static inline? const void * is pretty much telling the compiler to turn off type checking.

Oh joy. Why is rwlock's lock member marked volatile?

Yeah, that was the problematic guy. However, I had to fix that anyway in this patch because otherwise the definition for prefetchw when !ARCH_HAS_PREFETCHW (which expands to __builtin_prefetch(x,1)) will explode.

So, given that I've fixed the rwlocks, I think I could put prefetch and prefetchw back to static inline functions. What do you reckon?

Will

author
#19 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Quoted 6 lines · Will Deacon
Quoted 1 line · Stephen Boyd

Oh joy. Why is rwlock's lock member marked volatile?

Yeah, that was the problematic guy. However, I had to fix that anyway in this patch because otherwise the definition for prefetchw when !ARCH_HAS_PREFETCHW (which expands to __builtin_prefetch(x,1)) will explode.

Just realised you're still looking at v1, which doesn't have any changes to the rwlocks. I posted v2 here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186411.html

Enjoy.

Will

#20 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

In reply to Will Deacon
Quoted 45 lines · Will Deacon
Quoted 44 lines · Stephen Boyd
Quoted 43 lines · Stephen Boyd
Quoted 33 lines · Will Deacon
Quoted 32 lines · Nicolas Pitre
Quoted 30 lines · Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our {spin,read,write}_[try]lock implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/spinlock.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm/include/asm/spinlock.h +1 -1
index 0de7bec..3e1cc9d 100644
@@ -5,7 +5,7 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#include <asm/processor.h>
+#include <linux/prefetch.h>
 
 /*
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K

@@ -70,6 +70,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	u32 newval;
 	arch_spinlock_t lockval;
Quoted 5 lines
Quoted 2 lines

arch/arm/include/asm/spinlock.h static inline void arch_spin_lock(arch_spinlock_t *lock) · line 73

Quoted 1 line
arch/arm/include/asm/spinlock.h +1 -0
+	prefetchw((const void *)&lock->slock);

Couldn't that cast be carried in the definition of prefetchw() instead?

I think that would mean implementing prefetchw as a macro rather than an inline function, since the core code expects to pass a const pointer and GCC gets angry if the type signatures don't match.

Maybe I'm wrong, but can't you just remove the casts and leave the function as static inline? const void * is pretty much telling the compiler to turn off type checking.

Quoted 7 lines
Quoted 1 line

Oh joy. Why is rwlock's lock member marked volatile?

Yeah, that was the problematic guy. However, I had to fix that anyway in this patch because otherwise the definition for prefetchw when !ARCH_HAS_PREFETCHW (which expands to __builtin_prefetch(x,1)) will explode.

So, given that I've fixed the rwlocks, I think I could put prefetch and prefetchw back to static inline functions. What do you reckon?

It would be good to match the builtin function's signature so that we don't explode in the future on ARCH_HAS_PREFETCHW configs.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
author
#21 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Quoted 12 lines · Stephen Boyd
Quoted 11 lines · Will Deacon
Quoted 6 lines · Stephen Boyd
Quoted 4 lines · Stephen Boyd

Maybe I'm wrong, but can't you just remove the casts and leave the function as static inline? const void * is pretty much telling the compiler to turn off type checking.

Oh joy. Why is rwlock's lock member marked volatile?

Yeah, that was the problematic guy. However, I had to fix that anyway in this patch because otherwise the definition for prefetchw when !ARCH_HAS_PREFETCHW (which expands to __builtin_prefetch(x,1)) will explode.

Quoted 5 lines
Quoted 2 lines

So, given that I've fixed the rwlocks, I think I could put prefetch and prefetchw back to static inline functions. What do you reckon?

It would be good to match the builtin function's signature so that we don't explode in the future on ARCH_HAS_PREFETCHW configs.

Ok, so that's basically just undoing the macroisation on top of v2 (fixup below).

Will

--->8

arch/arm/include/asm/processor.h +16 -16
index dde7ecc..dac9429 100644
@@ -109,25 +109,25 @@ unsigned long get_wchan(struct task_struct *p);
 #if __LINUX_ARM_ARCH__ >= 5
 
 #define ARCH_HAS_PREFETCH
-#define prefetch(p)                                                    \
-({                                                                     \
-       __asm__ __volatile__(                                           \
-               "pld\t%a0"                                              \
-               :: "p" (p));                                            \
-})
+static inline void prefetch(const void *ptr)
+{
+       __asm__ __volatile__(
+               "pld\t%a0"
+               :: "p" (ptr));
+}
 
 #if __LINUX_ARM_ARCH__ >= 7 && defined(CONFIG_SMP)
 #define ARCH_HAS_PREFETCHW
-#define prefetchw(p)                                                   \
-({                                                                     \
-       __asm__ __volatile__(                                           \
-               ".arch_extension        mp\n"                           \
-               __ALT_SMP_ASM(                                          \
-                       WASM(pldw)              "\t%a0",                \
-                       WASM(pld)               "\t%a0"                 \
-               )                                                       \
-               :: "p" (p));                                            \
-})
+static inline void prefetchw(const void *ptr)
+{
+       __asm__ __volatile__(
+               ".arch_extension        mp\n"
+               __ALT_SMP_ASM(
+                       WASM(pldw)              "\t%a0",
+                       WASM(pld)               "\t%a0"
+               )
+               :: "p" (ptr));
+}
 #endif
 #endif
#22 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Quoted 13 lines · Will Deacon
Quoted 12 lines · Stephen Boyd
Quoted 11 lines · Will Deacon
Quoted 6 lines · Stephen Boyd
Quoted 4 lines · Stephen Boyd

Maybe I'm wrong, but can't you just remove the casts and leave the function as static inline? const void * is pretty much telling the compiler to turn off type checking.

Oh joy. Why is rwlock's lock member marked volatile?

Yeah, that was the problematic guy. However, I had to fix that anyway in this patch because otherwise the definition for prefetchw when !ARCH_HAS_PREFETCHW (which expands to __builtin_prefetch(x,1)) will explode.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

So, given that I've fixed the rwlocks, I think I could put prefetch and prefetchw back to static inline functions. What do you reckon?

It would be good to match the builtin function's signature so that we don't explode in the future on ARCH_HAS_PREFETCHW configs.

Ok, so that's basically just undoing the macroisation on top of v2 (fixup below).

Will

Looks good to me. Thanks.

Quoted 49 lines

--->8

arch/arm/include/asm/processor.h +16 -16
index dde7ecc..dac9429 100644
@@ -109,25 +109,25 @@ unsigned long get_wchan(struct task_struct *p);
 #if __LINUX_ARM_ARCH__ >= 5
 
 #define ARCH_HAS_PREFETCH
-#define prefetch(p)                                                    \
-({                                                                     \
-       __asm__ __volatile__(                                           \
-               "pld\t%a0"                                              \
-               :: "p" (p));                                            \
-})
+static inline void prefetch(const void *ptr)
+{
+       __asm__ __volatile__(
+               "pld\t%a0"
+               :: "p" (ptr));
+}
 
 #if __LINUX_ARM_ARCH__ >= 7 && defined(CONFIG_SMP)
 #define ARCH_HAS_PREFETCHW
-#define prefetchw(p)                                                   \
-({                                                                     \
-       __asm__ __volatile__(                                           \
-               ".arch_extension        mp\n"                           \
-               __ALT_SMP_ASM(                                          \
-                       WASM(pldw)              "\t%a0",                \
-                       WASM(pld)               "\t%a0"                 \
-               )                                                       \
-               :: "p" (p));                                            \
-})
+static inline void prefetchw(const void *ptr)
+{
+       __asm__ __volatile__(
+               ".arch_extension        mp\n"
+               __ALT_SMP_ASM(
+                       WASM(pldw)              "\t%a0",
+                       WASM(pld)               "\t%a0"
+               )
+               :: "p" (ptr));
+}
 #endif
 #endif
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
#23 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

In reply to Will Deacon
Quoted 46 lines · Will Deacon
Quoted 45 lines · Stephen Boyd
Quoted 43 lines · Stephen Boyd
Quoted 33 lines · Will Deacon
Quoted 32 lines · Nicolas Pitre
Quoted 30 lines · Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our {spin,read,write}_[try]lock implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/spinlock.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm/include/asm/spinlock.h +1 -1
index 0de7bec..3e1cc9d 100644
@@ -5,7 +5,7 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#include <asm/processor.h>
+#include <linux/prefetch.h>
 
 /*
  * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K

@@ -70,6 +70,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	u32 newval;
 	arch_spinlock_t lockval;
Quoted 5 lines
Quoted 2 lines

arch/arm/include/asm/spinlock.h static inline void arch_spin_lock(arch_spinlock_t *lock) · line 73

Quoted 1 line
arch/arm/include/asm/spinlock.h +1 -0
+	prefetchw((const void *)&lock->slock);

Couldn't that cast be carried in the definition of prefetchw() instead?

I think that would mean implementing prefetchw as a macro rather than an inline function, since the core code expects to pass a const pointer and GCC gets angry if the type signatures don't match.

Maybe I'm wrong, but can't you just remove the casts and leave the function as static inline? const void * is pretty much telling the compiler to turn off type checking.

Quoted 5 lines
Quoted 1 line

Oh joy. Why is rwlock's lock member marked volatile?

Yeah, that was the problematic guy. However, I had to fix that anyway in this patch because otherwise the definition for prefetchw when !ARCH_HAS_PREFETCHW (which expands to __builtin_prefetch(x,1)) will explode.

Why not always keep ARCH_HAS_PREFETCHW defined then, and have __builtin_prefetch([const volatile void *)x,1)) when we can't do better?

Or why not adding this cast to the generic definition?

Nicolas

author
#24 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Hi Nicolas,

Quoted 4 lines · Nicolas Pitre
Quoted 3 lines · Will Deacon
Quoted 2 lines

Oh joy. Why is rwlock's lock member marked volatile?

Quoted 8 lines
Quoted 3 lines

Yeah, that was the problematic guy. However, I had to fix that anyway in this patch because otherwise the definition for prefetchw when !ARCH_HAS_PREFETCHW (which expands to __builtin_prefetch(x,1)) will explode.

Why not always keep ARCH_HAS_PREFETCHW defined then, and have __builtin_prefetch([const volatile void *)x,1)) when we can't do better?

Or why not adding this cast to the generic definition?

Yes, I'd prefer to fix the generic definition as a separate patch. However, I think the only case that this generates warnings is when a volatile * is passed. If volatile is actually required, I question whether prefetching from that location even makes sense in the first place since, in the kernel, the keyword seems to be generally associated with IO rather than CPUs accessing shared memory via caches.

What do you think?

Will

#25 lore raw

PATCH 4/6 ARM: locks: prefetch the destination word for write prior to strex

Quoted 16 lines · Will Deacon

Hi Nicolas,

Quoted 4 lines · Nicolas Pitre
Quoted 3 lines · Will Deacon
Quoted 2 lines

Oh joy. Why is rwlock's lock member marked volatile?

Quoted 8 lines
Quoted 3 lines

Yeah, that was the problematic guy. However, I had to fix that anyway in this patch because otherwise the definition for prefetchw when !ARCH_HAS_PREFETCHW (which expands to __builtin_prefetch(x,1)) will explode.

Why not always keep ARCH_HAS_PREFETCHW defined then, and have __builtin_prefetch([const volatile void *)x,1)) when we can't do better?

Or why not adding this cast to the generic definition?

Quoted 8 lines

Yes, I'd prefer to fix the generic definition as a separate patch. However, I think the only case that this generates warnings is when a volatile * is passed. If volatile is actually required, I question whether prefetching from that location even makes sense in the first place since, in the kernel, the keyword seems to be generally associated with IO rather than CPUs accessing shared memory via caches.

What do you think?

The only thing volatile is good for is to tell the compiler not to attempt any kind of optimization on the access. Obviously this is needed for IO accessors, but we don't want the compiler to think it can cache a previous reading of the lock value either. So as long as all accesses are done through some accessors (you did add ACCESS_ONCE() in your patch removing the volatile for that reason) then we don't need that keyword in the lock definition.

Nicolas

author
#26 lore raw

PATCH 5/6 ARM: atomics: prefetch the destination word for write prior to strex

In reply to Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our atomic access implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start. Only the barrier-less functions are updated, since memory barriers can limit the usefulness of prefetching data.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/atomic.h | 7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/include/asm/atomic.h +7 -0
index da1c77d..e3f052a 100644
@@ -12,6 +12,7 @@
 #define __ASM_ARM_ATOMIC_H
 
 #include <linux/compiler.h>
+#include <linux/prefetch.h>
 #include <linux/types.h>
 #include <linux/irqflags.h>
 #include <asm/barrier.h>

@@ -41,6 +42,7 @@ static inline void atomic_add(int i, atomic_t *v)
 	unsigned long tmp;
 	int result;
 
+	prefetchw((const void *)&v->counter);
 	__asm__ __volatile__("@ atomic_add\n"
 "1:	ldrex	%0, [%3]\n"
 "	add	%0, %0, %4\n"

@@ -79,6 +81,7 @@ static inline void atomic_sub(int i, atomic_t *v)
 	unsigned long tmp;
 	int result;
 
+	prefetchw((const void *)&v->counter);
 	__asm__ __volatile__("@ atomic_sub\n"
 "1:	ldrex	%0, [%3]\n"
 "	sub	%0, %0, %4\n"

@@ -138,6 +141,7 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
 {
 	unsigned long tmp, tmp2;
 
+	prefetchw((const void *)addr);
 	__asm__ __volatile__("@ atomic_clear_mask\n"
 "1:	ldrex	%0, [%3]\n"
 "	bic	%0, %0, %4\n"

@@ -283,6 +287,7 @@ static inline void atomic64_set(atomic64_t *v, u64 i)
 {
 	u64 tmp;
 
+	prefetchw((const void *)&v->counter);
 	__asm__ __volatile__("@ atomic64_set\n"
 "1:	ldrexd	%0, %H0, [%2]\n"
 "	strexd	%0, %3, %H3, [%2]\n"

@@ -299,6 +304,7 @@ static inline void atomic64_add(u64 i, atomic64_t *v)
 	u64 result;
 	unsigned long tmp;
 
+	prefetchw((const void *)&v->counter);
 	__asm__ __volatile__("@ atomic64_add\n"
 "1:	ldrexd	%0, %H0, [%3]\n"
 "	adds	%0, %0, %4\n"

@@ -339,6 +345,7 @@ static inline void atomic64_sub(u64 i, atomic64_t *v)
 	u64 result;
 	unsigned long tmp;
 
+	prefetchw((const void *)&v->counter);
 	__asm__ __volatile__("@ atomic64_sub\n"
 "1:	ldrexd	%0, %H0, [%3]\n"
 "	subs	%0, %0, %4\n"
Signature · 1 line
1.8.2.2
#27 lore raw

PATCH 5/6 ARM: atomics: prefetch the destination word for write prior to strex

Quoted 28 lines · Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our atomic access implementations with pldw instructions (on CPUs which support them) to try and grab the line in exclusive state from the start. Only the barrier-less functions are updated, since memory barriers can limit the usefulness of prefetching data.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/include/asm/atomic.h | 7 +++++++
 1 file changed, 7 insertions(+)
arch/arm/include/asm/atomic.h +1 -0
index da1c77d..e3f052a 100644
@@ -12,6 +12,7 @@
 #define __ASM_ARM_ATOMIC_H
 
 #include <linux/compiler.h>
+#include <linux/prefetch.h>
 #include <linux/types.h>
 #include <linux/irqflags.h>
 #include <asm/barrier.h>

arch/arm/include/asm/atomic.h static inline void atomic_add(int i, atomic_t *v) · line 45

Quoted 4 lines
arch/arm/include/asm/atomic.h +1 -0
@@ -41,6 +42,7 @@ static inline void atomic_add(int i, atomic_t *v)
 	unsigned long tmp;
 	int result;
 
+	prefetchw((const void *)&v->counter);

Same comment as for the previous patch wrt the cast. Even more so given it is spreading all over the place now.

Quoted 52 lines
arch/arm/include/asm/atomic.h +5 -0
 	__asm__ __volatile__("@ atomic_add\n"
 "1:	ldrex	%0, [%3]\n"
 "	add	%0, %0, %4\n"

@@ -79,6 +81,7 @@ static inline void atomic_sub(int i, atomic_t *v)
 	unsigned long tmp;
 	int result;
 
+	prefetchw((const void *)&v->counter);
 	__asm__ __volatile__("@ atomic_sub\n"
 "1:	ldrex	%0, [%3]\n"
 "	sub	%0, %0, %4\n"

@@ -138,6 +141,7 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
 {
 	unsigned long tmp, tmp2;
 
+	prefetchw((const void *)addr);
 	__asm__ __volatile__("@ atomic_clear_mask\n"
 "1:	ldrex	%0, [%3]\n"
 "	bic	%0, %0, %4\n"

@@ -283,6 +287,7 @@ static inline void atomic64_set(atomic64_t *v, u64 i)
 {
 	u64 tmp;
 
+	prefetchw((const void *)&v->counter);
 	__asm__ __volatile__("@ atomic64_set\n"
 "1:	ldrexd	%0, %H0, [%2]\n"
 "	strexd	%0, %3, %H3, [%2]\n"

@@ -299,6 +304,7 @@ static inline void atomic64_add(u64 i, atomic64_t *v)
 	u64 result;
 	unsigned long tmp;
 
+	prefetchw((const void *)&v->counter);
 	__asm__ __volatile__("@ atomic64_add\n"
 "1:	ldrexd	%0, %H0, [%3]\n"
 "	adds	%0, %0, %4\n"

@@ -339,6 +345,7 @@ static inline void atomic64_sub(u64 i, atomic64_t *v)
 	u64 result;
 	unsigned long tmp;
 
+	prefetchw((const void *)&v->counter);
 	__asm__ __volatile__("@ atomic64_sub\n"
 "1:	ldrexd	%0, %H0, [%3]\n"
 "	subs	%0, %0, %4\n"
Signature · 7 lines
1.8.2.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author
#28 lore raw

PATCH 6/6 ARM: bitops: prefetch the destination word for write prior to strex

In reply to Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our atomic bitops implementation with prefetchw, to try and grab the line in exclusive state from the start. The testop macro is left alone, since the barrier semantics limit the usefulness of prefetching data.

Signed-off-by Will Deacon <[email protected]>
---
 arch/arm/lib/bitops.h | 5 +++++
 1 file changed, 5 insertions(+)
arch/arm/lib/bitops.h +5 -0
index d6408d1..e0c68d5 100644
@@ -10,6 +10,11 @@ UNWIND(	.fnstart	)
 	and	r3, r0, #31		@ Get bit offset
 	mov	r0, r0, lsr #5
 	add	r1, r1, r0, lsl #2	@ Get word offset
+#if __LINUX_ARM_ARCH__ >= 7
+	.arch_extension	mp
+	ALT_SMP(W(pldw)	[r1])
+	ALT_UP(W(nop))
+#endif
 	mov	r3, r2, lsl r3
 1:	ldrex	r2, [r1]
 	\instr	r2, r2, r3
Signature · 1 line
1.8.2.2
#29 lore raw

PATCH 6/6 ARM: bitops: prefetch the destination word for write prior to strex

Quoted 10 lines · Will Deacon

The cost of changing a cacheline from shared to exclusive state can be significant, especially when this is triggered by an exclusive store, since it may result in having to retry the transaction.

This patch prefixes our atomic bitops implementation with prefetchw, to try and grab the line in exclusive state from the start. The testop macro is left alone, since the barrier semantics limit the usefulness of prefetching data.

Signed-off-by Will Deacon <[email protected]>
Acked-by Nicolas Pitre <[email protected]>
Quoted 29 lines
---
 arch/arm/lib/bitops.h | 5 +++++
 1 file changed, 5 insertions(+)
arch/arm/lib/bitops.h +5 -0
index d6408d1..e0c68d5 100644
@@ -10,6 +10,11 @@ UNWIND(	.fnstart	)
 	and	r3, r0, #31		@ Get bit offset
 	mov	r0, r0, lsr #5
 	add	r1, r1, r0, lsl #2	@ Get word offset
+#if __LINUX_ARM_ARCH__ >= 7
+	.arch_extension	mp
+	ALT_SMP(W(pldw)	[r1])
+	ALT_UP(W(nop))
+#endif
 	mov	r3, r2, lsl r3
 1:	ldrex	r2, [r1]
 	\instr	r2, r2, r3
Signature · 7 lines
1.8.2.2


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