LDREX/STREX and pre-emption on SMP hardware

LDREX/STREX and pre-emption on SMP hardware

[Richard Crewe]

Section A2.9.3 of the ARM architecture ref. manual seems to imply that 
ldrex/strex instruction pairs won't work correctly if they are nested 
due to pre-emption.

Should a strex instruction be added to the low-level interrupt handler 
or should all ldrex/strex instruction pairs be protected from 
pre-emption by disabling interrupts?

I have a suspicion that this may only cause problems on SMP systems.

-- 
Rich

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Fri, 2009-08-21 at 16:07 +0100, Richard Crewe wrote:
> Section A2.9.3 of the ARM architecture ref. manual seems to imply that 
> ldrex/strex instruction pairs won't work correctly if they are nested 
> due to pre-emption.
> 
> Should a strex instruction be added to the low-level interrupt handler 
> or should all ldrex/strex instruction pairs be protected from 
> pre-emption by disabling interrupts?

There is no need to since preemption means rescheduling which implies a
call to the __switch_to function in entry-armv.S. This function clears
the exclusive monitor state explicitly.

> I have a suspicion that this may only cause problems on SMP systems.

In some situations, there may be a problem on UP systems as LDREX/STREX
are used on non-SMP kernels as well for some atomic operations.

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Jamie Lokier]

Catalin Marinas wrote:
> On Fri, 2009-08-21 at 16:07 +0100, Richard Crewe wrote:
> > Section A2.9.3 of the ARM architecture ref. manual seems to imply that 
> > ldrex/strex instruction pairs won't work correctly if they are nested 
> > due to pre-emption.
> > 
> > Should a strex instruction be added to the low-level interrupt handler 
> > or should all ldrex/strex instruction pairs be protected from 
> > pre-emption by disabling interrupts?
> 
> There is no need to since preemption means rescheduling which implies a
> call to the __switch_to function in entry-armv.S. This function clears
> the exclusive monitor state explicitly.

What about when an interrupt handler uses ldrex/strex?  There is no
call to __switch_to.

-- Jamie

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Fri, 2009-08-21 at 16:50 +0100, Jamie Lokier wrote:
> Catalin Marinas wrote:
> > On Fri, 2009-08-21 at 16:07 +0100, Richard Crewe wrote:
> > > Section A2.9.3 of the ARM architecture ref. manual seems to imply that 
> > > ldrex/strex instruction pairs won't work correctly if they are nested 
> > > due to pre-emption.
> > > 
> > > Should a strex instruction be added to the low-level interrupt handler 
> > > or should all ldrex/strex instruction pairs be protected from 
> > > pre-emption by disabling interrupts?
> > 
> > There is no need to since preemption means rescheduling which implies a
> > call to the __switch_to function in entry-armv.S. This function clears
> > the exclusive monitor state explicitly.
> 
> What about when an interrupt handler uses ldrex/strex?  There is no
> call to __switch_to.

I don't see any issues with interrupt handlers, the exclusives should
work as expected.

The problem is with user apps using the exclusives and the same virtual
address could be used with LDREX/STREX in two different applications.
The (local) exclusive monitor may consider a LDREX in the one app and
STREX in the other app are part of the same pair and store the data
successfully.

Interrupt handlers only use the kernel mapping which doesn't change with
context switches.

BTW, some time ago I sent a patch to clear the exclusive monitor at
every exception entry (interrupt, aborts etc.) but wasn't accepted. The
main reason for that was to be able to only use STR for atomic_set()
rather than the combination of LDREX/STREX we have now. A similar reason
for STR used in application signal handlers which are usually invoked
without a __switch_to call (unless the signal handler is in a different
application from the running one).

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[David Xiao]

On Fri, 2009-08-21 at 08:58 -0700, Catalin Marinas wrote:
> On Fri, 2009-08-21 at 16:50 +0100, Jamie Lokier wrote:
> > Catalin Marinas wrote:
> > > On Fri, 2009-08-21 at 16:07 +0100, Richard Crewe wrote:
> > > > Section A2.9.3 of the ARM architecture ref. manual seems to imply that 
> > > > ldrex/strex instruction pairs won't work correctly if they are nested 
> > > > due to pre-emption.
> > > > 
> > > > Should a strex instruction be added to the low-level interrupt handler 
> > > > or should all ldrex/strex instruction pairs be protected from 
> > > > pre-emption by disabling interrupts?
> > > 
> > > There is no need to since preemption means rescheduling which implies a
> > > call to the __switch_to function in entry-armv.S. This function clears
> > > the exclusive monitor state explicitly.
> > 
> > What about when an interrupt handler uses ldrex/strex?  There is no
> > call to __switch_to.
> 
> I don't see any issues with interrupt handlers, the exclusives should
> work as expected.
> 
> The problem is with user apps using the exclusives and the same virtual
> address could be used with LDREX/STREX in two different applications.
> The (local) exclusive monitor may consider a LDREX in the one app and
> STREX in the other app are part of the same pair and store the data
> successfully.
> 
The DDI0406A ARM V7 Architecture Reference Manual (section A3.4.1) seems
to indicate that the exclusive monitor is tagging/matching the physical
memory address accessed by the LDREX/STREX instructions.

And in the same document (section A3.4.5), it seems to suggest that the
reason we need to do CLREX during the context switch is that because the
IsExclusiveLocal() implementation does not have to do memory
address/size check, but just the exclusive state check.

Could you confirm that, Catalin? Thanks.

David

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Fri, 2009-08-21 at 14:29 -0700, David Xiao wrote:
> The DDI0406A ARM V7 Architecture Reference Manual (section A3.4.1) seems
> to indicate that the exclusive monitor is tagging/matching the physical
> memory address accessed by the LDREX/STREX instructions.
> 
> And in the same document (section A3.4.5), it seems to suggest that the
> reason we need to do CLREX during the context switch is that because the
> IsExclusiveLocal() implementation does not have to do memory
> address/size check, but just the exclusive state check.

Yes, that's correct. And the reason we don't need this in interrupt
handlers is that we would never call a STREX without a preceding LDREX
or just a LDREX without a being followed by a STREX and interrupt
handlers are in the worst case nested rather than freely preemptible.

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[David Xiao]

On Mon, 2009-08-24 at 08:44 -0700, Catalin Marinas wrote:
> On Fri, 2009-08-21 at 14:29 -0700, David Xiao wrote:
> > The DDI0406A ARM V7 Architecture Reference Manual (section A3.4.1) seems
> > to indicate that the exclusive monitor is tagging/matching the physical
> > memory address accessed by the LDREX/STREX instructions.
> > 
> > And in the same document (section A3.4.5), it seems to suggest that the
> > reason we need to do CLREX during the context switch is that because the
> > IsExclusiveLocal() implementation does not have to do memory
> > address/size check, but just the exclusive state check.
> 
> Yes, that's correct. And the reason we don't need this in interrupt
> handlers is that we would never call a STREX without a preceding LDREX
> or just a LDREX without a being followed by a STREX and interrupt
> handlers are in the worst case nested rather than freely preemptible.
> 

If an IRQ handler is registered with IRQF_DISABLED, then the handling of
this IRQ will not be preempted by any other IRQ handlers; however, if it
is not using that flag, which is the common case, that IRQ handler could
be interrupted/preempted by another different IRQ handler though.

Meanwhile, if we could assume that interrupt handlers are always using
the LDREX/CLREX in pairs, then the same thing could be assumed for any
other contexts in the system, kernel/user threads. Therefore, I do not
think that we can make that assumption.
  
Given that, I think we need to add the same CLREX for the switching of
the ISR context as well.

David

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-08-24 at 10:14 -0700, David Xiao wrote:
> On Mon, 2009-08-24 at 08:44 -0700, Catalin Marinas wrote:
> > On Fri, 2009-08-21 at 14:29 -0700, David Xiao wrote:
> > > The DDI0406A ARM V7 Architecture Reference Manual (section A3.4.1) seems
> > > to indicate that the exclusive monitor is tagging/matching the physical
> > > memory address accessed by the LDREX/STREX instructions.
> > > 
> > > And in the same document (section A3.4.5), it seems to suggest that the
> > > reason we need to do CLREX during the context switch is that because the
> > > IsExclusiveLocal() implementation does not have to do memory
> > > address/size check, but just the exclusive state check.
> > 
> > Yes, that's correct. And the reason we don't need this in interrupt
> > handlers is that we would never call a STREX without a preceding LDREX
> > or just a LDREX without a being followed by a STREX and interrupt
> > handlers are in the worst case nested rather than freely preemptible.
> 
> If an IRQ handler is registered with IRQF_DISABLED, then the handling of
> this IRQ will not be preempted by any other IRQ handlers; however, if it
> is not using that flag, which is the common case, that IRQ handler could
> be interrupted/preempted by another different IRQ handler though.

Yes, but the exclusives are not affected (see below).

> Meanwhile, if we could assume that interrupt handlers are always using
> the LDREX/CLREX in pairs, then the same thing could be assumed for any
> other contexts in the system, kernel/user threads. Therefore, I do not
> think that we can make that assumption.

In user space you can get (T1, T2 are threads):

T1			T2
LDREX
			LDREX
STREX (succeeds)
			STREX (fails)


So the STREX in T1 shouldn't really succeed because there was a context
switch and LDREX in T2 (this particular case may not be a problem but if
you create a situation with 3 threads that is incorrect).

With interrupts (I1, I2 interrupt handlers)

I1			I2
LDREX
			LDREX
			STREX (succeeds)
STREX (fails)

In the interrupt case, they are nested so the STREX in I2 is always
executed before STREX in I1 (you can extrapolate with several nested
interrupts).

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[David Xiao]

On Mon, 2009-08-24 at 10:41 -0700, Catalin Marinas wrote:
> On Mon, 2009-08-24 at 10:14 -0700, David Xiao wrote:
> > On Mon, 2009-08-24 at 08:44 -0700, Catalin Marinas wrote:
> > > On Fri, 2009-08-21 at 14:29 -0700, David Xiao wrote:
> > > > The DDI0406A ARM V7 Architecture Reference Manual (section A3.4.1) seems
> > > > to indicate that the exclusive monitor is tagging/matching the physical
> > > > memory address accessed by the LDREX/STREX instructions.
> > > > 
> > > > And in the same document (section A3.4.5), it seems to suggest that the
> > > > reason we need to do CLREX during the context switch is that because the
> > > > IsExclusiveLocal() implementation does not have to do memory
> > > > address/size check, but just the exclusive state check.
> > > 
> > > Yes, that's correct. And the reason we don't need this in interrupt
> > > handlers is that we would never call a STREX without a preceding LDREX
> > > or just a LDREX without a being followed by a STREX and interrupt
> > > handlers are in the worst case nested rather than freely preemptible.
> > 
> > If an IRQ handler is registered with IRQF_DISABLED, then the handling of
> > this IRQ will not be preempted by any other IRQ handlers; however, if it
> > is not using that flag, which is the common case, that IRQ handler could
> > be interrupted/preempted by another different IRQ handler though.
> 
> Yes, but the exclusives are not affected (see below).
> 
> > Meanwhile, if we could assume that interrupt handlers are always using
> > the LDREX/CLREX in pairs, then the same thing could be assumed for any
> > other contexts in the system, kernel/user threads. Therefore, I do not
> > think that we can make that assumption.
> 
> In user space you can get (T1, T2 are threads):
> 
> T1			T2
> LDREX
> 			LDREX
> STREX (succeeds)
> 			STREX (fails)
> 
> 
> So the STREX in T1 shouldn't really succeed because there was a context
> switch and LDREX in T2 (this particular case may not be a problem but if
> you create a situation with 3 threads that is incorrect).
> 
> With interrupts (I1, I2 interrupt handlers)
> 
> I1			I2
> LDREX
> 			LDREX
> 			STREX (succeeds)
> STREX (fails)
> 
> In the interrupt case, they are nested so the STREX in I2 is always
> executed before STREX in I1 (you can extrapolate with several nested
> interrupts).
> 
Thanks for the illustration, Catalin.

Yes, so if all the ISRs "behave" and are indeed using the LDREX/STREX in
pairs we shall not have a problem. But if one ISR does not follow this,
it can not only "hang itself" but actually affect other ISRs in the
system. In your above example, if I2 is only using LDREX without STREX
then I1 can actually succeed in its STREX.

Of course, I am probably over-concerned since all the
atomic/synchronization primitives are already in place inside the kernel
with correct LDREX/STREX in pairs, the chance of inventing some new
atomic operation with the "bad behavior" by an ISR writer is almost
none...

David

LDREX/STREX and pre-emption on SMP hardware

[Jamie Lokier]

Catalin Marinas wrote:
> With interrupts (I1, I2 interrupt handlers)
> 
> I1			I2
> LDREX
> 			LDREX
> 			STREX (succeeds)
> STREX (fails)
> 
> In the interrupt case, they are nested so the STREX in I2 is always
> executed before STREX in I1 (you can extrapolate with several nested
> interrupts).

This assumes LDREX/STREX are always called in pairs.  But this is in
fact _not_ the case.  Take a look at atomic_cmpxchg:

	do {
		__asm__ __volatile__("@ atomic_cmpxchg\n"
		"ldrex	%1, [%2]\n"
		"mov	%0, #0\n"
		"teq	%1, %3\n"
		"strexeq %0, %4, [%2]\n"
		    : "=&r" (res), "=&r" (oldval)
		    : "r" (&ptr->counter), "Ir" (old), "r" (new)
		    : "cc");
	} while (res);

In the case where ptr->counter != old, STREX is not executed, and the
do{...}while loop does not loop.  Thus LDREX/STREX aren't paired.

It may be that atomic_cmpxchg() is always called in a loop, but if so
I don't think that's a documented requirement for all callers.

A simple solution is to either call CLREXNE after STREXEQ, or change
STREXEQ to STREX and change the logic so that if ptr->counter != old,
the value it tries to store is what it read.  The latter uses fewer
instructions - and even eliminates the do...while, making it really
compact, but may cause more cache line dirtying.

If you think I'm right and tell me what you prefer I'll prepare a
patch.

-- Jamie

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-09-14 at 02:43 +0100, Jamie Lokier wrote:
> Catalin Marinas wrote:
> > With interrupts (I1, I2 interrupt handlers)
> > 
> > I1			I2
> > LDREX
> > 			LDREX
> > 			STREX (succeeds)
> > STREX (fails)
> > 
> > In the interrupt case, they are nested so the STREX in I2 is always
> > executed before STREX in I1 (you can extrapolate with several nested
> > interrupts).
> 
> This assumes LDREX/STREX are always called in pairs.  But this is in
> fact _not_ the case.  Take a look at atomic_cmpxchg:
> 
> 	do {
> 		__asm__ __volatile__("@ atomic_cmpxchg\n"
> 		"ldrex	%1, [%2]\n"
> 		"mov	%0, #0\n"
> 		"teq	%1, %3\n"
> 		"strexeq %0, %4, [%2]\n"
> 		    : "=&r" (res), "=&r" (oldval)
> 		    : "r" (&ptr->counter), "Ir" (old), "r" (new)
> 		    : "cc");
> 	} while (res);
> 
> In the case where ptr->counter != old, STREX is not executed, and the
> do{...}while loop does not loop.  Thus LDREX/STREX aren't paired.

I think you are correct here, though we may not have code to trigger
this (but it needs to be fixed anyway).

> A simple solution is to either call CLREXNE after STREXEQ, or change
> STREXEQ to STREX and change the logic so that if ptr->counter != old,
> the value it tries to store is what it read.  The latter uses fewer
> instructions - and even eliminates the do...while, making it really
> compact, but may cause more cache line dirtying.

The do...while loop is still needed for the case where STREX fails
because of a STREX on a different thread of execution (same CPU or a
different one).

I would prefer the CLREXNE because of cache line dirtying (which is even
more expensive on SMP). But on ARM1136 you may not have the CLREX
instruction, so either dummy STREX would be useful. Maybe you could make
it conditional on CONFIG_CPU_32v6K.

Alternatively, a return from exception could clear the exclusive monitor
- this would also allow us to use STR for atomic_set() (if it has any
benefits). It also solves a similar issue with user-space atomic
operations in signal handlers (I had some past discussion with Hans
Boehm on the atomic and memory model in C++0x and they intend to use STR
for atomic set).

I'm more in favour of the latter option as there isn't really any
significant performance impact but I remember Russell objected to it in
the past. If that's still the case, I would prefer the conditional
compilation with STREX and CLREXNE.

Russell, any opinion here?

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Russell King - ARM Linux]

On Mon, Sep 14, 2009 at 02:43:53AM +0100, Jamie Lokier wrote:
> Catalin Marinas wrote:
> > With interrupts (I1, I2 interrupt handlers)
> > 
> > I1			I2
> > LDREX
> > 			LDREX
> > 			STREX (succeeds)
> > STREX (fails)
> > 
> > In the interrupt case, they are nested so the STREX in I2 is always
> > executed before STREX in I1 (you can extrapolate with several nested
> > interrupts).
> 
> This assumes LDREX/STREX are always called in pairs.  But this is in
> fact _not_ the case.  Take a look at atomic_cmpxchg:
> 
> 	do {
> 		__asm__ __volatile__("@ atomic_cmpxchg\n"
> 		"ldrex	%1, [%2]\n"
> 		"mov	%0, #0\n"
> 		"teq	%1, %3\n"
> 		"strexeq %0, %4, [%2]\n"
> 		    : "=&r" (res), "=&r" (oldval)
> 		    : "r" (&ptr->counter), "Ir" (old), "r" (new)
> 		    : "cc");
> 	} while (res);
> 
> In the case where ptr->counter != old, STREX is not executed, and the
> do{...}while loop does not loop.  Thus LDREX/STREX aren't paired.

It doesn't matter though - consider two threads using LDREX on the same
location:

	T1		T2
	LDREX
			LDREX
			STREXEQ (not satsified)
	STREX


T1's STREX ultimately succeeds.  The value stored there hasn't changed
since it did the LDREX, so it's perfectly fine for the STREX to occur.
This is really no different from this case:

	T1		T2
	LDREX
			LDREX
	STREX (succeeds)
			STREX (fails)


What is not fine is for there to be STREX instructions without a
preceding LDREX unless it can be guaranteed that the location being
stored to doesn't matter whether the store succeeds or not.

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-09-14 at 11:00 +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 14, 2009 at 02:43:53AM +0100, Jamie Lokier wrote:
> > Catalin Marinas wrote:
> > > With interrupts (I1, I2 interrupt handlers)
> > > 
> > > I1			I2
> > > LDREX
> > > 			LDREX
> > > 			STREX (succeeds)
> > > STREX (fails)
> > > 
> > > In the interrupt case, they are nested so the STREX in I2 is always
> > > executed before STREX in I1 (you can extrapolate with several nested
> > > interrupts).
> > 
> > This assumes LDREX/STREX are always called in pairs.  But this is in
> > fact _not_ the case.  Take a look at atomic_cmpxchg:
> > 
> > 	do {
> > 		__asm__ __volatile__("@ atomic_cmpxchg\n"
> > 		"ldrex	%1, [%2]\n"
> > 		"mov	%0, #0\n"
> > 		"teq	%1, %3\n"
> > 		"strexeq %0, %4, [%2]\n"
> > 		    : "=&r" (res), "=&r" (oldval)
> > 		    : "r" (&ptr->counter), "Ir" (old), "r" (new)
> > 		    : "cc");
> > 	} while (res);
> > 
> > In the case where ptr->counter != old, STREX is not executed, and the
> > do{...}while loop does not loop.  Thus LDREX/STREX aren't paired.
> 
> It doesn't matter though - consider two threads using LDREX on the same
> location:
> 
> 	T1		T2
> 	LDREX
> 			LDREX
> 			STREXEQ (not satsified)
> 	STREX

As I replied to Jamie on a similar issue, you can have:

T1			T2
LDREX
			LDREX
			STREXEQ (satisfied, succeeds)
			LDREX
			STREXEQ (not satisfied)
STREX (succeeds)

Though this may be an unlikely sequence.

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-09-14 at 11:06 +0100, Catalin Marinas wrote:
> On Mon, 2009-09-14 at 11:00 +0100, Russell King - ARM Linux wrote:
> > It doesn't matter though - consider two threads using LDREX on the same
> > location:
> > 
> > 	T1		T2
> > 	LDREX
> > 			LDREX
> > 			STREXEQ (not satsified)
> > 	STREX
> 
> As I replied to Jamie on a similar issue, you can have:
> 
> T1			T2
> LDREX
> 			LDREX
> 			STREXEQ (satisfied, succeeds)
> 			LDREX
> 			STREXEQ (not satisfied)
> STREX (succeeds)
> 
> Though this may be an unlikely sequence.

Searching for STREXEQ, it looks like there are several other locking
functions using it in spinlock.h. Do we have a real problem with these?

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-09-14 at 12:47 +0100, Catalin Marinas wrote:
> On Mon, 2009-09-14 at 11:06 +0100, Catalin Marinas wrote:
> > On Mon, 2009-09-14 at 11:00 +0100, Russell King - ARM Linux wrote:
> > > It doesn't matter though - consider two threads using LDREX on the same
> > > location:
> > > 
> > > 	T1		T2
> > > 	LDREX
> > > 			LDREX
> > > 			STREXEQ (not satsified)
> > > 	STREX
> > 
> > As I replied to Jamie on a similar issue, you can have:
> > 
> > T1			T2
> > LDREX
> > 			LDREX
> > 			STREXEQ (satisfied, succeeds)
> > 			LDREX
> > 			STREXEQ (not satisfied)
> > STREX (succeeds)
> > 
> > Though this may be an unlikely sequence.
> 
> Searching for STREXEQ, it looks like there are several other locking
> functions using it in spinlock.h. Do we have a real problem with these?

And here's an untested patch to clear the exclusive monitor on the
exception return path. If you are OK with the idea, I'll do some testing
before pushing it for upstream:


Clear the exclusive monitor when returning from an exception

From: Catalin Marinas <catalin.marinas@arm.com>

The patch adds a CLREX or dummy STREX to the exception return path. This
is needed because several atomic/locking operations use a pair of
LDREX/STREXEQ and the EQ condition may not always be satisfied. This
would leave the exclusive monitor status set and may cause problems with
atomic/locking operations in the interrupted code.

With this patch, the atomic_set() operation can be a simple STR
instruction (on SMP systems, the global exclusive monitor is cleared by
STR anyway). Clearing the exclusive monitor during context switch is no
longer needed as this is handled by the exception return path anyway.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Jamie Lokier <jamie@shareable.org>
---
 arch/arm/include/asm/atomic.h  |   21 ++-------------------
 arch/arm/kernel/entry-armv.S   |    7 -------
 arch/arm/kernel/entry-header.S |   12 ++++++++++++
 3 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index 9ed2377..74045f4 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -20,30 +20,15 @@
 #ifdef __KERNEL__
 
 #define atomic_read(v)	((v)->counter)
+#define atomic_set(v,i)	(((v)->counter) = (i))
 
 #if __LINUX_ARM_ARCH__ >= 6
 
 /*
  * ARMv6 UP and SMP safe atomic ops.  We use load exclusive and
  * store exclusive to ensure that these are atomic.  We may loop
- * to ensure that the update happens.  Writing to 'v->counter'
- * without using the following operations WILL break the atomic
- * nature of these ops.
+ * to ensure that the update happens.
  */
-static inline void atomic_set(atomic_t *v, int i)
-{
-	unsigned long tmp;
-
-	__asm__ __volatile__("@ atomic_set\n"
-"1:	ldrex	%0, [%1]\n"
-"	strex	%0, %2, [%1]\n"
-"	teq	%0, #0\n"
-"	bne	1b"
-	: "=&r" (tmp)
-	: "r" (&v->counter), "r" (i)
-	: "cc");
-}
-
 static inline void atomic_add(int i, atomic_t *v)
 {
 	unsigned long tmp;
@@ -163,8 +148,6 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#define atomic_set(v,i)	(((v)->counter) = (i))
-
 static inline int atomic_add_return(int i, atomic_t *v)
 {
 	unsigned long flags;
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 468425f..a7196d2 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -736,13 +736,6 @@ ENTRY(__switch_to)
 #ifdef CONFIG_MMU
 	ldr	r6, [r2, #TI_CPU_DOMAIN]
 #endif
-#if __LINUX_ARM_ARCH__ >= 6
-#ifdef CONFIG_CPU_32v6K
-	clrex
-#else
-	strex	r5, r4, [ip]			@ Clear exclusive monitor
-#endif
-#endif
 #if defined(CONFIG_HAS_TLS_REG)
 	mcr	p15, 0, r3, c13, c0, 3		@ set TLS register
 #elif !defined(CONFIG_TLS_REG_EMUL)
diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S
index a4eaf4f..24875db 100644
--- a/arch/arm/kernel/entry-header.S
+++ b/arch/arm/kernel/entry-header.S
@@ -75,11 +75,21 @@
 
 #ifndef CONFIG_THUMB2_KERNEL
 	.macro	svc_exit, rpsr
+#ifdef CONFIG_CPU_32v6K
+	clrex					@ clear the exclusive monitor
+#else
+	strex	r0, r1, [sp, #-4]		@ clear the exclusive monitor
+#endif
 	msr	spsr_cxsf, \rpsr
 	ldmia	sp, {r0 - pc}^			@ load r0 - pc, cpsr
 	.endm
 
 	.macro	restore_user_regs, fast = 0, offset = 0
+#ifdef CONFIG_CPU_32v6K
+	clrex					@ clear the exclusive monitor
+#else
+	strex	r1, lr, [sp, #-4]		@ clear the exclusive monitor
+#endif
 	ldr	r1, [sp, #\offset + S_PSR]	@ get calling cpsr
 	ldr	lr, [sp, #\offset + S_PC]!	@ get pc
 	msr	spsr_cxsf, r1			@ save in spsr_svc
@@ -98,6 +108,7 @@
 	.endm
 #else	/* CONFIG_THUMB2_KERNEL */
 	.macro	svc_exit, rpsr
+	clrex					@ clear the exclusive monitor
 	ldr	r0, [sp, #S_SP]			@ top of the stack
 	ldr	r1, [sp, #S_PC]			@ return address
 	tst	r0, #4				@ orig stack 8-byte aligned?
@@ -110,6 +121,7 @@
 	.endm
 
 	.macro	restore_user_regs, fast = 0, offset = 0
+	clrex					@ clear the exclusive monitor
 	mov	r2, sp
 	load_user_sp_lr r2, r3, \offset + S_SP	@ calling sp, lr
 	ldr	r1, [sp, #\offset + S_PSR]	@ get calling cpsr


-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Bill Gatliff]

Catalin Marinas wrote:
> And here's an untested patch to clear the exclusive monitor on the
> exception return path. If you are OK with the idea, I'll do some testing
> before pushing it for upstream:
>   

How are you going to "test" for this one? It's such a corner case, it's 
going to be tricky to validate that your changes are actually getting 
test coverage methinks...


b.g.

-- 
Bill Gatliff
bgat at billgatliff.com

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-09-14 at 07:43 -0500, Bill Gatliff wrote:
> Catalin Marinas wrote:
> > And here's an untested patch to clear the exclusive monitor on the
> > exception return path. If you are OK with the idea, I'll do some testing
> > before pushing it for upstream:
> 
> How are you going to "test" for this one? It's such a corner case, it's 
> going to be tricky to validate that your changes are actually getting 
> test coverage methinks...

I'm just testing it to make sure it doesn't break the compilation :-). I
can do a few more tests like running LTP in parallel but it may not find
any problems. It's strange that we haven't hit a bug yet (and we run
some intensive testing for CPU validation in ARM) or maybe we blamed
something else.

Apart from the switch_to and atomic_set modifications, the patch cannot
do more harm. As for these functions, I don't see any reason why the
modifications would not be correct but we can delay the merging until we
are sure (anyway, the more people looking at the patch, the better).

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Bill Gatliff]

Catalin Marinas wrote:
> Apart from the switch_to and atomic_set modifications, the patch cannot
> do more harm. As for these functions, I don't see any reason why the
> modifications would not be correct but we can delay the merging until we
> are sure (anyway, the more people looking at the patch, the better).
>   

Clearly, you understand the fundamentals behind my question--- and you 
have a pretty satisfactory answer as well.

That's all I was after, really.  Carry on.  :)


b.g.

-- 
Bill Gatliff
bgat at billgatliff.com

LDREX/STREX and pre-emption on SMP hardware

[Russell King - ARM Linux]

On Mon, Sep 14, 2009 at 01:21:21PM +0100, Catalin Marinas wrote:
> And here's an untested patch to clear the exclusive monitor on the
> exception return path. If you are OK with the idea, I'll do some testing
> before pushing it for upstream:

However, a plain STR is not guaranteed to change the state of the
exclusive monitor - it is implementation defined whether STR does
or not.

This is true for both the local and global exclusive monitors.  This
means that if you use STR on a location which is being used by
exclusive access instructions on another CPU:

	CPU0	CPU1
	LDREX
		STR
	STREX

results in the STR getting lost since the STREX can complete.

Basically, don't use STR on exclusive access locations - it's
certainly unsafe in a SMP environment.

LDREX/STREX and pre-emption on SMP hardware

[Russell King - ARM Linux]

On Mon, Sep 14, 2009 at 03:09:09PM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 14, 2009 at 01:21:21PM +0100, Catalin Marinas wrote:
> > And here's an untested patch to clear the exclusive monitor on the
> > exception return path. If you are OK with the idea, I'll do some testing
> > before pushing it for upstream:
> 
> However, a plain STR is not guaranteed to change the state of the
> exclusive monitor - it is implementation defined whether STR does
> or not.
> 
> This is true for both the local and global exclusive monitors.

... only for the same local CPU.  A STR done by another CPU to the
address being monitored clears the global monitor.

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-09-14 at 15:21 +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 14, 2009 at 03:09:09PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Sep 14, 2009 at 01:21:21PM +0100, Catalin Marinas wrote:
> > > And here's an untested patch to clear the exclusive monitor on the
> > > exception return path. If you are OK with the idea, I'll do some testing
> > > before pushing it for upstream:
> > 
> > However, a plain STR is not guaranteed to change the state of the
> > exclusive monitor - it is implementation defined whether STR does
> > or not.
> > 
> > This is true for both the local and global exclusive monitors.
> 
> ... only for the same local CPU.  A STR done by another CPU to the
> address being monitored clears the global monitor.

Yes, that's A3.4.2 in the ARM ARM. And with this patch, we take care for
the local monitor by clearing the monitor state on exception return.

I think the reason we weren't seeing any issues is that many locking
operations take place with interrupts disabled.

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-09-14 at 13:21 +0100, Catalin Marinas wrote:
> And here's an untested patch to clear the exclusive monitor on the
> exception return path. If you are OK with the idea, I'll do some testing
> before pushing it for upstream:

That's an update that compiles. It takes care of the fact that a dummy
STREX may actually succeed even if a previous LDREX didn't use the same
address (it's not as nice as having a CLREX but that's the situation
with non-ARMv6K hardware):


Clear the exclusive monitor when returning from an exception

From: Catalin Marinas <catalin.marinas@arm.com>

The patch adds a CLREX or dummy STREX to the exception return path. This
is needed because several atomic/locking operations use a pair of
LDREX/STREXEQ and the EQ condition may not always be satisfied. This
would leave the exclusive monitor status set and may cause problems with
atomic/locking operations in the interrupted code.

With this patch, the atomic_set() operation can be a simple STR
instruction (on SMP systems, the global exclusive monitor is cleared by
STR anyway). Clearing the exclusive monitor during context switch is no
longer needed as this is handled by the exception return path anyway.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Jamie Lokier <jamie@shareable.org>
---
 arch/arm/include/asm/atomic.h  |   21 ++-------------------
 arch/arm/kernel/entry-armv.S   |    7 -------
 arch/arm/kernel/entry-header.S |   14 ++++++++++++++
 3 files changed, 16 insertions(+), 26 deletions(-)

diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
index 9ed2377..74045f4 100644
--- a/arch/arm/include/asm/atomic.h
+++ b/arch/arm/include/asm/atomic.h
@@ -20,30 +20,15 @@
 #ifdef __KERNEL__
 
 #define atomic_read(v)	((v)->counter)
+#define atomic_set(v,i)	(((v)->counter) = (i))
 
 #if __LINUX_ARM_ARCH__ >= 6
 
 /*
  * ARMv6 UP and SMP safe atomic ops.  We use load exclusive and
  * store exclusive to ensure that these are atomic.  We may loop
- * to ensure that the update happens.  Writing to 'v->counter'
- * without using the following operations WILL break the atomic
- * nature of these ops.
+ * to ensure that the update happens.
  */
-static inline void atomic_set(atomic_t *v, int i)
-{
-	unsigned long tmp;
-
-	__asm__ __volatile__("@ atomic_set\n"
-"1:	ldrex	%0, [%1]\n"
-"	strex	%0, %2, [%1]\n"
-"	teq	%0, #0\n"
-"	bne	1b"
-	: "=&r" (tmp)
-	: "r" (&v->counter), "r" (i)
-	: "cc");
-}
-
 static inline void atomic_add(int i, atomic_t *v)
 {
 	unsigned long tmp;
@@ -163,8 +148,6 @@ static inline void atomic_clear_mask(unsigned long mask, unsigned long *addr)
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
-#define atomic_set(v,i)	(((v)->counter) = (i))
-
 static inline int atomic_add_return(int i, atomic_t *v)
 {
 	unsigned long flags;
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 468425f..a7196d2 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -736,13 +736,6 @@ ENTRY(__switch_to)
 #ifdef CONFIG_MMU
 	ldr	r6, [r2, #TI_CPU_DOMAIN]
 #endif
-#if __LINUX_ARM_ARCH__ >= 6
-#ifdef CONFIG_CPU_32v6K
-	clrex
-#else
-	strex	r5, r4, [ip]			@ Clear exclusive monitor
-#endif
-#endif
 #if defined(CONFIG_HAS_TLS_REG)
 	mcr	p15, 0, r3, c13, c0, 3		@ set TLS register
 #elif !defined(CONFIG_TLS_REG_EMUL)
diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S
index a4eaf4f..e17e3c3 100644
--- a/arch/arm/kernel/entry-header.S
+++ b/arch/arm/kernel/entry-header.S
@@ -76,13 +76,25 @@
 #ifndef CONFIG_THUMB2_KERNEL
 	.macro	svc_exit, rpsr
 	msr	spsr_cxsf, \rpsr
+#if defined(CONFIG_CPU_32v6K)
+	clrex					@ clear the exclusive monitor
 	ldmia	sp, {r0 - pc}^			@ load r0 - pc, cpsr
+#elif defined (CONFIG_CPU_V6)
+	ldr	r0, [sp]
+	strex	r1, r2, [sp]			@ clear the exclusive monitor
+	ldmib	sp, {r1 - pc}^			@ load r1 - pc, cpsr
+#endif
 	.endm
 
 	.macro	restore_user_regs, fast = 0, offset = 0
 	ldr	r1, [sp, #\offset + S_PSR]	@ get calling cpsr
 	ldr	lr, [sp, #\offset + S_PC]!	@ get pc
 	msr	spsr_cxsf, r1			@ save in spsr_svc
+#if defined(CONFIG_CPU_32v6K)
+	clrex					@ clear the exclusive monitor
+#elif defined (CONFIG_CPU_V6)
+	strex	r1, r2, [sp]			@ clear the exclusive monitor
+#endif
 	.if	\fast
 	ldmdb	sp, {r1 - lr}^			@ get calling r1 - lr
 	.else
@@ -98,6 +110,7 @@
 	.endm
 #else	/* CONFIG_THUMB2_KERNEL */
 	.macro	svc_exit, rpsr
+	clrex					@ clear the exclusive monitor
 	ldr	r0, [sp, #S_SP]			@ top of the stack
 	ldr	r1, [sp, #S_PC]			@ return address
 	tst	r0, #4				@ orig stack 8-byte aligned?
@@ -110,6 +123,7 @@
 	.endm
 
 	.macro	restore_user_regs, fast = 0, offset = 0
+	clrex					@ clear the exclusive monitor
 	mov	r2, sp
 	load_user_sp_lr r2, r3, \offset + S_SP	@ calling sp, lr
 	ldr	r1, [sp, #\offset + S_PSR]	@ get calling cpsr


-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Jamie Lokier]

Catalin Marinas wrote:
>  #define atomic_read(v)	((v)->counter)
> +#define atomic_set(v,i)	(((v)->counter) = (i))
>  
>  #if __LINUX_ARM_ARCH__ >= 6
>  
>  /*
>   * ARMv6 UP and SMP safe atomic ops.  We use load exclusive and
>   * store exclusive to ensure that these are atomic.  We may loop
> - * to ensure that the update happens.  Writing to 'v->counter'
> - * without using the following operations WILL break the atomic
> - * nature of these ops.
> + * to ensure that the update happens.
>   */
> -static inline void atomic_set(atomic_t *v, int i)
> -{
> -	unsigned long tmp;
> -
> -	__asm__ __volatile__("@ atomic_set\n"
> -"1:	ldrex	%0, [%1]\n"
> -"	strex	%0, %2, [%1]\n"
> -"	teq	%0, #0\n"
> -"	bne	1b"
> -	: "=&r" (tmp)
> -	: "r" (&v->counter), "r" (i)
> -	: "cc");
> -}

I was going to say this won't work, because I'd read this in
<asm/atomic.h>:

    /* Atomic operations are already serializing on ARM */
    #define smp_mb__before_atomic_dec()    barrier()
    [etc]

and imagined that atomic_set() needed to provide the same.

But then but then I spotted Documentation/atomic_ops.txt:

    *** WARNING: atomic_read() and atomic_set() DO NOT IMPLY BARRIERS! ***

So that's alright then.

Besides, it turns out the comment in <asm/atomic.h> was wrong, and was
removed recently, with explicit barrier instructions being added where
expected.

Regarding the patch, a command saying _why_ it's ok for atomic_set()
to be a simple assignment would be good:

    /*
     * On ARM, ordinary assignment (str instruction) doesn't clear the
     * _local_ strex/ldrex monitor on some implementations.  The
     * reason we can use it for atomic_set() is the clrex or dummy
     * strex done on every exception return and context switch.
     */

-- Jamie

LDREX/STREX and pre-emption on SMP hardware

[Russell King - ARM Linux]

On Mon, Sep 14, 2009 at 11:06:13AM +0100, Catalin Marinas wrote:
> On Mon, 2009-09-14 at 11:00 +0100, Russell King - ARM Linux wrote:
> > On Mon, Sep 14, 2009 at 02:43:53AM +0100, Jamie Lokier wrote:
> > > Catalin Marinas wrote:
> > > > With interrupts (I1, I2 interrupt handlers)
> > > > 
> > > > I1			I2
> > > > LDREX
> > > > 			LDREX
> > > > 			STREX (succeeds)
> > > > STREX (fails)
> > > > 
> > > > In the interrupt case, they are nested so the STREX in I2 is always
> > > > executed before STREX in I1 (you can extrapolate with several nested
> > > > interrupts).
> > > 
> > > This assumes LDREX/STREX are always called in pairs.  But this is in
> > > fact _not_ the case.  Take a look at atomic_cmpxchg:
> > > 
> > > 	do {
> > > 		__asm__ __volatile__("@ atomic_cmpxchg\n"
> > > 		"ldrex	%1, [%2]\n"
> > > 		"mov	%0, #0\n"
> > > 		"teq	%1, %3\n"
> > > 		"strexeq %0, %4, [%2]\n"
> > > 		    : "=&r" (res), "=&r" (oldval)
> > > 		    : "r" (&ptr->counter), "Ir" (old), "r" (new)
> > > 		    : "cc");
> > > 	} while (res);
> > > 
> > > In the case where ptr->counter != old, STREX is not executed, and the
> > > do{...}while loop does not loop.  Thus LDREX/STREX aren't paired.
> > 
> > It doesn't matter though - consider two threads using LDREX on the same
> > location:
> > 
> > 	T1		T2
> > 	LDREX
> > 			LDREX
> > 			STREXEQ (not satsified)
> > 	STREX
> 
> As I replied to Jamie on a similar issue, you can have:
> 
> T1			T2
> LDREX
> 			LDREX
> 			STREXEQ (satisfied, succeeds)
> 			LDREX
> 			STREXEQ (not satisfied)
> STREX (succeeds)
> 
> Though this may be an unlikely sequence.

Actually, both of these can't happen because they imply a context switch
and a context switch clears the exclusive monitor.  So the real sequence
is:

> > 	T1		T2
> > 	LDREX
		CLREX
> > 			LDREX
> > 			STREXEQ (not satsified)
		CLREX
> > 	STREX

which results in the STREX not succeeding.  Ditto for your case.

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-09-14 at 15:23 +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 14, 2009 at 11:06:13AM +0100, Catalin Marinas wrote:
> > On Mon, 2009-09-14 at 11:00 +0100, Russell King - ARM Linux wrote:
> > > On Mon, Sep 14, 2009 at 02:43:53AM +0100, Jamie Lokier wrote:
> > > > Catalin Marinas wrote:
> > > > > With interrupts (I1, I2 interrupt handlers)
> > > > > 
> > > > > I1			I2
> > > > > LDREX
> > > > > 			LDREX
> > > > > 			STREX (succeeds)
> > > > > STREX (fails)
> > > > > 
> > > > > In the interrupt case, they are nested so the STREX in I2 is always
> > > > > executed before STREX in I1 (you can extrapolate with several nested
> > > > > interrupts).
> > > > 
> > > > This assumes LDREX/STREX are always called in pairs.  But this is in
> > > > fact _not_ the case.  Take a look at atomic_cmpxchg:
> > > > 
> > > > 	do {
> > > > 		__asm__ __volatile__("@ atomic_cmpxchg\n"
> > > > 		"ldrex	%1, [%2]\n"
> > > > 		"mov	%0, #0\n"
> > > > 		"teq	%1, %3\n"
> > > > 		"strexeq %0, %4, [%2]\n"
> > > > 		    : "=&r" (res), "=&r" (oldval)
> > > > 		    : "r" (&ptr->counter), "Ir" (old), "r" (new)
> > > > 		    : "cc");
> > > > 	} while (res);
> > > > 
> > > > In the case where ptr->counter != old, STREX is not executed, and the
> > > > do{...}while loop does not loop.  Thus LDREX/STREX aren't paired.
> > > 
> > > It doesn't matter though - consider two threads using LDREX on the same
> > > location:
> > > 
> > > 	T1		T2
> > > 	LDREX
> > > 			LDREX
> > > 			STREXEQ (not satsified)
> > > 	STREX
> > 
> > As I replied to Jamie on a similar issue, you can have:
> > 
> > T1			T2
> > LDREX
> > 			LDREX
> > 			STREXEQ (satisfied, succeeds)
> > 			LDREX
> > 			STREXEQ (not satisfied)
> > STREX (succeeds)
> > 
> > Though this may be an unlikely sequence.
> 
> Actually, both of these can't happen because they imply a context switch
> and a context switch clears the exclusive monitor.  

T2 here in an interrupt handler (misleadingly called thread, though the
architecture people here in ARM just call everything a thread of
execution). Jamie's reply here was referring to interrupt handlers.

> So the real sequence
> is:
> 
> > > 	T1		T2
> > > 	LDREX
> 		CLREX
> > > 			LDREX
> > > 			STREXEQ (not satsified)
> 		CLREX
> > > 	STREX
> 
> which results in the STREX not succeeding.  Ditto for your case.

The other case where I replied was discussing the need for CLREX at
context switch and that's needed to get the correct behaviour (T1 and T2
are OS threads in this case).

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Russell King - ARM Linux]

On Mon, Sep 14, 2009 at 03:29:36PM +0100, Catalin Marinas wrote:
> The other case where I replied was discussing the need for CLREX at
> context switch and that's needed to get the correct behaviour (T1 and T2
> are OS threads in this case).

Well, lets get your patch in then.  Please try to get it to me (patch
system / git pull) by tomorrow evening, thanks.

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Fri, 2009-09-18 at 21:20 +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 14, 2009 at 03:29:36PM +0100, Catalin Marinas wrote:
> > The other case where I replied was discussing the need for CLREX at
> > context switch and that's needed to get the correct behaviour (T1 and T2
> > are OS threads in this case).
> 
> Well, lets get your patch in then.  Please try to get it to me (patch
> system / git pull) by tomorrow evening, thanks.

The patch pretty much as it was originally posted but with Jamie's
comment is available on the for-rmk branch below (the patch has
atomic_set modified to STR and CLREX removed from __switch_to as it is
no longer needed - explicit calls to schedule() don't have a problem as
they don't happen in the middle of an atomic LDREX/STREX construct).

I added two more patches to the for-rmk branch - the W macro fix in
unified.h and kernel (not user) undefined instructions handling in
Thumb-2 (I run it on a model without VFP and reading FPSID was causing
an oops).

You can merge the branch as it is but you can skip the Thumb-2 patches
by merging for-rmk^ or for-rmk^^ (in case you find any issues, I'm not
sure I'll be able to reply before tomorrow night).

Thanks.


The following changes since commit df58bee21ed218cb7dfb561a590b1bd2a99531cf:
  Linus Torvalds (1):
        Merge branch 'x86-mce-for-linus' of git://git.kernel.org/.../tip/linux-2.6-tip

are available in the git repository at:

  git://linux-arm.org/linux-2.6 for-rmk

Alexey Dobriyan (1):
      Fix "W" macro in arch/arm/include/asm/unified.h

Catalin Marinas (2):
      Clear the exclusive monitor when returning from an exception
      Thumb-2: Correctly handle undefined instructions in the kernel

 arch/arm/include/asm/atomic.h  |   26 +++++++-------------------
 arch/arm/include/asm/unified.h |    4 ++++
 arch/arm/kernel/entry-armv.S   |   19 +++++++++++--------
 arch/arm/kernel/entry-header.S |   14 ++++++++++++++
 4 files changed, 36 insertions(+), 27 deletions(-)

-- 
Catalin

LDREX/STREX and pre-emption on SMP hardware

[Jamie Lokier]

Catalin Marinas wrote:
> BTW, some time ago I sent a patch to clear the exclusive monitor at
> every exception entry (interrupt, aborts etc.) but wasn't accepted. The
> main reason for that was to be able to only use STR for atomic_set()
> rather than the combination of LDREX/STREX we have now.

Would CLREX+STR work, and would it be an improvement?

> A similar reason for STR used in application signal handlers which
> are usually invoked without a __switch_to call (unless the signal
> handler is in a different application from the running one).

How about adding CLREX to the signal handling path?  Not exception
entry, but the part which sets up the signal frame.

If userspace can use STR for atomic_set normally, it's quite obscure
(and very difficult to detect) that that's fine everywhere except in
signal handlers, and even more obscure if it means siglongjmp() and
setcontext() can cause unrelated atomic ops to fail.

-- Jamie

LDREX/STREX and pre-emption on SMP hardware

[Catalin Marinas]

On Mon, 2009-08-24 at 22:12 +0100, Jamie Lokier wrote:
> Catalin Marinas wrote:
> > BTW, some time ago I sent a patch to clear the exclusive monitor at
> > every exception entry (interrupt, aborts etc.) but wasn't accepted. The
> > main reason for that was to be able to only use STR for atomic_set()
> > rather than the combination of LDREX/STREX we have now.
> 
> Would CLREX+STR work, and would it be an improvement?

IMHO, yes. You use CLREX at every exception entry and make the
atomic_set() function simply use STR. But the argument was that
atomic_set() isn't used very often anyway.

> > A similar reason for STR used in application signal handlers which
> > are usually invoked without a __switch_to call (unless the signal
> > handler is in a different application from the running one).
> 
> How about adding CLREX to the signal handling path?  Not exception
> entry, but the part which sets up the signal frame.

Yes, that's the solution, only that I've never had the time to revisit
it and send another patch.

-- 
Catalin