[PATCH] remove volatile from nmi.c

[PATCH] remove volatile from nmi.c

[Steven Rostedt]

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.

Thanks,

-- Steve

Signed-off-by: Steven Rostedt <rostedt@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

Re: [PATCH] remove volatile from nmi.c

[Nick Piggin]

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.
> 
> 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.

> 
> Thanks,
> 
> -- Steve
> 
> Signed-off-by: Steven Rostedt <rostedt@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


-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] remove volatile from nmi.c

[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;
 

Re: [PATCH] remove volatile from nmi.c

[Chase Venters]

On Fri, 14 Jul 2006, Linus Torvalds wrote:

> 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

Re: [PATCH] remove volatile from nmi.c

[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

Re: [PATCH] remove volatile from nmi.c

[Steven Rostedt]

On Fri, 2006-07-14 at 09:47 -0700, Linus Torvalds wrote:

> 
> 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

Re: [PATCH] remove volatile from nmi.c

[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

Re: [PATCH] remove volatile from nmi.c

[Steven Rostedt]

On Fri, 2006-07-14 at 10:28 -0700, Linus Torvalds 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.

-- Steve

Re: [PATCH] remove volatile from nmi.c

[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

Re: [PATCH] remove volatile from nmi.c

[Andrew Morton]

On Fri, 14 Jul 2006 10:41:58 -0700 (PDT)
Linus Torvalds <torvalds@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.

[PATCH 00/02] remove set_wmb

[Steven Rostedt]

set_wmb(var, value) is not used anywhere in the kernel. And it doesn't
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

[PATCH 01/02] remove set_wmb - doc update

[Steven Rostedt]

This patch removes the reference to set_wmb from memory-barriers.txt
since it shouldn't be used.

Signed-off-by: Steven Rostedt <rostedt@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.
 
 

[PATCH 02/02] remove set_wmb - arch removal

[Steven Rostedt]

set_wmb should not be used in the kernel because it just confuses the
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 <rostedt@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__)
 

Re: [PATCH 01/02] remove set_wmb - doc update

[Matthew Wilcox]

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()?

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.

Re: [PATCH 01/02] remove set_wmb - doc update

[Steven Rostedt]

On Fri, 2006-07-14 at 20:22 -0600, Matthew Wilcox wrote:
> 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 <rostedt@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();