* RE: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
2013-04-06 18:01 ` [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT Linus Torvalds
@ 2013-04-06 19:54 ` Jacquiot, Aurelien
2013-04-09 16:33 ` [PATCH] tile: comment assumption about __insn_mtspr for <asm/irqflags.h> Chris Metcalf
1 sibling, 0 replies; 3+ messages in thread
From: Jacquiot, Aurelien @ 2013-04-06 19:54 UTC (permalink / raw)
To: Linus Torvalds, Vineet Gupta, Mark Salter
Cc: Thomas Gleixner, Christian Ruppert, Pierrick Hascoet,
Frederic Weisbecker, Steven Rostedt, Peter Zijlstra, Ingo Molnar,
Linux Kernel Mailing List, linux-arch@vger.kernel.org,
linux-c6x-dev@linux-c6x.org
Agreed. We will fix it.
Aurelien
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 12.654.784
-----Original Message-----
From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of Linus Torvalds
Sent: Saturday, April 06, 2013 8:01 PM
To: Vineet Gupta; Mark Salter; Jacquiot, Aurelien
Cc: Thomas Gleixner; Christian Ruppert; Pierrick Hascoet; Frederic Weisbecker; Steven Rostedt; Peter Zijlstra; Ingo Molnar; Linux Kernel Mailing List; linux-arch@vger.kernel.org; linux-c6x-dev@linux-c6x.org
Subject: Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
Looking around, it looks like c6x has the same bug.
Some other architectures (tile) have such subtle implementations (where is __insn_mtspr() defined?) that I have a hard time judging.
And maybe I missed something, but the rest seem ok.
Linus
On Sat, Apr 6, 2013 at 9:13 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> This is all *COMPLETELY* wrong.
>
> Neither the normal preempt macros, nor the plain spinlocks, should
> protect anything at all against interrupts.
>
> The real protection should come from the spin_lock_irqsave() in
> lock_timer_base(), not from spinlocks, and not from preemption.
>
> It sounds like ARC is completely buggered, and hasn't made the irq
> disable be a compiler barrier. That's an ARC bug, and it's a big one,
> and can affect a lot more than just the timers.
>
> So the real fix is to add a "memory" clobber to
> arch_local_irq_save/restore() and friends, so that the compiler
> doesn't get to cache memory state from the irq-enabled region into the
> irq-disabled one.
>
> Fix ARC, don't try to blame generic code. You should have asked
> yourself why only ARC saw this bug, when the code apparently works
> fine for everybody else!
>
> Linus
>
> On Sat, Apr 6, 2013 at 6:34 AM, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
>>> On 04/05/2013 10:06 AM, Vineet Gupta wrote:
>>> Hi Thomas,
>>>
>>> Given that we are closing on 3.9 release, and that one/more of these
>>> patches fix a real issue for us - can you please consider my earlier
>>> patch to fix
>>> timer_pending() only for 3.9
>>> [http://www.spinics.net/lists/kernel/msg1508224.html]
>>> This will be a localized / low risk change for this late in cycle.
>>>
>>> For 3.10 - assuming preempt_* change is blessed, we can revert this
>>> one and add that fuller/better fix.
>>>
>>> What do you think ?
>>>
>>> Thx,
>>> -Vineet
>>>
>>
>> Ping ! Sorry for pestering, but one of the fixes is needed before 3.9 goes out.
>>
>> Simple localized fix:
>> http://www.spinics.net/lists/kernel/msg1508224.html
>> Better but risky: http://www.spinics.net/lists/kernel/msg1510885.html
>>
>> Thx,
>> -Vineet
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH] tile: comment assumption about __insn_mtspr for <asm/irqflags.h>
2013-04-06 18:01 ` [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT Linus Torvalds
2013-04-06 19:54 ` Jacquiot, Aurelien
@ 2013-04-09 16:33 ` Chris Metcalf
1 sibling, 0 replies; 3+ messages in thread
From: Chris Metcalf @ 2013-04-09 16:33 UTC (permalink / raw)
To: Linus Torvalds
Cc: Vineet Gupta, Mark Salter, Aurelien Jacquiot, Thomas Gleixner,
Christian Ruppert, Pierrick Hascoet, Frederic Weisbecker,
Steven Rostedt, Peter Zijlstra, Ingo Molnar,
Linux Kernel Mailing List, linux-arch, linux-c6x-dev
The arch_local_irq_save(), etc., routines are required to function
as compiler barriers. They do, but it's subtle and requires knowing
that the gcc builtin __insn_mtspr() is marked as a memory clobber.
Provide a comment explaining the assumption.
Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
arch/tile/include/asm/irqflags.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Linus wrote:
> Some other architectures (tile) have such subtle implementations
> (where is __insn_mtspr() defined?) that I have a hard time judging.
diff --git a/arch/tile/include/asm/irqflags.h b/arch/tile/include/asm/irqflags.h
index 241c0bb..c96f9bb 100644
--- a/arch/tile/include/asm/irqflags.h
+++ b/arch/tile/include/asm/irqflags.h
@@ -40,7 +40,15 @@
#include <asm/percpu.h>
#include <arch/spr_def.h>
-/* Set and clear kernel interrupt masks. */
+/*
+ * Set and clear kernel interrupt masks.
+ *
+ * NOTE: __insn_mtspr() is a compiler builtin marked as a memory
+ * clobber. We rely on it being equivalent to a compiler barrier in
+ * this code since arch_local_irq_save() and friends must act as
+ * compiler barriers. This compiler semantic is baked into enough
+ * places that the compiler will maintain it going forward.
+ */
#if CHIP_HAS_SPLIT_INTR_MASK()
#if INT_PERF_COUNT < 32 || INT_AUX_PERF_COUNT < 32 || INT_MEM_ERROR >= 32
# error Fix assumptions about which word various interrupts are in
--
1.7.10.3
^ permalink raw reply related [flat|nested] 3+ messages in thread