ARM: change definition of cpu_relax() for ARM11MPCore

Linux expects that if a CPU modifies a memory location, then that modification will eventually become visible to other CPUs in the system.

On an ARM11MPCore processor, loads are prioritised over stores so it is possible for a store operation to be postponed if a polling loop immediately follows it. If the variable being polled indirectly depends on the outstanding store [for example, another CPU may be polling the variable that is pending modification] then there is the potential for deadlock if interrupts are disabled. This deadlock occurs in the KGDB testsuire when executing on an SMP ARM11MPCore configuration.

This patch changes the definition of cpu_relax() to smp_mb() for ARMv6 cores, forcing the write buffer to drain while inside a polling loop on an SMP system. If the Kernel is not compiled for SMP support, this will expand to a barrier() as before.

---
 arch/arm/include/asm/processor.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

index 6a89567..da2078e 100644
@@ -91,7 +91,16 @@ extern void release_thread(struct task_struct *);
 
 unsigned long get_wchan(struct task_struct *p);
 
+#if __LINUX_ARM_ARCH__ == 6
+/*
+ * The store buffer on ARM11MPCore is not guaranteed to drain when the CPU
+ * is performing aggresive loads. This is usually the case in a polling loop,
+ * so we add a memory barrier to allow any pending stores to complete.
+ */
+#define cpu_relax()			smp_mb()
+#else
 #define cpu_relax()			barrier()
+#endif
 
 /*
  * Create a new kernel thread

1.6.3.3

Linus asked how expensive (in terms of power rather than performance) this was; so far that question has remained unanswered. Can someone please answer his question?

Hi,

If I've followed these threads [1][2] correctly, this ARM11 MPCore issue was discovered while running the "KGDB: internal test suite" (KGDB_TESTS) and that problem is resolved via "kgdb: use atomic_inc and atomic_dec instead of atomic_set" [3]. If so, isn't the original ARM11 MPCore KGDB cpu_relax() issue just a red herring? Shouldn't any polling loops which depend on specific (hardware) write/read order implement appropriate barriers rather than rely on cpu_relax() to guarantee order? If perhaps there are indeed other cases where cpu_relax() is being used incorrectly, then maybe those should be fixed instead? Just curious...

Thanks!

--
Regards,
George
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/010691.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011076.html
[3] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff_plain;h=ae6bf53e0255c8ab04b6fe31806e318432570e3c

Note that the need to force the write buffer to drain is _not_ an ordering issue. It's a buffer draining issue. :-)

I'm not sure if Linux smp_wmb() guarantees to ensure prior writes will be visible to other CPUs in a short time, or if it only guarantees write order.

-- Jamie

Hi George,

Yes, I encountered the issue in the KGDB testsuite. However, the patch [3] in mainline doesn't resolve the issue as pointed out here:

http://lkml.org/lkml/2010/4/8/214

As pointed out by Jamie Lokier, this isn't about ordering, it's about ensuring that stores eventually make it to memory.

According to Linus:

`Linux does expect that if some other CPU modifies a memory location, then we _will_ see that modification eventually. If the CPU needs help to do so, then cpu_relax() needs to do that.'

Since the ARM11MPCore doesn't make this guarantee in hardware, then we have to fix it up in software. We can either fix up every polling loop in the Kernel individually or [as suggested above] modify cpu_relax() to do the work for us. Note that we only need to modify cpu_relax() for this particular core; other ARM SMP variants behave more sanely.

Thanks,

Will

Hi Russell,

I gave my opinion on the potential performance impact here:

http://sourceforge.net/mailarchive/message.php?msg_name=000001cada64%24b9f84200%242de8c600%24%40deacon%40arm.com

To summarise: in the context of polling loops, I believe that the performance impact of adding an smp_mb() to cpu_relax() for v6 SMP is minimal. If cpu_relax() is used in other scenarios, the performance impact can only be reviewed on a case-by-case basis.

Are there any other issues that need to be considered, or can I submit this to the patch system?

Thanks,

Will