ARM: fix cpu_relax() in case of doing dmb
PATCH 1/1 ARM: fix cpu_relax() in case of doing dmb
There is an issue reported on imx6q restart function. The issue is only seen with the image building ARMv7 and ARMv6 together, where cpu_relax() is define to do dmb. It's been root-caused by Russell as below.
Quoted 3 lines · Russell King - ARM Linux
I suspect having this dmb inside
cpu_relax()is flooding the interconnects with traffic, which then prevents other CPUs getting a look-in (maybe there's no fairness when it comes to dmb's.
Fix the issue by insert a few NOPs into cpu_relax() where doing dmb.
--- arch/arm/include/asm/processor.h | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
arch/arm/include/asm/processor.h +8 -1
@@ -80,7 +80,14 @@ extern void release_thread(struct task_struct *);unsigned long get_wchan (struct task_struct *p);#if __LINUX_ARM_ARCH__ ==6 || defined(CONFIG_ARM_ERRATA_754327 ) -#define cpu_relax () smp_mb() +#define cpu_relax () do { \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + smp_mb(); \ + } while (0)#else #define cpu_relax () barrier()#endif
Signature · 1 line
1.7.5.4
PATCH 1/1 ARM: fix cpu_relax() in case of doing dmb
Quoted 4 lines · Shawn Guo
There is an issue reported on imx6q restart function. The issue is only seen with the image building ARMv7 and ARMv6 together, where
cpu_relax()is define to do dmb. It's been root-caused by Russell as below.
Quoted 9 lines
Quoted 4 lines · Russell King - ARM Linux
I suspect having this dmb inside
cpu_relax()is flooding the interconnects with traffic, which then prevents other CPUs getting a look-in (maybe there's no fairness when it comes to dmb's.Fix the issue by insert a few NOPs into
cpu_relax()where doing dmb.
PATCH 1/1 ARM: fix cpu_relax() in case of doing dmb
In reply to Shawn GuoQuoted 5 lines · Shawn Guo
There is an issue reported on imx6q restart function. The issue is only seen with the image building ARMv7 and ARMv6 together, where
cpu_relax()is define to do dmb. It's been root-caused by Russell as below.Russell King - ARM Linux wrote:
Quoted 8 lines
Quoted 3 lines
I suspect having this dmb inside
cpu_relax()is flooding the interconnects with traffic, which then prevents other CPUs getting a look-in (maybe there's no fairness when it comes to dmb's.Fix the issue by insert a few NOPs into
cpu_relax()where doing dmb.Cc <[email protected]>Signed-off-by Shawn Guo <[email protected]>
Tested on a i.MX6 SabreLite board together with the patch 'ARM: imx6q:
remove imx_src_prepare_restart() call':
Thanks
Dirk
Quoted 24 lines
--- arch/arm/include/asm/processor.h | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)arch/arm/include/asm/processor.h +8
-1@@ -80,7 +80,14 @@ extern void release_thread(struct task_struct *);unsigned long get_wchan (struct task_struct *p);#if __LINUX_ARM_ARCH__ ==6 || defined(CONFIG_ARM_ERRATA_754327 ) -#define cpu_relax () smp_mb() +#define cpu_relax () do { \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + smp_mb(); \ + } while (0)#else #define cpu_relax () barrier()#endif
PATCH 1/1 ARM: fix cpu_relax() in case of doing dmb
In reply to Shawn GuoQuoted 5 lines · Shawn Guo
arch/arm/include/asm/processor.h +0
-0
arch/arm/include/asm/processor.h extern void release_thread(struct task_struct *); · line 88
Quoted 10 lines
arch/arm/include/asm/processor.h +6
-1@@ -80,7 +80,14 @@ extern void release_thread(struct task_struct *);unsigned long get_wchan (struct task_struct *p);#if __LINUX_ARM_ARCH__ ==6 || defined(CONFIG_ARM_ERRATA_754327 ) -#define cpu_relax () smp_mb() +#define cpu_relax () do { \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \
Can you use nop() instead of the explicit asm? Also, I think we should try and use some methodology on deciding the number of nops to insert. Without having a full handle on the problem at the moment, it would seem that we need at least NR_CPUS worth (since the number of spinning secondaries is NR_CPUS-1 and they may execute their barriers in lock-step).
Will
PATCH 1/1 ARM: fix cpu_relax() in case of doing dmb
Quoted 8 lines · Will Deacon
Quoted 7 lines · Shawn Guo
arch/arm/include/asm/processor.h +0
-0@@ -80,7 +80,14 @@ extern void release_thread(struct task_struct *);unsigned long get_wchan (struct task_struct *p);
Quoted 10 lines
arch/arm/include/asm/processor.h extern void release_thread(struct task_struct *); · line 88
Quoted 8 lines
arch/arm/include/asm/processor.h +6
-1#if __LINUX_ARM_ARCH__ ==6 || defined(CONFIG_ARM_ERRATA_754327 ) -#define cpu_relax () smp_mb() +#define cpu_relax () do { \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \Can you use
nop()instead of the explicit asm?
Yes. I just tried, and it works too.
Quoted 6 lines
Also, I think we should try
and use some methodology on deciding the number of nops to insert. Without having a full handle on the problem at the moment, it would seem that we need at leastNR_CPUSworth (since the number of spinning secondaries is NR_CPUS-1 and they may execute their barriers in lock-step).
I'm not sure we get something like that. In my testing here, I need at least 5 nops to get rid of the issue.
Signature · 2 lines
Regards, Shawn
PATCH 1/1 ARM: fix cpu_relax() in case of doing dmb
Quoted 22 lines · Shawn Guo
Quoted 8 lines · Will Deacon
Quoted 7 lines · Shawn Guo
arch/arm/include/asm/processor.h +0
-0@@ -80,7 +80,14 @@ extern void release_thread(struct task_struct *);unsigned long get_wchan (struct task_struct *p);Quoted 10 lines
arch/arm/include/asm/processor.h extern void release_thread(struct task_struct *); · line 88
Quoted 8 lines
arch/arm/include/asm/processor.h +6
-1#if __LINUX_ARM_ARCH__ ==6 || defined(CONFIG_ARM_ERRATA_754327 ) -#define cpu_relax () smp_mb() +#define cpu_relax () do { \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \Can you use
nop()instead of the explicit asm?Yes. I just tried, and it works too.
Quoted 8 lines
Quoted 6 lines
Also, I think we should try
and use some methodology on deciding the number of nops to insert. Without having a full handle on the problem at the moment, it would seem that we need at leastNR_CPUSworth (since the number of spinning secondaries is NR_CPUS-1 and they may execute their barriers in lock-step).I'm not sure we get something like that. In my testing here, I need at least 5 nops to get rid of the issue.
Doesn't A9 do dual issue? If so, the maths for your 4 core iMX6Q might match up with Will's hypothesis. You could try the theory by building say with CONFIG_NR_CPUS == 3.
Signature · 1 line
Tixy
PATCH 1/1 ARM: fix cpu_relax() in case of doing dmb
Quoted 29 lines · Jon Medhurst (Tixy)
Quoted 28 lines · Shawn Guo
Quoted 8 lines · Will Deacon
Quoted 7 lines · Shawn Guo
arch/arm/include/asm/processor.h +0
-0@@ -80,7 +80,14 @@ extern void release_thread(struct task_struct *);unsigned long get_wchan (struct task_struct *p);Quoted 10 lines
arch/arm/include/asm/processor.h extern void release_thread(struct task_struct *); · line 88
Quoted 8 lines
arch/arm/include/asm/processor.h +6
-1#if __LINUX_ARM_ARCH__ ==6 || defined(CONFIG_ARM_ERRATA_754327 ) -#define cpu_relax () smp_mb() +#define cpu_relax () do { \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \Can you use
nop()instead of the explicit asm?Yes. I just tried, and it works too.
Quoted 6 lines
Also, I think we should try
and use some methodology on deciding the number of nops to insert. Without having a full handle on the problem at the moment, it would seem that we need at leastNR_CPUSworth (since the number of spinning secondaries is NR_CPUS-1 and they may execute their barriers in lock-step).
Quoted 4 lines
Quoted 2 lines
I'm not sure we get something like that. In my testing here, I need at least 5 nops to get rid of the issue.
Doesn't A9 do dual issue?
Do you have some details about the issue to share?
Quoted 4 lines
If so, the maths for your 4 core iMX6Q might
match up with Will's hypothesis. You could try the theory by building say withCONFIG_NR_CPUS== 3.
I'm still not quite sure about the hypothesis, but I assume you are asking if 3 NOPs will fix the issue. If so, the answer is NO. I increase the number of NOP incrementally starting from 1, and the issue remains until we have 5 NOPs in there.
Signature · 2 lines
Regards, Shawn
PATCH 1/1 ARM: fix cpu_relax() in case of doing dmb
Quoted 30 lines · Shawn Guo
Quoted 29 lines · Jon Medhurst (Tixy)
Quoted 28 lines · Shawn Guo
Quoted 8 lines · Will Deacon
Quoted 7 lines · Shawn Guo
arch/arm/include/asm/processor.h +0
-0@@ -80,7 +80,14 @@ extern void release_thread(struct task_struct *);unsigned long get_wchan (struct task_struct *p);Quoted 10 lines
arch/arm/include/asm/processor.h extern void release_thread(struct task_struct *); · line 88
Quoted 8 lines
arch/arm/include/asm/processor.h +6
-1#if __LINUX_ARM_ARCH__ ==6 || defined(CONFIG_ARM_ERRATA_754327 ) -#define cpu_relax () smp_mb() +#define cpu_relax () do { \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \Can you use
nop()instead of the explicit asm?Yes. I just tried, and it works too.
Quoted 6 lines
Also, I think we should try
and use some methodology on deciding the number of nops to insert. Without having a full handle on the problem at the moment, it would seem that we need at leastNR_CPUSworth (since the number of spinning secondaries is NR_CPUS-1 and they may execute their barriers in lock-step).
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
I'm not sure we get something like that. In my testing here, I need at least 5 nops to get rid of the issue.
Doesn't A9 do dual issue?
Do you have some details about the issue to share?
I don't have any particular insight, I was just making the observation that if CPU clock cycles executed in the loop were a consideration, then the fact that the A9 would probably execute two nops in a clock cycle would be pertinent.
Quoted 8 lines
Quoted 4 lines
If so, the maths for your 4 core iMX6Q might
match up with Will's hypothesis. You could try the theory by building say withCONFIG_NR_CPUS== 3.I'm still not quite sure about the hypothesis, but I assume you are asking if 3 NOPs will fix the issue. If so, the answer is NO. I increase the number of NOP incrementally starting from 1, and the issue remains until we have 5 NOPs in there.
Right, your and Hui test seems to scupper the idea that number of nops should be related to number of CPUs. (Which I didn't really under either.)
Perhaps the issue is related to the size of the loop buffer, or possibly cache line boundaries?
Signature · 1 line
Tixy
PATCH 1/1 ARM: fix cpu_relax() in case of doing dmb
In reply to Will DeaconQuoted 9 lines · Will Deacon
Quoted 7 lines · Shawn Guo
arch/arm/include/asm/processor.h +0
-0@@ -80,7 +80,14 @@ extern void release_thread(struct task_struct *);unsigned long get_wchan (struct task_struct *p);
Quoted 16 lines
arch/arm/include/asm/processor.h extern void release_thread(struct task_struct *);
Quoted 9 lines
arch/arm/include/asm/processor.h +6
-1#if __LINUX_ARM_ARCH__ ==6 || defined(CONFIG_ARM_ERRATA_754327 ) -#define cpu_relax () smp_mb() +#define cpu_relax () do { \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \ + asm("nop"); \Can you use
nop()instead of the explicit asm? Also, I think we should try and use some methodology on deciding the number of nops to insert. Without having a full handle on the problem at the moment, it would seem that we need at leastNR_CPUSworth (since the number of spinning secondaries is NR_CPUS-1 and they may execute their barriers in lock-step).
Your concern sounds reasonable, but i did a test, the result show there is no explicit relation between NR_CPUS and the number of nop needed.
NR_CPUS = 4 and NR_CPUS = 2 need at least the same number of nop.
Regards,
Hui.
Quoted 3 lines
Will