Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Tue, 2013-04-09 at 07:32 -0700, Linus Torvalds wrote: > Something like the attached (still untested, although it seems to at > least compile) patch. Comments? Yes, makes me feel far more comfortable than this "asm volatile" business (which as you noted has holes in it). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Tue, 2013-04-09 at 07:32 -0700, Linus Torvalds wrote: Something like the attached (still untested, although it seems to at least compile) patch. Comments? Yes, makes me feel far more comfortable than this asm volatile business (which as you noted has holes in it). -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, Apr 8, 2013 at 8:07 AM, Linus Torvalds wrote: > On Mon, Apr 8, 2013 at 7:59 AM, Steven Rostedt wrote: >>> +/* This is only a barrier to other asms. Notably get_user/put_user */ >> >> Probably should add in the comment: >> >> " or anything else that can cause a hidden schedule. " >> > > Fair enough. And I just remembered why I thought UP was special - we > need to do the same thing about spinlocks, for the same reasons. > > So that "asm_barrier()" should probably be in along > with the "normal" barrier() definition. > I'm a moron. Yes, "asm_barrier()" is a valid barrier for asms. But without the "memory" clobber, it doesn't actually end up being a barrier to any normal C loads and stores from memory, so it doesn't actually help. So I suspect we need to just make UP spinlocks and preemption enable/disable be full compiler barriers after all. Something like the attached (still untested, although it seems to at least compile) patch. Comments? Linus patch.diff Description: Binary data
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, Apr 8, 2013 at 8:07 AM, Linus Torvalds torva...@linux-foundation.org wrote: On Mon, Apr 8, 2013 at 7:59 AM, Steven Rostedt srost...@redhat.com wrote: +/* This is only a barrier to other asms. Notably get_user/put_user */ Probably should add in the comment: or anything else that can cause a hidden schedule. Fair enough. And I just remembered why I thought UP was special - we need to do the same thing about spinlocks, for the same reasons. So that asm_barrier() should probably be in linux/compiler.h along with the normal barrier() definition. I'm a moron. Yes, asm_barrier() is a valid barrier for asms. But without the memory clobber, it doesn't actually end up being a barrier to any normal C loads and stores from memory, so it doesn't actually help. So I suspect we need to just make UP spinlocks and preemption enable/disable be full compiler barriers after all. Something like the attached (still untested, although it seems to at least compile) patch. Comments? Linus patch.diff Description: Binary data
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, Apr 8, 2013 at 7:59 AM, Steven Rostedt wrote:
>> +/* This is only a barrier to other asms. Notably get_user/put_user */
>
> Probably should add in the comment:
>
> " or anything else that can cause a hidden schedule. "
>
Fair enough. And I just remembered why I thought UP was special - we
need to do the same thing about spinlocks, for the same reasons.
So that "asm_barrier()" should probably be in along
with the "normal" barrier() definition.
*AND* somebody should re-check the gcc documentation on "volatile
asm". I'm pretty sure it used to say "not moved significantly,
including against each other" or something like that, but the gcc asm
docs have changed over time.
I'd hate to have to add a memory clobber to the get_user/put_user
asms, because that would *really* hurt. But maybe we could add some
other clobber ("cc" or similar) to make sure they can't be re-ordered
if the "volatile" isn't sufficient to make sure asms don't get
re-ordered wrt each other.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, 2013-04-08 at 07:50 -0700, Linus Torvalds wrote:
> ---
> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index 5a710b9c578e..465df1c13386 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -93,14 +93,17 @@ do { \
>
> #else /* !CONFIG_PREEMPT_COUNT */
>
> -#define preempt_disable() do { } while (0)
> -#define sched_preempt_enable_no_resched() do { } while (0)
> -#define preempt_enable_no_resched() do { } while (0)
> -#define preempt_enable() do { } while (0)
> -
> -#define preempt_disable_notrace() do { } while (0)
> -#define preempt_enable_no_resched_notrace() do { } while (0)
> -#define preempt_enable_notrace() do { } while (0)
> +/* This is only a barrier to other asms. Notably get_user/put_user */
Probably should add in the comment:
" or anything else that can cause a hidden schedule. "
-- Steve
> +#define asm_barrier() asm volatile("")
> +
> +#define preempt_disable() asm_barrier()
> +#define sched_preempt_enable_no_resched() asm_barrier()
> +#define preempt_enable_no_resched() asm_barrier()
> +#define preempt_enable() asm_barrier()
> +
> +#define preempt_disable_notrace() asm_barrier()
> +#define preempt_enable_no_resched_notrace() asm_barrier()
> +#define preempt_enable_notrace() asm_barrier()
>
> #endif /* CONFIG_PREEMPT_COUNT */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, Apr 8, 2013 at 7:31 AM, Steven Rostedt wrote:
>
> It requires gcc reordering the code to where a preempt can happen inside
> preempt_disable. And also put in a position where the preempt_disable
> code it gets added matters.
>
> Then if gcc does this, we need a page fault to occur with a get_user()
> operation, which in practice seldom happens as most get user operations
> are done on freshly modified memory.
>
> And then, it would require the page fault to cause a schedule. This is
> the most likely of the things needed to occur, but itself is not a
> problem.
>
> Then, the schedule would have to cause the data that is being protect by
> the preempt_disable() to be corrupted. Either by scheduling in another
> process that monkeys with the data. Or if it protects per-cpu data,
> scheduling to another CPU (for the SMP case only).
>
> If any of the above does not occur, then you wont see a bug.
Right. The gcc reordering is also hard to actually notice if it does
happen, so even testing the fix for this looks nontrivial.
Something like the appended (whitespace-damaged and TOTALLY UNTESTED)
might be sufficient, but..
Comments?
Linus
---
diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 5a710b9c578e..465df1c13386 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -93,14 +93,17 @@ do { \
#else /* !CONFIG_PREEMPT_COUNT */
-#define preempt_disable() do { } while (0)
-#define sched_preempt_enable_no_resched() do { } while (0)
-#define preempt_enable_no_resched() do { } while (0)
-#define preempt_enable() do { } while (0)
-
-#define preempt_disable_notrace() do { } while (0)
-#define preempt_enable_no_resched_notrace() do { } while (0)
-#define preempt_enable_notrace() do { } while (0)
+/* This is only a barrier to other asms. Notably get_user/put_user */
+#define asm_barrier() asm volatile("")
+
+#define preempt_disable() asm_barrier()
+#define sched_preempt_enable_no_resched() asm_barrier()
+#define preempt_enable_no_resched() asm_barrier()
+#define preempt_enable() asm_barrier()
+
+#define preempt_disable_notrace() asm_barrier()
+#define preempt_enable_no_resched_notrace() asm_barrier()
+#define preempt_enable_notrace() asm_barrier()
#endif /* CONFIG_PREEMPT_COUNT */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, 2013-04-08 at 15:37 +0200, Peter Zijlstra wrote: > That said, I can't remember ever having seen a BUG like this, even > though !PREEMPT is (or at least was) the most popular distro setting. It requires gcc reordering the code to where a preempt can happen inside preempt_disable. And also put in a position where the preempt_disable code it gets added matters. Then if gcc does this, we need a page fault to occur with a get_user() operation, which in practice seldom happens as most get user operations are done on freshly modified memory. And then, it would require the page fault to cause a schedule. This is the most likely of the things needed to occur, but itself is not a problem. Then, the schedule would have to cause the data that is being protect by the preempt_disable() to be corrupted. Either by scheduling in another process that monkeys with the data. Or if it protects per-cpu data, scheduling to another CPU (for the SMP case only). If any of the above does not occur, then you wont see a bug. This is highly unlikely to happen, but that's no excuse to not fix it. But it probably explains why we never saw a bug report. Heck, it may have happened, but it would be hard to reproduce, and just forgotten about. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Sun, 2013-04-07 at 21:48 -0700, Linus Torvalds wrote: > Ingo? Peter? I'm not sure anybody really uses UP+no-preempt on x86, > but it does seem to be a bug.. Comments? I believe a lot of people still use no-preempt. Well, at least voluntary preemption, which would have the same bug. I'm thinking that we may have just been lucky that gcc didn't move the get_user() into a place that would cause issues. Sounds like a bug to me. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Sun, 2013-04-07 at 21:48 -0700, Linus Torvalds wrote:
> That said, thinking about barriers and preemption made me realize that
> we do have a potential issue between: (a) non-preemption UP kernel
> (with no barriers in the preempt_enable/disable()) and (b)
> architectures that use inline asm without a memory clobber for
> get_user/put_user(). That includes x86.
>
> The reason is that I could imagine code like
>
> if (get_user(val, addr))
> return -EFAULT;
> preempt_disable();
> ... do something percpu ..
> preempt_enable();
>
> and it turns out that for non-preempt UP, we don't tell the compiler
> anywhere that it can't move the get_user past the preempt_disable. But
> the get_user() can cause a preemption point because of a page fault,
> obviously.
>
> I suspect that the best way to fix this ends up relying on the gcc
> "asm volatile" rules, and make the rule be that:
> - preempt_disable/enable have to generate an asm volatile() string
> (preferably just a ASM comment saying "preempt disable/enable")
> - get_user/put_user doesn't need to add a memory clobber, but needs
> to be done with an asm volatile too.
>
> Then the gcc "no reordering of volatile asms" should make the above be
> ok, without having to add an actual compiler memory barrier.
>
> Ingo? Peter? I'm not sure anybody really uses UP+no-preempt on x86,
> but it does seem to be a bug.. Comments?
Right, stuff between preempt_disable() and preempt_enable() is supposed
to appear atomic wrt scheduling contexts, allowing any schedule to
happen in between would violate this.
I'm not seeing how this would be UP only though, I can see the same
thing happening on SMP+no-preempt.
Also, is {get,put}_user() the only construct that can do this? If so,
using the "asm volatile" rules as described might be the best way,
otherwise making the PREEMPT_COUNT=n operations be compiler barriers
seems like the safer option.
That said, I can't remember ever having seen a BUG like this, even
though !PREEMPT is (or at least was) the most popular distro setting.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Sun, 2013-04-07 at 21:48 -0700, Linus Torvalds wrote:
That said, thinking about barriers and preemption made me realize that
we do have a potential issue between: (a) non-preemption UP kernel
(with no barriers in the preempt_enable/disable()) and (b)
architectures that use inline asm without a memory clobber for
get_user/put_user(). That includes x86.
The reason is that I could imagine code like
if (get_user(val, addr))
return -EFAULT;
preempt_disable();
... do something percpu ..
preempt_enable();
and it turns out that for non-preempt UP, we don't tell the compiler
anywhere that it can't move the get_user past the preempt_disable. But
the get_user() can cause a preemption point because of a page fault,
obviously.
I suspect that the best way to fix this ends up relying on the gcc
asm volatile rules, and make the rule be that:
- preempt_disable/enable have to generate an asm volatile() string
(preferably just a ASM comment saying preempt disable/enable)
- get_user/put_user doesn't need to add a memory clobber, but needs
to be done with an asm volatile too.
Then the gcc no reordering of volatile asms should make the above be
ok, without having to add an actual compiler memory barrier.
Ingo? Peter? I'm not sure anybody really uses UP+no-preempt on x86,
but it does seem to be a bug.. Comments?
Right, stuff between preempt_disable() and preempt_enable() is supposed
to appear atomic wrt scheduling contexts, allowing any schedule to
happen in between would violate this.
I'm not seeing how this would be UP only though, I can see the same
thing happening on SMP+no-preempt.
Also, is {get,put}_user() the only construct that can do this? If so,
using the asm volatile rules as described might be the best way,
otherwise making the PREEMPT_COUNT=n operations be compiler barriers
seems like the safer option.
That said, I can't remember ever having seen a BUG like this, even
though !PREEMPT is (or at least was) the most popular distro setting.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Sun, 2013-04-07 at 21:48 -0700, Linus Torvalds wrote: Ingo? Peter? I'm not sure anybody really uses UP+no-preempt on x86, but it does seem to be a bug.. Comments? I believe a lot of people still use no-preempt. Well, at least voluntary preemption, which would have the same bug. I'm thinking that we may have just been lucky that gcc didn't move the get_user() into a place that would cause issues. Sounds like a bug to me. -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, 2013-04-08 at 15:37 +0200, Peter Zijlstra wrote: That said, I can't remember ever having seen a BUG like this, even though !PREEMPT is (or at least was) the most popular distro setting. It requires gcc reordering the code to where a preempt can happen inside preempt_disable. And also put in a position where the preempt_disable code it gets added matters. Then if gcc does this, we need a page fault to occur with a get_user() operation, which in practice seldom happens as most get user operations are done on freshly modified memory. And then, it would require the page fault to cause a schedule. This is the most likely of the things needed to occur, but itself is not a problem. Then, the schedule would have to cause the data that is being protect by the preempt_disable() to be corrupted. Either by scheduling in another process that monkeys with the data. Or if it protects per-cpu data, scheduling to another CPU (for the SMP case only). If any of the above does not occur, then you wont see a bug. This is highly unlikely to happen, but that's no excuse to not fix it. But it probably explains why we never saw a bug report. Heck, it may have happened, but it would be hard to reproduce, and just forgotten about. -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, Apr 8, 2013 at 7:31 AM, Steven Rostedt srost...@redhat.com wrote:
It requires gcc reordering the code to where a preempt can happen inside
preempt_disable. And also put in a position where the preempt_disable
code it gets added matters.
Then if gcc does this, we need a page fault to occur with a get_user()
operation, which in practice seldom happens as most get user operations
are done on freshly modified memory.
And then, it would require the page fault to cause a schedule. This is
the most likely of the things needed to occur, but itself is not a
problem.
Then, the schedule would have to cause the data that is being protect by
the preempt_disable() to be corrupted. Either by scheduling in another
process that monkeys with the data. Or if it protects per-cpu data,
scheduling to another CPU (for the SMP case only).
If any of the above does not occur, then you wont see a bug.
Right. The gcc reordering is also hard to actually notice if it does
happen, so even testing the fix for this looks nontrivial.
Something like the appended (whitespace-damaged and TOTALLY UNTESTED)
might be sufficient, but..
Comments?
Linus
---
diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 5a710b9c578e..465df1c13386 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -93,14 +93,17 @@ do { \
#else /* !CONFIG_PREEMPT_COUNT */
-#define preempt_disable() do { } while (0)
-#define sched_preempt_enable_no_resched() do { } while (0)
-#define preempt_enable_no_resched() do { } while (0)
-#define preempt_enable() do { } while (0)
-
-#define preempt_disable_notrace() do { } while (0)
-#define preempt_enable_no_resched_notrace() do { } while (0)
-#define preempt_enable_notrace() do { } while (0)
+/* This is only a barrier to other asms. Notably get_user/put_user */
+#define asm_barrier() asm volatile()
+
+#define preempt_disable() asm_barrier()
+#define sched_preempt_enable_no_resched() asm_barrier()
+#define preempt_enable_no_resched() asm_barrier()
+#define preempt_enable() asm_barrier()
+
+#define preempt_disable_notrace() asm_barrier()
+#define preempt_enable_no_resched_notrace() asm_barrier()
+#define preempt_enable_notrace() asm_barrier()
#endif /* CONFIG_PREEMPT_COUNT */
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, 2013-04-08 at 07:50 -0700, Linus Torvalds wrote:
---
diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 5a710b9c578e..465df1c13386 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -93,14 +93,17 @@ do { \
#else /* !CONFIG_PREEMPT_COUNT */
-#define preempt_disable() do { } while (0)
-#define sched_preempt_enable_no_resched() do { } while (0)
-#define preempt_enable_no_resched() do { } while (0)
-#define preempt_enable() do { } while (0)
-
-#define preempt_disable_notrace() do { } while (0)
-#define preempt_enable_no_resched_notrace() do { } while (0)
-#define preempt_enable_notrace() do { } while (0)
+/* This is only a barrier to other asms. Notably get_user/put_user */
Probably should add in the comment:
or anything else that can cause a hidden schedule.
-- Steve
+#define asm_barrier() asm volatile()
+
+#define preempt_disable() asm_barrier()
+#define sched_preempt_enable_no_resched() asm_barrier()
+#define preempt_enable_no_resched() asm_barrier()
+#define preempt_enable() asm_barrier()
+
+#define preempt_disable_notrace() asm_barrier()
+#define preempt_enable_no_resched_notrace() asm_barrier()
+#define preempt_enable_notrace() asm_barrier()
#endif /* CONFIG_PREEMPT_COUNT */
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Mon, Apr 8, 2013 at 7:59 AM, Steven Rostedt srost...@redhat.com wrote: +/* This is only a barrier to other asms. Notably get_user/put_user */ Probably should add in the comment: or anything else that can cause a hidden schedule. Fair enough. And I just remembered why I thought UP was special - we need to do the same thing about spinlocks, for the same reasons. So that asm_barrier() should probably be in linux/compiler.h along with the normal barrier() definition. *AND* somebody should re-check the gcc documentation on volatile asm. I'm pretty sure it used to say not moved significantly, including against each other or something like that, but the gcc asm docs have changed over time. I'd hate to have to add a memory clobber to the get_user/put_user asms, because that would *really* hurt. But maybe we could add some other clobber (cc or similar) to make sure they can't be re-ordered if the volatile isn't sufficient to make sure asms don't get re-ordered wrt each other. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Sun, Apr 7, 2013 at 9:20 PM, Vineet Gupta wrote: > > Would you be OK if I send the single patch to ARC by email (for 3.9-rc7) or > you'd > rather have the pull request. I got distracted by thinking about user-accesses vs preemption, but yes, sending the ARC patch to fix things by email as a plain patch is fine. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Sun, Apr 7, 2013 at 9:20 PM, Vineet Gupta wrote: > > Christian had already proposed that change - only I was reluctant to take it > - as > local_irq_* is used heavily in a configuration of ARC700 linux where (!SMP) > cpu > doesn't support load-locked/store-conditional instructions - hence atomic > R-M-W > sequences need those routines. Adding a mem clobber would lead to pathetic > generated code hence the reason it was likely removed by me at some point in > time. Umm. The irq instructions absolutely *HAVE* to have the barriers. And yes, the R-M-W instructions (if they are protected by them) obviously need to have the load and the store inside the spinlocked region. > Also I'd thought that given that barriers are already present in PREEMPT_COUNT > variant of preempt_* macros we might as well add them to !PREEMPT_COUNT ones. That's absolutely insane. It's insane for two reasons: - it's not SUFFICIENT. Any user of irq-safety needs the loads and stores to stay inside the irq-safe region, and the preempt macros have absolutely nothing to do with that. - it's POINTLESS and WRONG. If you run on UP, and have preemption disabled, then there is no reason for a barrier in the preempt code, since moving code around them doesn't matter - nothing is ever going to preempt that. The thing is, the irq disable/enable sequence absolutely has to have a memory barrier in it, because if the compiler moves a load outside of the irq-protected region, then that is a bug. Now, if you want to play games with your atomics and do them with handcoded asm, go right ahead, but that really isn't an argument for getting everything else wrong. That said, thinking about barriers and preemption made me realize that we do have a potential issue between: (a) non-preemption UP kernel (with no barriers in the preempt_enable/disable()) and (b) architectures that use inline asm without a memory clobber for get_user/put_user(). That includes x86. The reason is that I could imagine code like if (get_user(val, addr)) return -EFAULT; preempt_disable(); ... do something percpu .. preempt_enable(); and it turns out that for non-preempt UP, we don't tell the compiler anywhere that it can't move the get_user past the preempt_disable. But the get_user() can cause a preemption point because of a page fault, obviously. I suspect that the best way to fix this ends up relying on the gcc "asm volatile" rules, and make the rule be that: - preempt_disable/enable have to generate an asm volatile() string (preferably just a ASM comment saying "preempt disable/enable") - get_user/put_user doesn't need to add a memory clobber, but needs to be done with an asm volatile too. Then the gcc "no reordering of volatile asms" should make the above be ok, without having to add an actual compiler memory barrier. Ingo? Peter? I'm not sure anybody really uses UP+no-preempt on x86, but it does seem to be a bug.. Comments? But the irq disable/enable sequences definitely need the memory clobber. Because caching memory that could be changed by an interrupt in registers is not good. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
Hi Linus, On 04/06/2013 09:43 PM, Linus Torvalds 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 Christian had already proposed that change - only I was reluctant to take it - as local_irq_* is used heavily in a configuration of ARC700 linux where (!SMP) cpu doesn't support load-locked/store-conditional instructions - hence atomic R-M-W sequences need those routines. Adding a mem clobber would lead to pathetic generated code hence the reason it was likely removed by me at some point in time. Also I'd thought that given that barriers are already present in PREEMPT_COUNT variant of preempt_* macros we might as well add them to !PREEMPT_COUNT ones. But thinking about it more - you are right (as you always are) - the situation I saw with timers code, could very well show up with vanilla local_irq_save/restore when a piece of code races with itself (specially for our mostly !SMP configs). Would you be OK if I send the single patch to ARC by email (for 3.9-rc7) or you'd rather have the pull request. Thx, -Vineet -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
Hi Linus, On 04/06/2013 09:43 PM, Linus Torvalds 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 Christian had already proposed that change - only I was reluctant to take it - as local_irq_* is used heavily in a configuration of ARC700 linux where (!SMP) cpu doesn't support load-locked/store-conditional instructions - hence atomic R-M-W sequences need those routines. Adding a mem clobber would lead to pathetic generated code hence the reason it was likely removed by me at some point in time. Also I'd thought that given that barriers are already present in PREEMPT_COUNT variant of preempt_* macros we might as well add them to !PREEMPT_COUNT ones. But thinking about it more - you are right (as you always are) - the situation I saw with timers code, could very well show up with vanilla local_irq_save/restore when a piece of code races with itself (specially for our mostly !SMP configs). Would you be OK if I send the single patch to ARC by email (for 3.9-rc7) or you'd rather have the pull request. Thx, -Vineet -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Sun, Apr 7, 2013 at 9:20 PM, Vineet Gupta vineet.gup...@synopsys.com wrote: Christian had already proposed that change - only I was reluctant to take it - as local_irq_* is used heavily in a configuration of ARC700 linux where (!SMP) cpu doesn't support load-locked/store-conditional instructions - hence atomic R-M-W sequences need those routines. Adding a mem clobber would lead to pathetic generated code hence the reason it was likely removed by me at some point in time. Umm. The irq instructions absolutely *HAVE* to have the barriers. And yes, the R-M-W instructions (if they are protected by them) obviously need to have the load and the store inside the spinlocked region. Also I'd thought that given that barriers are already present in PREEMPT_COUNT variant of preempt_* macros we might as well add them to !PREEMPT_COUNT ones. That's absolutely insane. It's insane for two reasons: - it's not SUFFICIENT. Any user of irq-safety needs the loads and stores to stay inside the irq-safe region, and the preempt macros have absolutely nothing to do with that. - it's POINTLESS and WRONG. If you run on UP, and have preemption disabled, then there is no reason for a barrier in the preempt code, since moving code around them doesn't matter - nothing is ever going to preempt that. The thing is, the irq disable/enable sequence absolutely has to have a memory barrier in it, because if the compiler moves a load outside of the irq-protected region, then that is a bug. Now, if you want to play games with your atomics and do them with handcoded asm, go right ahead, but that really isn't an argument for getting everything else wrong. That said, thinking about barriers and preemption made me realize that we do have a potential issue between: (a) non-preemption UP kernel (with no barriers in the preempt_enable/disable()) and (b) architectures that use inline asm without a memory clobber for get_user/put_user(). That includes x86. The reason is that I could imagine code like if (get_user(val, addr)) return -EFAULT; preempt_disable(); ... do something percpu .. preempt_enable(); and it turns out that for non-preempt UP, we don't tell the compiler anywhere that it can't move the get_user past the preempt_disable. But the get_user() can cause a preemption point because of a page fault, obviously. I suspect that the best way to fix this ends up relying on the gcc asm volatile rules, and make the rule be that: - preempt_disable/enable have to generate an asm volatile() string (preferably just a ASM comment saying preempt disable/enable) - get_user/put_user doesn't need to add a memory clobber, but needs to be done with an asm volatile too. Then the gcc no reordering of volatile asms should make the above be ok, without having to add an actual compiler memory barrier. Ingo? Peter? I'm not sure anybody really uses UP+no-preempt on x86, but it does seem to be a bug.. Comments? But the irq disable/enable sequences definitely need the memory clobber. Because caching memory that could be changed by an interrupt in registers is not good. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
On Sun, Apr 7, 2013 at 9:20 PM, Vineet Gupta vineet.gup...@synopsys.com wrote: Would you be OK if I send the single patch to ARC by email (for 3.9-rc7) or you'd rather have the pull request. I got distracted by thinking about user-accesses vs preemption, but yes, sending the ARC patch to fix things by email as a plain patch is fine. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
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: linus...@gmail.com [mailto:linus...@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-a...@vger.kernel.org; linux-c6x-...@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
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
> 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
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
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 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 > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
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.gup...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 torva...@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.gup...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
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: linus...@gmail.com [mailto:linus...@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-a...@vger.kernel.org; linux-c6x-...@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 torva...@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.gup...@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
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
Hi Thomas, On 04/04/2013 08:58 PM, Christian Ruppert wrote: > Hi Vineet, > > Our stress testing campaign has just successfully completed on this > patch. It seems to solve several issues we have seen in unpatched > versions, amongst others the original timer issue, a crash in hrtimer > rb-tree manipulation etc. > > Greetings, > Christian 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
Hi Vineet, Our stress testing campaign has just successfully completed on this patch. It seems to solve several issues we have seen in unpatched versions, amongst others the original timer issue, a crash in hrtimer rb-tree manipulation etc. Greetings, Christian On Wed, Apr 03, 2013 at 07:41:22PM +0530, Vineet Gupta wrote: > spinlocks built in a !PREEMPT_COUNT config don't have the compiler > barrier provided by preempt_* routines. This can break lot of code which > relies on barrier semantics. > > This manifested as random crashes in timer code when stress testing > ARC Linux (3.9-rc3): !SMP && !PREEMPT_COUNT > > [...] > > Signed-off-by: Vineet Gupta > Reported-by: Christian Ruppert Tested-by: Christian Ruppert > Cc: Thomas Gleixner > Cc: Christian Ruppert > Cc: Pierrick Hascoet > Cc: Robert Love > Cc: kpreempt-t...@lists.sourceforge.net > Cc: Frederic Weisbecker > Cc: Steven Rostedt > Cc: Peter Zijlstra > Cc: Ingo Molnar > Cc: linux-kernel@vger.kernel.org > [...] -- Christian Ruppert , /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
Hi Vineet, Our stress testing campaign has just successfully completed on this patch. It seems to solve several issues we have seen in unpatched versions, amongst others the original timer issue, a crash in hrtimer rb-tree manipulation etc. Greetings, Christian On Wed, Apr 03, 2013 at 07:41:22PM +0530, Vineet Gupta wrote: spinlocks built in a !PREEMPT_COUNT config don't have the compiler barrier provided by preempt_* routines. This can break lot of code which relies on barrier semantics. This manifested as random crashes in timer code when stress testing ARC Linux (3.9-rc3): !SMP !PREEMPT_COUNT [...] Signed-off-by: Vineet Gupta vgu...@synopsys.com Reported-by: Christian Ruppert christian.rupp...@abilis.com Tested-by: Christian Ruppert christian.rupp...@abilis.com Cc: Thomas Gleixner t...@linutronix.de Cc: Christian Ruppert christian.rupp...@abilis.com Cc: Pierrick Hascoet pierrick.hasc...@abilis.com Cc: Robert Love r...@tech9.net Cc: kpreempt-t...@lists.sourceforge.net Cc: Frederic Weisbecker fweis...@gmail.com Cc: Steven Rostedt srost...@redhat.com Cc: Peter Zijlstra pet...@infradead.org Cc: Ingo Molnar mi...@kernel.org Cc: linux-kernel@vger.kernel.org [...] -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [PATCH] Gaurantee spinlocks implicit barrier for !PREEMPT_COUNT
Hi Thomas, On 04/04/2013 08:58 PM, Christian Ruppert wrote: Hi Vineet, Our stress testing campaign has just successfully completed on this patch. It seems to solve several issues we have seen in unpatched versions, amongst others the original timer issue, a crash in hrtimer rb-tree manipulation etc. Greetings, Christian 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
