LDREX/STREX and pre-emption on SMP hardware
Subsystems (2)
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.
Signature · 1 line
Rich
Quoted 7 lines · 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?
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.
Quoted 1 line
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.
Signature · 1 line
Catalin
Quoted 5 lines · Catalin Marinas
Quoted 4 lines · 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.
Quoted 7 lines
Quoted 3 lines
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_tofunction inentry-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
Quoted 10 lines · Jamie Lokier
Quoted 9 lines · Catalin Marinas
Quoted 7 lines · 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?
Quoted 6 lines
Quoted 3 lines
There is no need to since preemption means rescheduling which implies a call to the
__switch_tofunction inentry-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).
Signature · 1 line
Catalin
Quoted 18 lines · Catalin Marinas
Quoted 10 lines · Jamie Lokier
Quoted 9 lines · Catalin Marinas
Quoted 7 lines · 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?
Quoted 6 lines
Quoted 3 lines
There is no need to since preemption means rescheduling which implies a call to the
__switch_tofunction inentry-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.
Quoted 9 lines
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
Quoted 8 lines · David Xiao
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.
Signature · 1 line
Catalin
Quoted 10 lines · Catalin Marinas
Quoted 8 lines · David Xiao
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.
Quoted 5 lines
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
Quoted 16 lines · David Xiao
Quoted 10 lines · Catalin Marinas
Quoted 8 lines · David Xiao
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.Quoted 4 lines
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.
Quoted 4 lines
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).
Quoted 4 lines
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).
Signature · 1 line
Catalin
Quoted 50 lines · Catalin Marinas
Quoted 16 lines · David Xiao
Quoted 10 lines · Catalin Marinas
Quoted 8 lines · David Xiao
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.Quoted 4 lines
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.
Quoted 4 lines
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).
Quoted 4 lines
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)
Quoted 4 lines
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
Quoted 11 lines · Catalin Marinas
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
Quoted 27 lines · Jamie Lokier
Quoted 11 lines · Catalin Marinas
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);
Quoted 2 lines
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).
Quoted 5 lines
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?
Signature · 1 line
Catalin
Quoted 27 lines · Jamie Lokier
Quoted 11 lines · Catalin Marinas
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);
Quoted 2 lines
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.
Quoted 31 lines · Russell King - ARM Linux
Quoted 27 lines · Jamie Lokier
Quoted 11 lines · Catalin Marinas
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);Quoted 2 lines
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.
Quoted 8 lines
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.
Signature · 1 line
Catalin
Quoted 20 lines · Catalin Marinas
Quoted 8 lines · Russell King - ARM Linux
It doesn't matter though - consider two threads using LDREX on the same location:
T1 T2 LDREX LDREX STREXEQ (not satsified) STREXAs I replied to Jamie on a similar issue, you can have:
T1 T2
LDREX
LDREX
STREXEQ (satisfied, succeeds) LDREX
STREXEQ (not satisfied)
STREX (succeeds)
Quoted 1 line
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?
Signature · 1 line
Catalin
Quoted 21 lines · Catalin Marinas
Quoted 20 lines · Catalin Marinas
Quoted 8 lines · Russell King - ARM Linux
It doesn't matter though - consider two threads using LDREX on the same location:
T1 T2 LDREX LDREX STREXEQ (not satsified) STREXAs I replied to Jamie on a similar issue, you can have:
T1 T2
LDREX
LDREX
STREXEQ (satisfied, succeeds) LDREX
STREXEQ (not satisfied)
STREX (succeeds)
Quoted 4 lines
Quoted 1 line
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 <[email protected]>
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.
--- 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(-)
arch/arm/include/asm/atomic.h +2 -19
@@ -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;
arch/arm/kernel/entry-armv.S +0 -7
@@ -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)
arch/arm/kernel/entry-header.S +12 -0
@@ -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
Signature · 1 line
Catalin
Quoted 4 lines · Catalin Marinas
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.
Signature · 2 lines
Bill Gatliff bgat at billgatliff.com
Quoted 8 lines · Bill Gatliff
Quoted 3 lines · Catalin Marinas
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).
Signature · 1 line
Catalin
Quoted 5 lines · Catalin Marinas
Apart from the
switch_toandatomic_setmodifications, 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.
Signature · 2 lines
Bill Gatliff bgat at billgatliff.com
Quoted 3 lines · Catalin Marinas
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.
Quoted 10 lines · Russell King - ARM Linux
Quoted 3 lines · Catalin Marinas
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.
Quoted 6 lines · Russell King - ARM Linux
Quoted 5 lines · Russell King - ARM Linux
Quoted 3 lines · Catalin Marinas
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:
Quoted 8 lines
Quoted 5 lines
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.
Signature · 1 line
Catalin
Quoted 3 lines · Catalin Marinas
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 <[email protected]>
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.
--- 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(-)
arch/arm/include/asm/atomic.h +2 -19
@@ -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;
arch/arm/kernel/entry-armv.S +0 -7
@@ -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)
arch/arm/kernel/entry-header.S +14 -0
@@ -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
Signature · 1 line
Catalin
Quoted 16 lines · Catalin Marinas
#define
atomic_read(v)((v)->counter)arch/arm/include/asm/atomic.h +2
-6+#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;
arch/arm/include/asm/atomic.h line 45
Quoted 10 lines
arch/arm/include/asm/atomic.h +0
-10- - __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
Quoted 51 lines · Catalin Marinas
Quoted 31 lines · Russell King - ARM Linux
Quoted 27 lines · Jamie Lokier
Quoted 11 lines · Catalin Marinas
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);Quoted 2 lines
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.Quoted 8 lines
It doesn't matter though - consider two threads using LDREX on the same location:
T1 T2 LDREX LDREX STREXEQ (not satsified) STREXAs I replied to Jamie on a similar issue, you can have:
T1 T2
LDREX
LDREX
STREXEQ (satisfied, succeeds) LDREX
STREXEQ (not satisfied)
STREX (succeeds)
Quoted 1 line
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:
Quoted 2 lines
Quoted 2 lines
T1 T2 LDREX
CLREX
Quoted 2 lines
Quoted 2 lines
LDREX
STREXEQ (not satsified)
CLREX
Quoted 1 line
Quoted 1 line
STREX
which results in the STREX not succeeding. Ditto for your case.
Quoted 52 lines · Russell King - ARM Linux
Quoted 51 lines · Catalin Marinas
Quoted 31 lines · Russell King - ARM Linux
Quoted 27 lines · Jamie Lokier
Quoted 11 lines · Catalin Marinas
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);Quoted 2 lines
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.Quoted 8 lines
It doesn't matter though - consider two threads using LDREX on the same location:
T1 T2 LDREX LDREX STREXEQ (not satsified) STREXAs I replied to Jamie on a similar issue, you can have:
T1 T2
LDREX
LDREX
STREXEQ (satisfied, succeeds) LDREX
STREXEQ (not satisfied)
STREX (succeeds)
Quoted 4 lines
Quoted 1 line
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.
Quoted 5 lines
So the real sequence
is:Quoted 2 lines
Quoted 2 lines
T1 T2 LDREX
Quoted 7 lines
CLREX
Quoted 2 lines
Quoted 2 lines
LDREX
STREXEQ (not satsified)CLREX
Quoted 1 line
Quoted 1 line
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).
Signature · 1 line
Catalin
Quoted 3 lines · Catalin Marinas
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.
Quoted 7 lines · Russell King - ARM Linux
Quoted 3 lines · Catalin Marinas
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(-)
Signature · 1 line
Catalin
Quoted 4 lines · Catalin Marinas
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?
Quoted 3 lines
A similar reason for STR used in application signal handlers which are usually invoked without a
__switch_tocall (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
Quoted 7 lines · Jamie Lokier
Quoted 4 lines · Catalin Marinas
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.
Quoted 6 lines
Quoted 3 lines
A similar reason for STR used in application signal handlers which are usually invoked without a
__switch_tocall (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.
Signature · 1 line
Catalin