[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Will Deacon]

The cpu_relax() macro is often used in the body of busy-wait loops to ensure
that the variable being spun on is re-loaded for each iteration. On the
ARM11MPCore processor [where loads are prioritised over stores], spinning in
such a loop will prevent the write buffer from draining. If a write contained
in the write buffer indirectly affects the variable being spun on, there is a
potential for deadlock. This deadlock is experienced when executing the KGDB
testsuite on an SMP ARM11MPCore configuration.

This patch changes the definition of cpu_relax() to smp_mb() for ARMv6 cores,
forcing a flushing of the write buffer on SMP systems before the next load
takes place. If the Kernel is not compiled for SMP support, this will expand
to a barrier() as before.

Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

Russell - the discussion on this last month seemed to draw to a close.
Can I submit this to the patch system please, or are there any outstanding
issues?

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

diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index 6a89567..7bed3da 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -91,7 +91,11 @@ extern void release_thread(struct task_struct *);
 
 unsigned long get_wchan(struct task_struct *p);
 
+#if __LINUX_ARM_ARCH__ == 6
+#define cpu_relax()			smp_mb()
+#else
 #define cpu_relax()			barrier()
+#endif
 
 /*
  * Create a new kernel thread
-- 
1.6.3.3

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Shilimkar, Santosh]

Will,

> -----Original Message-----
> From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux-arm-kernel-
> bounces at lists.infradead.org] On Behalf Of Will Deacon
> Sent: Thursday, May 27, 2010 8:42 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: Russell King - ARM Linux; Will Deacon
> Subject: [PATCH] ARM: change definition of cpu_relax() for ARM11MPCore
> 
> The cpu_relax() macro is often used in the body of busy-wait loops to ensure
> that the variable being spun on is re-loaded for each iteration. On the
> ARM11MPCore processor [where loads are prioritised over stores], spinning in
> such a loop will prevent the write buffer from draining. If a write contained
> in the write buffer indirectly affects the variable being spun on, there is a
> potential for deadlock. This deadlock is experienced when executing the KGDB
> testsuite on an SMP ARM11MPCore configuration.
> 
> This patch changes the definition of cpu_relax() to smp_mb() for ARMv6 cores,
> forcing a flushing of the write buffer on SMP systems before the next load
> takes place. If the Kernel is not compiled for SMP support, this will expand
> to a barrier() as before.
>
I have missed this thread. Is this not applicable for ARMv7 MP Cores as well??
 
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> 
> Russell - the discussion on this last month seemed to draw to a close.
> Can I submit this to the patch system please, or are there any outstanding
> issues?
> 
>  arch/arm/include/asm/processor.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
> index 6a89567..7bed3da 100644
> --- a/arch/arm/include/asm/processor.h
> +++ b/arch/arm/include/asm/processor.h
> @@ -91,7 +91,11 @@ extern void release_thread(struct task_struct *);
> 
>  unsigned long get_wchan(struct task_struct *p);
> 
> +#if __LINUX_ARM_ARCH__ == 6
> +#define cpu_relax()			smp_mb()
> +#else
>  #define cpu_relax()			barrier()
> +#endif
> 
>  /*
>   * Create a new kernel thread
> --
> 1.6.3.3

Regards,
Santosh

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Will Deacon]

Hi Santosh,

> > This patch changes the definition of cpu_relax() to smp_mb() for ARMv6 cores,
> > forcing a flushing of the write buffer on SMP systems before the next load
> > takes place. If the Kernel is not compiled for SMP support, this will expand
> > to a barrier() as before.
> >
> I have missed this thread. Is this not applicable for ARMv7 MP Cores as well??

You can pick up on some of the old thread here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-April/012874.html

Actually, the ChangeLog I wrote for that patch is better, so I'll update
my current one.

To answer your question; this problem only occurs on ARM11MPCore. For ARMv7MP,
stores are guaranteed to become visible eventually [i.e. they can't be held up
indefinitely by a sequence of aggresive loads].

An addition to the ARM ARM [Section A3.8.2] says:

* For an implementation that does not include the Multiprocessing Extensions,
it is IMPLEMENTATION DEFINED whether all writes become globally observed in
their shareability domain in a finite period of time, or whether in some cases,
software must execute a DSB to ensure the visibility of some writes.

* For an implementation that includes the Multiprocessing Extensions, all writes
become globally observed in their shareability domain in a finite period of time. 

Will

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Shilimkar, Santosh]

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon at arm.com]
> Sent: Thursday, May 27, 2010 10:24 PM
> To: Shilimkar, Santosh; linux-arm-kernel at lists.infradead.org
> Cc: Russell King - ARM Linux
> Subject: RE: [PATCH] ARM: change definition of cpu_relax() for ARM11MPCore
> 
> Hi Santosh,
> 
> > > This patch changes the definition of cpu_relax() to smp_mb() for ARMv6 cores,
> > > forcing a flushing of the write buffer on SMP systems before the next load
> > > takes place. If the Kernel is not compiled for SMP support, this will expand
> > > to a barrier() as before.
> > >
> > I have missed this thread. Is this not applicable for ARMv7 MP Cores as well??
> 
> You can pick up on some of the old thread here:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-April/012874.html
> 
> Actually, the ChangeLog I wrote for that patch is better, so I'll update
> my current one.
> 
> To answer your question; this problem only occurs on ARM11MPCore. For ARMv7MP,
> stores are guaranteed to become visible eventually [i.e. they can't be held up
> indefinitely by a sequence of aggresive loads].
> 
> An addition to the ARM ARM [Section A3.8.2] says:
> 
> * For an implementation that does not include the Multiprocessing Extensions,
> it is IMPLEMENTATION DEFINED whether all writes become globally observed in
> their shareability domain in a finite period of time, or whether in some cases,
> software must execute a DSB to ensure the visibility of some writes.
> 
> * For an implementation that includes the Multiprocessing Extensions, all writes
> become globally observed in their shareability domain in a finite period of time.
> 
Thanks. It's clear to me now.

Regards,
Santosh

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Will Deacon]

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.

Cc: KGDB Mailing List <kgdb-bugreport@lists.sourceforge.net>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Jason Wessel <jason.wessel@windriver.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/processor.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index 6a89567..da2078e 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -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

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Russell King - ARM Linux]

On Mon, Apr 12, 2010 at 06:23:58PM +0100, Will Deacon wrote:
> 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.

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?

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Will Deacon]

Hi Russell,

> On Mon, Apr 12, 2010 at 06:23:58PM +0100, Will Deacon wrote:
> > 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.
> 
> 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?

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

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Will Deacon]

The cpu_relax() macro is often used in the body of busy-wait loops to ensure
that the variable being spun on is re-loaded for each iteration. On the
ARM11MPCore processor [where loads are prioritised over stores], spinning in
such a loop will prevent the write buffer from draining. If a write contained
in the write buffer indirectly affects the variable being spun on, there is a
potential for deadlock. This deadlock is experienced when executing the KGDB
testsuite on an SMP ARM11MPCore configuration.

This patch changes the definition of cpu_relax() to smp_mb() for ARMv6 cores,
forcing a flushing of the write buffer on SMP systems. If the Kernel is not
compiled for SMP support, this will expand to a barrier() as before.

Cc: KGDB Mailing List <kgdb-bugreport@lists.sourceforge.net>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/processor.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index 6a89567..7bed3da 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -91,7 +91,11 @@ extern void release_thread(struct task_struct *);
 
 unsigned long get_wchan(struct task_struct *p);
 
+#if __LINUX_ARM_ARCH__ == 6
+#define cpu_relax()			smp_mb()
+#else
 #define cpu_relax()			barrier()
+#endif
 
 /*
  * Create a new kernel thread
-- 
1.6.3.3

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Russell King - ARM Linux]

On Tue, Mar 09, 2010 at 04:06:08PM +0000, Will Deacon wrote:
> The cpu_relax() macro is often used in the body of busy-wait loops to ensure
> that the variable being spun on is re-loaded for each iteration.

No, cpu_relax() exists to avoid x86 CPUs overheating - if you spin like
so:

	for(;;);

the CPU will overheat, so it's conventional to write:

	for(;;)
		cpu_relax();

so that architectures can prevent those kinds of problems occuring.

cpu_relax() is also defined to be a compiler barrier so that the compiler
reloads the variable on every iteration.

> This patch changes the definition of cpu_relax() to smp_mb() for ARMv6 cores,
> forcing a flushing of the write buffer on SMP systems. If the Kernel is not
> compiled for SMP support, this will expand to a barrier() as before.

I don't think this is correct.  You're making a macro do something on ARM
which no other platform, apart from blackfin (which I believe is wrong)
makes it do.

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Will Deacon]

Hi Russell,

> On Tue, Mar 09, 2010 at 04:06:08PM +0000, Will Deacon wrote:
> > The cpu_relax() macro is often used in the body of busy-wait loops to ensure
> > that the variable being spun on is re-loaded for each iteration.
> 
> No, cpu_relax() exists to avoid x86 CPUs overheating - if you spin like
> so:
> 
> 	for(;;);
> 
> the CPU will overheat, so it's conventional to write:
> 
> 	for(;;)
> 		cpu_relax();
> 
> so that architectures can prevent those kinds of problems occuring.

Ok. I was going by the comments in Documentation/volatile-considered-harmful.txt
where cpu_relax() is also used as a memory barrier.
 
> cpu_relax() is also defined to be a compiler barrier so that the compiler
> reloads the variable on every iteration.
> 
> > This patch changes the definition of cpu_relax() to smp_mb() for ARMv6 cores,
> > forcing a flushing of the write buffer on SMP systems. If the Kernel is not
> > compiled for SMP support, this will expand to a barrier() as before.
> 
> I don't think this is correct.  You're making a macro do something on ARM
> which no other platform, apart from blackfin (which I believe is wrong)
> makes it do.

In the KGDB case [where this cropped up], if cpu_relax() is left as it is, then
an smp_mb() is required in the architecture independent code. This also seems wrong
because it's only needed for the ARM11MPCore. There may also potentially be other
situations in the Kernel which are prone to deadlock because it is assumed that the
write buffer will always drain.

Since both solutions to this problem are nasty, it would be good to get some
clarification on the definition of cpu_relax() across the Kernel. If the KGDB guys
don't mind adding an extra memory barrier, we can solve the problem that way instead
and hope that it doesn't occur elsewhere.

Thoughts?

Cheers,

Will

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Russell King - ARM Linux]

On Tue, Mar 09, 2010 at 04:35:00PM -0000, Will Deacon wrote:
> Hi Russell,
> 
> > On Tue, Mar 09, 2010 at 04:06:08PM +0000, Will Deacon wrote:
> > > The cpu_relax() macro is often used in the body of busy-wait loops to ensure
> > > that the variable being spun on is re-loaded for each iteration.
> > 
> > No, cpu_relax() exists to avoid x86 CPUs overheating - if you spin like
> > so:
> > 
> > 	for(;;);
> > 
> > the CPU will overheat, so it's conventional to write:
> > 
> > 	for(;;)
> > 		cpu_relax();
> > 
> > so that architectures can prevent those kinds of problems occuring.
> 
> Ok. I was going by the comments in Documentation/volatile-considered-harmful.txt
> where cpu_relax() is also used as a memory barrier.

I thought you might; I've just submitted a patch for that to akpm, lkml
and linux-arch.

> In the KGDB case [where this cropped up], if cpu_relax() is left as it is, then
> an smp_mb() is required in the architecture independent code. This also seems wrong
> because it's only needed for the ARM11MPCore. There may also potentially be other
> situations in the Kernel which are prone to deadlock because it is assumed that the
> write buffer will always drain.

Why is KGDB being special about this?  Ah yes, it's being brain dead:

static atomic_t                 passive_cpu_wait[NR_CPUS];
static atomic_t                 cpu_in_kgdb[NR_CPUS];

        while (atomic_read(&passive_cpu_wait[cpu]))
                cpu_relax();

                for (i = 0; i < NR_CPUS; i++)
                        atomic_set(&passive_cpu_wait[i], 1);

                for (i = NR_CPUS-1; i >= 0; i--)
                        atomic_set(&passive_cpu_wait[i], 0);

        atomic_set(&cpu_in_kgdb[cpu], 1);

        atomic_set(&cpu_in_kgdb[cpu], 0);

        atomic_set(&cpu_in_kgdb[ks->cpu], 1);

        for_each_online_cpu(i) {
                while (!atomic_read(&cpu_in_kgdb[i]))
                        cpu_relax();
        }

        atomic_set(&cpu_in_kgdb[ks->cpu], 0);

                for_each_online_cpu(i) {
                        while (atomic_read(&cpu_in_kgdb[i]))
                                cpu_relax();
                }

        if (!atomic_read(&cpu_in_kgdb[cpu]) &&
                        atomic_read(&kgdb_active) != cpu &&
                        atomic_read(&cpu_in_kgdb[atomic_read(&kgdb_active)])) {

None of the above is atomic in any way, and I suspect that all these
places in kernel/kgdb.c are buggy.  As I've said in the past, just
because something is called 'atomic' does not make it so:

On atomic_set():
  The setting is atomic in that the return values of the atomic operations by
  all threads are guaranteed to be correct reflecting either the value that has
  been set with this operation or set with another operation.  A proper implicit
  or explicit memory barrier is needed before the value set with the operation
  is guaranteed to be readable with atomic_read from another thread.

On atomic_read():
  The read is atomic in that the return value is guaranteed to be one of the
  values initialized or modified with the interface operations if a proper
  implicit or explicit memory barrier is used after possible runtime
  initialization by any other thread and the value is modified only with the
  interface operations.  atomic_read does not guarantee that the runtime
  initialization by any other thread is visible yet, so the user of the
  interface must take care of that with a proper implicit or explicit memory
  barrier.

Clearly, kgdb is using atomic_set()/atomic_read() in a way which does not
match this documentation - it's certainly missing the barriers as required
by the above quoted paragraphs.

Let me repeat: atomic_set() and atomic_read() are NOT atomic.  There's
nothing atomic about them.  All they do is provide a pair of accessors
to the underlying value in the atomic type.  They are no different to
declaring a volatile int and reading/writing it directly.

[PATCH] ARM: change definition of cpu_relax() for ARM11MPCore

[Will Deacon]

> > Ok. I was going by the comments in Documentation/volatile-considered-harmful.txt
> > where cpu_relax() is also used as a memory barrier.
> 
> I thought you might; I've just submitted a patch for that to akpm, lkml
> and linux-arch.

Cheers, I'll take a read.

> > In the KGDB case [where this cropped up], if cpu_relax() is left as it is, then
> > an smp_mb() is required in the architecture independent code. This also seems wrong
> > because it's only needed for the ARM11MPCore. There may also potentially be other
> > situations in the Kernel which are prone to deadlock because it is assumed that the
> > write buffer will always drain.
> 
> Why is KGDB being special about this?  Ah yes, it's being brain dead:

<snip>

> Clearly, kgdb is using atomic_set()/atomic_read() in a way which does not
> match this documentation - it's certainly missing the barriers as required
> by the above quoted paragraphs.
> 
> Let me repeat: atomic_set() and atomic_read() are NOT atomic.  There's
> nothing atomic about them.  All they do is provide a pair of accessors
> to the underlying value in the atomic type.  They are no different to
> declaring a volatile int and reading/writing it directly.

Indeed. I'm not familiar enough with KGDB internals to dive in and look at all the
potential barrier conflicts, so I'll submit a patch that addresses the one that's
bitten me so far. Maybe it will motivate somebody else to take a closer look!

Cheers,

Will