[PATCH] remove volatile from nmi.c
Steven Rostedt
memory barriers. So if something is incorrect, please let me know.
This patch removes the volatile keyword from arch/i386/kernel/nmi.c.
The first removal is trivial, since the barrier in the while loop makes
it unnecessary. (as proved in "[patch] spinlocks: remove 'volatile'"
thread)
http://marc.theaimsgroup.com/?l=linux-kernel&m=115217423929806&w=2
The second is what I think is correct. So please review.
Thanks,
-- Steve
Signed-off-by: Steven Rostedt <ros...@goodmis.org>
Index: linux-2.6.18-rc1/arch/i386/kernel/nmi.c
===================================================================
--- linux-2.6.18-rc1.orig/arch/i386/kernel/nmi.c 2006-07-14 08:35:00.000000000 -0400
+++ linux-2.6.18-rc1/arch/i386/kernel/nmi.c 2006-07-14 08:38:07.000000000 -0400
@@ -106,7 +106,7 @@ int nmi_active;
*/
static __init void nmi_cpu_busy(void *data)
{
- volatile int *endflag = data;
+ int *endflag = data;
local_irq_enable_in_hardirq();
/* Intentionally don't use cpu_relax here. This is
to make sure that the performance counter really ticks,
@@ -121,7 +121,7 @@ static __init void nmi_cpu_busy(void *da
static int __init check_nmi_watchdog(void)
{
- volatile int endflag = 0;
+ int endflag = 0;
unsigned int *prev_nmi_count;
int cpu;
@@ -150,7 +150,7 @@ static int __init check_nmi_watchdog(voi
continue;
#endif
if (nmi_count(cpu) - prev_nmi_count[cpu] <= 5) {
- endflag = 1;
+ set_wmb(endflag, 1);
printk("CPU#%d: NMI appears to be stuck (%d->%d)!\n",
cpu,
prev_nmi_count[cpu],
@@ -161,7 +161,7 @@ static int __init check_nmi_watchdog(voi
return -1;
}
}
- endflag = 1;
+ set_wmb(endflag, 1);
printk("OK.\n");
/* now that we know it works we can reduce NMI frequency to
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Linus Torvalds
On Fri, 14 Jul 2006, Steven Rostedt wrote:
>
> OK, I'm using this as something of an exercise to completely understand
> memory barriers. So if something is incorrect, please let me know.
It's not an incorrect change, but perhaps more importantly, the old code
was buggy in other ways too. Which is sadly more-than-common with anything
that uses volatile - the issues that make people think using "volatile" is
a good idea also tend to cause other problems if the person in question
isn't careful (and using "volatile" obviously means that he/she/it wasn't
very careful when writing it).
In particular, notice how "endflag" is on the _stack_ of the CPU that
wants to send out the NMI to another CPU?
Now, think what that means for the case where we time out and return from
the function with an error.. In particular, think about the case of the
other CPU having been very busy, and now having a stale pointer that
points _where_ exactly?
Also, when the caller sets "endflag", it doesn't (for barrier reasons, see
more below) actually need to use a write barrier in either of the two
cases, because of some _other_ issues. There are two cases of the caller
setting endflag, and neither of them needs "set_wmb()", but it's perhaps
instructive to show _why_.
The first case is the initialization to zero. That one doesn't need a
write barrier, because it has _other_ serialization to any reader. In
order for another CPU to read that value, the other CPU needs to have
_gotten_ the pointer to it in the first place, and that implies that it
got the "smp_call_function()" thing.
And "smp_call_function()" will better have a serialization in it, because
otherwise _any_ user of smp_call_function() would potentially set up data
structures that aren't then readable from other CPUs. So for the
particular case of x86, see the "mb()" in smp_call_function() just before
it does the "send_IPI_allbutself()".
Now, the other case is the case where we set endflag to 1 because we're no
longer interested in the other CPU's. And the reason we don't need a
barrier there is that WE OBVIOUSLY NO LONGER CARE when the other side
sees the value - at that point, it's all moot, because there isn't any
serialization left, and it's just a flag to the other CPU's saying "don't
bother".
So let's go back to the bigger problem..
Now, there is a "reason" we'd want "endflag" to either be volatile, or
have the "set_wmb()", and that is that the code is incorrect in the first
place.
Without the volatile, or the "set_wmb()", the compiler could decide to not
do the last "endflag = 1" write _at_all_, because
- endflag is an automatic variable
- we're going to return from the function RSN, which de-allocates it
and as such, the "volatile" or "set_wmb()" actually forces that write to
happen at all. It so happens that because we have a printk() in there, and
gcc doesn't know that the printk() didn't get the address of the variable
through the "smp_call_function()" thing, gcc won't dare to remove the
write anyway, but let's say that the final 'printk("OK.\n");' wasn't
there, then the compiler could have removed it.
So in that sense, "volatile" and "set_wmb()" superficially "remove a bug",
since optimizing out the write is wrong. However, the REAL bug was totally
elsewhere, and is the fact that "endflag" is an automatic variable in the
first place! The compiler would have been _correct_ to optimize the store
away, because the compiler (unlike the programmer) would have correctly
realized that it cannot matter.
> The first removal is trivial, since the barrier in the while loop makes
> it unnecessary.
Yes, and the first removal is also very much correct.
> The second is what I think is correct.
See above. The second is "correct", in the sense that from a "volatile
removal" standpoint it does all the right things. But it's incorrect,
because it misses the bigger problem with the code.
So I would suggest that the _real_ fix is actually something like the
appended, but I have to say that I didn't really look very closely into
it.
I think that in _practice_ it probably doesn't really matter (in practice,
the other CPU's will either get the NMI or not, and in practice, the stack
location - even after it is released - will probably be overwritten by
something non-zero later anyway), but I think that my fix makes it more
obvious what is really going on, and it's easier to explain why it does
what it does because it no longer depends on insane code.
But somebody like Ingo should probably double-check this.
(The "Have we done this already" test is just covering my ass - I don't
think we should be calling that function more than once, but one of the
things that happens when the "endflag" semantics are fixed is that the
function now has history and the variable is no longer "per CPU". The
point is, that changes how initializations etc may need to be done: in
this case we only want to do it once, but in other cases this kind of
change may have more far-reaching implications).
Linus
---
diff --git a/arch/i386/kernel/nmi.c b/arch/i386/kernel/nmi.c
index 2dd928a..eb8bbbb 100644
--- a/arch/i386/kernel/nmi.c
+++ b/arch/i386/kernel/nmi.c
@@ -106,7 +106,7 @@ #ifdef CONFIG_SMP
*/
static __init void nmi_cpu_busy(void *data)
{
- volatile int *endflag = data;
+ int *endflag = data;
local_irq_enable_in_hardirq();
/* Intentionally don't use cpu_relax here. This is
to make sure that the performance counter really ticks,
@@ -121,10 +121,14 @@ #endif
static int __init check_nmi_watchdog(void)
{
- volatile int endflag = 0;
+ static int endflag = 0;
unsigned int *prev_nmi_count;
int cpu;
+ /* Have we done this already? */
+ if (endflag)
+ return 0;
+
if (nmi_watchdog == NMI_NONE)
return 0;
Chase Venters
> diff --git a/arch/i386/kernel/nmi.c b/arch/i386/kernel/nmi.c
> index 2dd928a..eb8bbbb 100644
> --- a/arch/i386/kernel/nmi.c
> +++ b/arch/i386/kernel/nmi.c
> @@ -106,7 +106,7 @@ #ifdef CONFIG_SMP
> */
> static __init void nmi_cpu_busy(void *data)
> {
> - volatile int *endflag = data;
> + int *endflag = data;
> local_irq_enable_in_hardirq();
> /* Intentionally don't use cpu_relax here. This is
> to make sure that the performance counter really ticks,
> @@ -121,10 +121,14 @@ #endif
>
> static int __init check_nmi_watchdog(void)
> {
> - volatile int endflag = 0;
> + static int endflag = 0;
Now that this is static, isn't this a candidate for __initdata?
> unsigned int *prev_nmi_count;
> int cpu;
>
> + /* Have we done this already? */
> + if (endflag)
> + return 0;
> +
> if (nmi_watchdog == NMI_NONE)
> return 0;
>
Thanks,
Chase
Linus Torvalds
On Fri, 14 Jul 2006, Chase Venters wrote:
> >
> > static int __init check_nmi_watchdog(void)
> > {
> > - volatile int endflag = 0;
> > + static int endflag = 0;
>
> Now that this is static, isn't this a candidate for __initdata?
Yes, that would be good.
Somebody want to test that it actually still _works_, and go through all
the logic?
On a similar vein: Steven, looking at the cmos version of the patch, I
have a hard time knowing whether the added barriers are needed, because I
didn't spend any time on looking at the context of the patch. But I
suspect that generally you do _not_ want to add barriers when you remove
volatiles.
Basically, "volatile" is not a sign that a barrier is needed per se. In
many cases, the _only_ thing that "volatile" implies is that the original
programmer was confused and/or lazy.
So replacing volatiles with accesses with barriers is usually the _wrong_
thing to do. The right thing to do is generally to just _remove_ the
volatile entirely, and then think hard about whether there was some _real_
reason why it existed in the first place.
Note that the only thing a volatile can do is a _compiler_ barrier, so if
you add a real memory barrier or make it use a "set_wmb()" or similar,
you're literally changing code that has been tested to work, and you're
in the process also removing the hint that the code may actually have
fundamental problems.
So I'd argue that it's actually _worse_ to do a "mindless" conversion away
from volatile, than it is to just remove them outright. Removing them
outright may show a bug that the volatile hid (and at that point, people
may see what the _deeper_ problem was), but at least it won't add a memory
barrier that isn't necessary and will potentially just confuse people.
Linus
Steven Rostedt
>
> So I'd argue that it's actually _worse_ to do a "mindless" conversion away
> from volatile, than it is to just remove them outright. Removing them
> outright may show a bug that the volatile hid (and at that point, people
> may see what the _deeper_ problem was), but at least it won't add a memory
> barrier that isn't necessary and will potentially just confuse people.
Perfectly agree, and that is why in my post I said this was a learning
experience for me and to please review. Thinking, at worst you guys
just tell me I'm completely wrong. At best I find a real bug and have a
fix for it. Seems I'm in between the two ;)
I believe I did find a real bug (just luck that it worked) but as you
say, my fix is wrong and if applied would hide the bug. So this was to
bring attention to would be bugs, and in the mean time, I learn exactly
how to use memory barriers and how to get rid of volatiles. Yes, this
was more of a blind change, and I should have looked more into exactly
what the code was doing. But this was more to bring attention to a
problem area than really to solve it.
Thanks for responding and giving me a lesson :)
-- Steve
Linus Torvalds
On Fri, 14 Jul 2006, Linus Torvalds wrote:
>
> Now, there is a "reason" we'd want "endflag" to either be volatile, or
> have the "set_wmb()", and that is that the code is incorrect in the first
> place.
Btw, and this may just be me, but I personally don't much like the
"set_wmb()" macro. I think it should be removed.
I don't think we actually use it anywhere, and the thing is, it's not
really useful. It is basically _always_ equivalent to
var = value;
smp_wmb();
except I think some architectures could _in_theory_ make the assignment be
a "store with release consistency". The only architecture where that might
make sense that I can think of is Itanium, and even there the ia64
set_wmb() macro doesn't actually do that.
Yeah, the
endflag = 1;
smp_wmb();
is a bit longer, but is actually easier to understand, I think.
I suspect "set_wmb()" was added just from an incorrect sense of
consistency with "set_mb()" (which I don't particularly like either, but
at least that one makes a difference on a real platform, ie on x86 that
"set_mb()" ends up being implemented as a single "xchg" instruction).
Linus
Steven Rostedt
>
> endflag = 1;
> smp_wmb();
>
This was what I originally wrote, and then I saw the set_wmb which made
me think that it was the proper way to do things (why else is it
there?). So if it shouldn't be used, then we should get rid of it or at
least mark it deprecated, otherwise you have people like me thinking
that we should use it.
-- Steve
Linus Torvalds
On Fri, 14 Jul 2006, Steven Rostedt wrote:
>
> > endflag = 1;
> > smp_wmb();
>
> This was what I originally wrote, and then I saw the set_wmb which made
> me think that it was the proper way to do things (why else is it
> there?). So if it shouldn't be used, then we should get rid of it or at
> least mark it deprecated, otherwise you have people like me thinking
> that we should use it.
Yeah, we should probably get rid of it. No need to even mark it
deprecated, since nobody uses it anyway.
At a minimum, I think we should not document it in the locking
documentation, making people incorrectly think it might be a good idea.
Hmm? Andrew?
Linus
Andrew Morton
Linus Torvalds <torv...@osdl.org> wrote:
>
>
> On Fri, 14 Jul 2006, Steven Rostedt wrote:
> >
> > > endflag = 1;
> > > smp_wmb();
> >
> > This was what I originally wrote, and then I saw the set_wmb which made
> > me think that it was the proper way to do things (why else is it
> > there?). So if it shouldn't be used, then we should get rid of it or at
> > least mark it deprecated, otherwise you have people like me thinking
> > that we should use it.
>
> Yeah, we should probably get rid of it. No need to even mark it
> deprecated, since nobody uses it anyway.
>
> At a minimum, I think we should not document it in the locking
> documentation, making people incorrectly think it might be a good idea.
>
> Hmm? Andrew?
>
It has no callers and can be trivially reimplemented by any out-of-tree
caller, so we should be able to remove it immediately.
Steven Rostedt
do anything special but shorten the typing of:
var = value;
wmb();
Which the above is much more readable, and thus set_wmb is just
something to confuse developers even more.
So this patch series removes set_wmb from the kernel. It's not
currently used in the kernel, and any out-of-kernel branch can easily
replace it. So there should be no harm in removing it.
The first patch removes it from Documentation/memory-barriers.txt and
the second patch does a sweep through all the architectures to get rid
of it. All archs do the above code except ia64 and sparc which do a
mb() instead. But regardless, it's still not used.
-- Steve
Steven Rostedt
since it shouldn't be used.
Signed-off-by: Steven Rostedt <ros...@goodmis.org>
Index: linux-2.6.18-rc1/Documentation/memory-barriers.txt
===================================================================
--- linux-2.6.18-rc1.orig/Documentation/memory-barriers.txt 2006-07-14 15:33:44.000000000 -0400
+++ linux-2.6.18-rc1/Documentation/memory-barriers.txt 2006-07-14 15:38:05.000000000 -0400
@@ -1015,10 +1015,9 @@ CPU from reordering them.
There are some more advanced barrier functions:
(*) set_mb(var, value)
- (*) set_wmb(var, value)
- These assign the value to the variable and then insert at least a write
- barrier after it, depending on the function. They aren't guaranteed to
+ This assigns the value to the variable and then inserts at least a write
+ barrier after it, depending on the function. It isn't guaranteed to
insert anything more than a compiler barrier in a UP compilation.
Steven Rostedt
code more and has no benefit. Since it is not currently used in the
kernel this patch removes it so that new code does not include it.
All archs define set_wmb(var, value) to do { var = value; wmb(); }
while(0) except ia64 and sparc which use a mb() instead. But this is
still moot since it is not used anyway.
Hasn't been tested on any archs but x86 and x86_64 (and only compiled
tested)
Signed-off-by: Steven Rostedt <ros...@goodmis.org>
Index: linux-2.6.18-rc1/include/asm-alpha/barrier.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-alpha/barrier.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-alpha/barrier.h 2006-07-14 15:39:01.000000000 -0400
@@ -30,7 +30,4 @@ __asm__ __volatile__("mb": : :"memory")
#define set_mb(var, value) \
do { var = value; mb(); } while (0)
-#define set_wmb(var, value) \
-do { var = value; wmb(); } while (0)
-
#endif /* __BARRIER_H */
Index: linux-2.6.18-rc1/include/asm-arm/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-arm/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-arm/system.h 2006-07-14 15:39:05.000000000 -0400
@@ -176,7 +176,6 @@ extern unsigned int user_debug;
#define wmb() mb()
#define read_barrier_depends() do { } while(0)
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#define nop() __asm__ __volatile__("mov\tr0,r0\t@ nop\n\t");
/*
Index: linux-2.6.18-rc1/include/asm-arm26/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-arm26/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-arm26/system.h 2006-07-14 15:39:13.000000000 -0400
@@ -90,7 +90,6 @@ extern unsigned int user_debug;
#define read_barrier_depends() do { } while(0)
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
/*
* We assume knowledge of how
Index: linux-2.6.18-rc1/include/asm-cris/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-cris/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-cris/system.h 2006-07-14 15:39:17.000000000 -0400
@@ -17,7 +17,6 @@ extern struct task_struct *resume(struct
#define wmb() mb()
#define read_barrier_depends() do { } while(0)
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#ifdef CONFIG_SMP
#define smp_mb() mb()
Index: linux-2.6.18-rc1/include/asm-frv/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-frv/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-frv/system.h 2006-07-14 15:39:20.000000000 -0400
@@ -179,7 +179,6 @@ do { \
#define rmb() asm volatile ("membar" : : :"memory")
#define wmb() asm volatile ("membar" : : :"memory")
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#define smp_mb() mb()
#define smp_rmb() rmb()
Index: linux-2.6.18-rc1/include/asm-h8300/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-h8300/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-h8300/system.h 2006-07-14 15:39:23.000000000 -0400
@@ -84,7 +84,6 @@ asmlinkage void resume(void);
#define wmb() asm volatile ("" : : :"memory")
#define set_rmb(var, value) do { xchg(&var, value); } while (0)
#define set_mb(var, value) set_rmb(var, value)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#ifdef CONFIG_SMP
#define smp_mb() mb()
Index: linux-2.6.18-rc1/include/asm-i386/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-i386/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-i386/system.h 2006-07-14 15:39:27.000000000 -0400
@@ -454,8 +454,6 @@ static inline unsigned long long __cmpxc
#define set_mb(var, value) do { var = value; barrier(); } while (0)
#endif
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
-
#include <linux/irqflags.h>
/*
Index: linux-2.6.18-rc1/include/asm-ia64/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-ia64/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-ia64/system.h 2006-07-14 15:39:43.000000000 -0400
@@ -98,12 +98,11 @@ extern struct ia64_boot_param {
#endif
/*
- * XXX check on these---I suspect what Linus really wants here is
+ * XXX check on this ---I suspect what Linus really wants here is
* acquire vs release semantics but we can't discuss this stuff with
* Linus just yet. Grrr...
*/
#define set_mb(var, value) do { (var) = (value); mb(); } while (0)
-#define set_wmb(var, value) do { (var) = (value); mb(); } while (0)
#define safe_halt() ia64_pal_halt_light() /* PAL_HALT_LIGHT */
Index: linux-2.6.18-rc1/include/asm-m32r/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-m32r/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-m32r/system.h 2006-07-14 15:39:47.000000000 -0400
@@ -336,7 +336,6 @@ __cmpxchg(volatile void *ptr, unsigned l
#endif
#define set_mb(var, value) do { xchg(&var, value); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#define arch_align_stack(x) (x)
Index: linux-2.6.18-rc1/include/asm-m68k/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-m68k/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-m68k/system.h 2006-07-14 15:39:52.000000000 -0400
@@ -80,7 +80,6 @@ static inline int irqs_disabled(void)
#define wmb() barrier()
#define read_barrier_depends() do { } while(0)
#define set_mb(var, value) do { xchg(&var, value); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#define smp_mb() barrier()
#define smp_rmb() barrier()
Index: linux-2.6.18-rc1/include/asm-m68knommu/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-m68knommu/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-m68knommu/system.h 2006-07-14 15:39:55.000000000 -0400
@@ -106,7 +106,6 @@ asmlinkage void resume(void);
#define wmb() asm volatile ("" : : :"memory")
#define set_rmb(var, value) do { xchg(&var, value); } while (0)
#define set_mb(var, value) set_rmb(var, value)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#ifdef CONFIG_SMP
#define smp_mb() mb()
Index: linux-2.6.18-rc1/include/asm-mips/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-mips/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-mips/system.h 2006-07-14 15:39:59.000000000 -0400
@@ -143,9 +143,6 @@
#define set_mb(var, value) \
do { var = value; mb(); } while (0)
-#define set_wmb(var, value) \
-do { var = value; wmb(); } while (0)
-
/*
* switch_to(n) should switch tasks to task nr n, first
* checking that n isn't the current task, in which case it does nothing.
Index: linux-2.6.18-rc1/include/asm-parisc/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-parisc/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-parisc/system.h 2006-07-14 15:40:03.000000000 -0400
@@ -143,8 +143,6 @@ static inline void set_eiem(unsigned lon
#define read_barrier_depends() do { } while(0)
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
-
#ifndef CONFIG_PA20
/* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
Index: linux-2.6.18-rc1/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-powerpc/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-powerpc/system.h 2006-07-14 15:40:07.000000000 -0400
@@ -39,7 +39,6 @@
#define read_barrier_depends() do { } while(0)
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#ifdef __KERNEL__
#ifdef CONFIG_SMP
Index: linux-2.6.18-rc1/include/asm-ppc/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-ppc/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-ppc/system.h 2006-07-14 15:40:11.000000000 -0400
@@ -33,7 +33,6 @@
#define read_barrier_depends() do { } while(0)
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#ifdef CONFIG_SMP
#define smp_mb() mb()
Index: linux-2.6.18-rc1/include/asm-s390/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-s390/system.h 2006-07-14 15:38:49.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-s390/system.h 2006-07-14 15:40:15.000000000 -0400
@@ -299,7 +299,6 @@ __cmpxchg(volatile void *ptr, unsigned l
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#ifdef __s390x__
Index: linux-2.6.18-rc1/include/asm-sh/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-sh/system.h 2006-07-14 15:38:50.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-sh/system.h 2006-07-14 15:40:18.000000000 -0400
@@ -101,7 +101,6 @@ extern void __xchg_called_with_bad_point
#endif
#define set_mb(var, value) do { xchg(&var, value); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
/* Interrupt Control */
static __inline__ void local_irq_enable(void)
Index: linux-2.6.18-rc1/include/asm-sh64/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-sh64/system.h 2006-07-14 15:38:50.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-sh64/system.h 2006-07-14 15:40:21.000000000 -0400
@@ -66,7 +66,6 @@ extern void __xchg_called_with_bad_point
#define set_rmb(var, value) do { xchg(&var, value); } while (0)
#define set_mb(var, value) set_rmb(var, value)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
/* Interrupt Control */
#ifndef HARD_CLI
Index: linux-2.6.18-rc1/include/asm-sparc/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-sparc/system.h 2006-07-14 15:38:50.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-sparc/system.h 2006-07-14 15:40:30.000000000 -0400
@@ -199,7 +199,6 @@ static inline unsigned long getipl(void)
#define wmb() mb()
#define read_barrier_depends() do { } while(0)
#define set_mb(__var, __value) do { __var = __value; mb(); } while(0)
-#define set_wmb(__var, __value) set_mb(__var, __value)
#define smp_mb() __asm__ __volatile__("":::"memory")
#define smp_rmb() __asm__ __volatile__("":::"memory")
#define smp_wmb() __asm__ __volatile__("":::"memory")
Index: linux-2.6.18-rc1/include/asm-sparc64/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-sparc64/system.h 2006-07-14 15:38:50.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-sparc64/system.h 2006-07-14 15:40:35.000000000 -0400
@@ -123,8 +123,6 @@ do { __asm__ __volatile__("ba,pt %%xcc,
#define read_barrier_depends() do { } while(0)
#define set_mb(__var, __value) \
do { __var = __value; membar_storeload_storestore(); } while(0)
-#define set_wmb(__var, __value) \
- do { __var = __value; wmb(); } while(0)
#ifdef CONFIG_SMP
#define smp_mb() mb()
Index: linux-2.6.18-rc1/include/asm-v850/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-v850/system.h 2006-07-14 15:38:50.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-v850/system.h 2006-07-14 15:40:39.000000000 -0400
@@ -68,7 +68,6 @@ static inline int irqs_disabled (void)
#define read_barrier_depends() ((void)0)
#define set_rmb(var, value) do { xchg (&var, value); } while (0)
#define set_mb(var, value) set_rmb (var, value)
-#define set_wmb(var, value) do { var = value; wmb (); } while (0)
#define smp_mb() mb ()
#define smp_rmb() rmb ()
Index: linux-2.6.18-rc1/include/asm-x86_64/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-x86_64/system.h 2006-07-14 15:38:50.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-x86_64/system.h 2006-07-14 15:40:42.000000000 -0400
@@ -240,7 +240,6 @@ static inline unsigned long __cmpxchg(vo
#endif
#define read_barrier_depends() do {} while(0)
#define set_mb(var, value) do { (void) xchg(&var, value); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#define warn_if_not_ulong(x) do { unsigned long foo; (void) (&(x) == &foo); } while (0)
Index: linux-2.6.18-rc1/include/asm-xtensa/system.h
===================================================================
--- linux-2.6.18-rc1.orig/include/asm-xtensa/system.h 2006-07-14 15:38:50.000000000 -0400
+++ linux-2.6.18-rc1/include/asm-xtensa/system.h 2006-07-14 15:40:44.000000000 -0400
@@ -99,7 +99,6 @@ static inline void disable_coprocessor(i
#endif
#define set_mb(var, value) do { var = value; mb(); } while (0)
-#define set_wmb(var, value) do { var = value; wmb(); } while (0)
#if !defined (__ASSEMBLY__)
Matthew Wilcox
> There are some more advanced barrier functions:
>
> (*) set_mb(var, value)
> - (*) set_wmb(var, value)
>
> - These assign the value to the variable and then insert at least a write
> - barrier after it, depending on the function. They aren't guaranteed to
> + This assigns the value to the variable and then inserts at least a write
> + barrier after it, depending on the function. It isn't guaranteed to
> insert anything more than a compiler barrier in a UP compilation.
"There is one more advanced barrier function"? ;-) Or did you want to
remove set_mb()?
Plus, the "depending on the function" bit means "respectively". So what
you really want as help is something like:
This assigns the value to the variable and then inserts a
barrier after the assignment. It isn't guaranteed to insert
Steven Rostedt
> On Fri, Jul 14, 2006 at 04:05:01PM -0400, Steven Rostedt wrote:
> > There are some more advanced barrier functions:
> >
> > (*) set_mb(var, value)
> > - (*) set_wmb(var, value)
> >
> > - These assign the value to the variable and then insert at least a write
> > - barrier after it, depending on the function. They aren't guaranteed to
> > + This assigns the value to the variable and then inserts at least a write
> > + barrier after it, depending on the function. It isn't guaranteed to
> > insert anything more than a compiler barrier in a UP compilation.
>
> "There is one more advanced barrier function"? ;-) Or did you want to
> remove set_mb()?
Actually below the patch area we still have:
(*) smp_mb__before_atomic_dec();
(*) smp_mb__after_atomic_dec();
(*) smp_mb__before_atomic_inc();
(*) smp_mb__after_atomic_inc();
So that "There are" references them too :)
>
> Plus, the "depending on the function" bit means "respectively". So what
> you really want as help is something like:
>
> This assigns the value to the variable and then inserts a
> barrier after the assignment. It isn't guaranteed to insert
> anything more than a compiler barrier in a UP compilation.
OK, you're right here, that "depending on the function" needs to go.
Here's a better version:
Thanks,
-- Steve
Signed-off-by: Steven Rostedt <ros...@goodmis.org>
Index: linux-2.6.18-rc1/Documentation/memory-barriers.txt
===================================================================
--- linux-2.6.18-rc1.orig/Documentation/memory-barriers.txt 2006-07-14 15:38:23.000000000 -0400
+++ linux-2.6.18-rc1/Documentation/memory-barriers.txt 2006-07-14 22:31:01.000000000 -0400
@@ -1015,11 +1015,10 @@ CPU from reordering them.
There are some more advanced barrier functions:
(*) set_mb(var, value)
- (*) set_wmb(var, value)
- These assign the value to the variable and then insert at least a write
- barrier after it, depending on the function. They aren't guaranteed to
- insert anything more than a compiler barrier in a UP compilation.
+ This assigns the value to the variable and then inserts a memory barrier
+ after it. It isn't guaranteed to insert anything more than a compiler
+ barrier in a UP compilation.
(*) smp_mb__before_atomic_dec();
Nick Piggin
> OK, I'm using this as something of an exercise to completely understand
> memory barriers. So if something is incorrect, please let me know.
>
> This patch removes the volatile keyword from arch/i386/kernel/nmi.c.
>
> The first removal is trivial, since the barrier in the while loop makes
> it unnecessary. (as proved in "[patch] spinlocks: remove 'volatile'"
> thread)
> http://marc.theaimsgroup.com/?l=linux-kernel&m=115217423929806&w=2
>
>
> The second is what I think is correct. So please review.
Please comment memory barriers if possible.
The second adds memory barriers that weren't there before... how come
they are needed? Basically, the comment should be a pointer to the
read side, or illustrate a typical readside where write reordering
would cause a problem.
--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com