Skip to contentSkip to conversation

remove volatile from nmi.c

PATCH In review Steven Rostedt 6 participants 15 messages Jul 14, 2006 – Jul 15, 2006

Subsystems (3)
Documentation Generic include/asm header files Linux kernel memory consistency model (lkmm)
lore
Share & tools
lore

Steven Rostedt

author
#0 lore raw

PATCH 1/1 remove volatile from nmi.c

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 <[email protected]>
arch/i386/kernel/nmi.c +4 -4
Index: linux-2.6.18-rc1/arch/i386/kernel/nmi.c
===================================================================
@@ -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
#1 lore raw
Quoted 3 lines · 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.

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.

Quoted 2 lines

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.

Quoted 1 line

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

---

arch/i386/kernel/nmi.c +6 -2
index 2dd928a..eb8bbbb 100644
@@ -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;
#2 lore raw
Quoted 14 lines · Linus Torvalds
arch/i386/kernel/nmi.c +1 -1
index 2dd928a..eb8bbbb 100644
@@ -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,

arch/i386/kernel/nmi.c #endif · line 124

Quoted 5 lines
arch/i386/kernel/nmi.c +1 -1
@@ -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?

arch/i386/kernel/nmi.c #endif · line 134

Quoted 10 lines
arch/i386/kernel/nmi.c +4 -0
 	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

#3 lore raw
Quoted 7 lines · Chase Venters

arch/i386/kernel/nmi.c #endif · line 124

Quoted 5 lines

static int __init check_nmi_watchdog(void)
{

arch/i386/kernel/nmi.c +1 -1
-	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

author
#4 lore raw
Quoted 6 lines · Linus Torvalds

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

#5 lore raw
In reply to Linus Torvalds
Quoted 4 lines · Linus Torvalds

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

author
#6 lore raw
Quoted 5 lines · Linus Torvalds

	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

#7 lore raw
Quoted 9 lines · Steven Rostedt
Quoted 3 lines
	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

#8 lore raw
Quoted 13 lines · On Fri, 14 Jul 2006 10:41:58 -0700 (PDT) Linus Torvalds <[email protected]>…
Quoted 9 lines · Steven Rostedt
Quoted 3 lines
	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.

Quoted 8 lines

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.

author
#9 lore raw

PATCH 1/2 remove set_wmb - doc update

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

Signed-off-by Steven Rostedt <[email protected]>
Documentation/memory-barriers.txt +2 -3
Index: linux-2.6.18-rc1/Documentation/memory-barriers.txt
===================================================================
@@ -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.
author
#12 lore raw

cover remove set_wmb

In reply to Andrew Morton

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

author
#13 lore raw

PATCH 2/2 remove set_wmb - arch removal

In reply to Andrew Morton

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 <[email protected]>
include/asm-alpha/barrier.h +0 -3
Index: linux-2.6.18-rc1/include/asm-alpha/barrier.h
===================================================================
@@ -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 */
include/asm-arm/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-arm/system.h
===================================================================
@@ -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");
 
 /*
include/asm-arm26/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-arm26/system.h
===================================================================
@@ -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
include/asm-cris/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-cris/system.h
===================================================================
@@ -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()
include/asm-frv/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-frv/system.h
===================================================================
@@ -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()
include/asm-h8300/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-h8300/system.h
===================================================================
@@ -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()
include/asm-i386/system.h +0 -2
Index: linux-2.6.18-rc1/include/asm-i386/system.h
===================================================================
@@ -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>
 
 /*
include/asm-ia64/system.h +1 -2
Index: linux-2.6.18-rc1/include/asm-ia64/system.h
===================================================================
@@ -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 */
include/asm-m32r/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-m32r/system.h
===================================================================
@@ -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)
include/asm-m68k/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-m68k/system.h
===================================================================
@@ -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()
include/asm-m68knommu/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-m68knommu/system.h
===================================================================
@@ -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()
include/asm-mips/system.h +0 -3
Index: linux-2.6.18-rc1/include/asm-mips/system.h
===================================================================
@@ -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.
include/asm-parisc/system.h +0 -2
Index: linux-2.6.18-rc1/include/asm-parisc/system.h
===================================================================
@@ -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,
include/asm-powerpc/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-powerpc/system.h
===================================================================
@@ -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
include/asm-ppc/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-ppc/system.h
===================================================================
@@ -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()
include/asm-s390/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-s390/system.h
===================================================================
@@ -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__
include/asm-sh/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-sh/system.h
===================================================================
@@ -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)
include/asm-sh64/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-sh64/system.h
===================================================================
@@ -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
include/asm-sparc/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-sparc/system.h
===================================================================
@@ -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")
include/asm-sparc64/system.h +0 -2
Index: linux-2.6.18-rc1/include/asm-sparc64/system.h
===================================================================
@@ -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()
include/asm-v850/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-v850/system.h
===================================================================
@@ -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 ()
include/asm-x86_64/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-x86_64/system.h
===================================================================
@@ -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)
include/asm-xtensa/system.h +0 -1
Index: linux-2.6.18-rc1/include/asm-xtensa/system.h
===================================================================
@@ -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__)
#10 lore raw

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

In reply to Steven Rostedt

Documentation/memory-barriers.txt CPU from reordering them. · line 1021

Quoted 10 lines · Steven Rostedt

There are some more advanced barrier functions:

(*) set_mb(var, value)
- (*) set_wmb(var, value)

Documentation/memory-barriers.txt +2 -2
-     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.

author
#11 lore raw

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

Quoted 6 lines · Matthew Wilcox
Quoted 5 lines · Steven Rostedt

There are some more advanced barrier functions:

(*) set_mb(var, value)
- (*) set_wmb(var, value)

Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +2 -2
-     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 :)

Quoted 7 lines

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 <[email protected]>
Documentation/memory-barriers.txt +3 -4
Index: linux-2.6.18-rc1/Documentation/memory-barriers.txt
===================================================================
@@ -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();
#14 lore raw
In reply to Steven Rostedt
Quoted 5 lines · 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.

Quoted 7 lines

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.

Quoted 47 lines

Thanks,

-- Steve

Signed-off-by Steven Rostedt <[email protected]>

Index: linux-2.6.18-rc1/arch/i386/kernel/nmi.c
===================================================================

arch/i386/kernel/nmi.c +4 -4
@@ -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
Signature · 2 lines
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com