* [PATCH] remove volatile from nmi.c @ 2006-07-14 13:04 Steven Rostedt 2006-07-14 13:28 ` Nick Piggin 2006-07-14 15:23 ` Linus Torvalds 0 siblings, 2 replies; 15+ messages in thread From: Steven Rostedt @ 2006-07-14 13:04 UTC (permalink / raw) To: Andrew Morton; +Cc: Ingo Molnar, Linus Torvalds, LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] remove volatile from nmi.c 2006-07-14 13:04 [PATCH] remove volatile from nmi.c Steven Rostedt @ 2006-07-14 13:28 ` Nick Piggin 2006-07-14 15:23 ` Linus Torvalds 1 sibling, 0 replies; 15+ messages in thread From: Nick Piggin @ 2006-07-14 13:28 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andrew Morton, Ingo Molnar, Linus Torvalds, LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] remove volatile from nmi.c 2006-07-14 13:04 [PATCH] remove volatile from nmi.c Steven Rostedt 2006-07-14 13:28 ` Nick Piggin @ 2006-07-14 15:23 ` Linus Torvalds 2006-07-14 16:32 ` Chase Venters 2006-07-14 17:28 ` Linus Torvalds 1 sibling, 2 replies; 15+ messages in thread From: Linus Torvalds @ 2006-07-14 15:23 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andrew Morton, Ingo Molnar, LKML 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; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] remove volatile from nmi.c 2006-07-14 15:23 ` Linus Torvalds @ 2006-07-14 16:32 ` Chase Venters 2006-07-14 16:47 ` Linus Torvalds 2006-07-14 17:28 ` Linus Torvalds 1 sibling, 1 reply; 15+ messages in thread From: Chase Venters @ 2006-07-14 16:32 UTC (permalink / raw) To: Linus Torvalds; +Cc: Steven Rostedt, Andrew Morton, Ingo Molnar, LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] remove volatile from nmi.c 2006-07-14 16:32 ` Chase Venters @ 2006-07-14 16:47 ` Linus Torvalds 2006-07-14 17:00 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Linus Torvalds @ 2006-07-14 16:47 UTC (permalink / raw) To: Chase Venters; +Cc: Steven Rostedt, Andrew Morton, Ingo Molnar, LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] remove volatile from nmi.c 2006-07-14 16:47 ` Linus Torvalds @ 2006-07-14 17:00 ` Steven Rostedt 0 siblings, 0 replies; 15+ messages in thread From: Steven Rostedt @ 2006-07-14 17:00 UTC (permalink / raw) To: Linus Torvalds; +Cc: Chase Venters, Andrew Morton, Ingo Molnar, LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] remove volatile from nmi.c 2006-07-14 15:23 ` Linus Torvalds 2006-07-14 16:32 ` Chase Venters @ 2006-07-14 17:28 ` Linus Torvalds 2006-07-14 17:38 ` Steven Rostedt 1 sibling, 1 reply; 15+ messages in thread From: Linus Torvalds @ 2006-07-14 17:28 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andrew Morton, Ingo Molnar, LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] remove volatile from nmi.c 2006-07-14 17:28 ` Linus Torvalds @ 2006-07-14 17:38 ` Steven Rostedt 2006-07-14 17:41 ` Linus Torvalds 0 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2006-07-14 17:38 UTC (permalink / raw) To: Linus Torvalds; +Cc: Andrew Morton, Ingo Molnar, LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] remove volatile from nmi.c 2006-07-14 17:38 ` Steven Rostedt @ 2006-07-14 17:41 ` Linus Torvalds 2006-07-14 17:58 ` Andrew Morton 0 siblings, 1 reply; 15+ messages in thread From: Linus Torvalds @ 2006-07-14 17:41 UTC (permalink / raw) To: Steven Rostedt; +Cc: Andrew Morton, Ingo Molnar, LKML 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] remove volatile from nmi.c 2006-07-14 17:41 ` Linus Torvalds @ 2006-07-14 17:58 ` Andrew Morton 2006-07-14 20:04 ` [PATCH 00/02] remove set_wmb Steven Rostedt ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Andrew Morton @ 2006-07-14 17:58 UTC (permalink / raw) To: Linus Torvalds; +Cc: rostedt, mingo, linux-kernel 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 00/02] remove set_wmb 2006-07-14 17:58 ` Andrew Morton @ 2006-07-14 20:04 ` Steven Rostedt 2006-07-14 20:05 ` [PATCH 01/02] remove set_wmb - doc update Steven Rostedt 2006-07-14 20:05 ` [PATCH 02/02] remove set_wmb - arch removal Steven Rostedt 2 siblings, 0 replies; 15+ messages in thread From: Steven Rostedt @ 2006-07-14 20:04 UTC (permalink / raw) To: Andrew Morton Cc: Linus Torvalds, mingo, linux-kernel, linux-arch, David Howells 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 01/02] remove set_wmb - doc update 2006-07-14 17:58 ` Andrew Morton 2006-07-14 20:04 ` [PATCH 00/02] remove set_wmb Steven Rostedt @ 2006-07-14 20:05 ` Steven Rostedt 2006-07-15 2:22 ` Matthew Wilcox 2006-07-14 20:05 ` [PATCH 02/02] remove set_wmb - arch removal Steven Rostedt 2 siblings, 1 reply; 15+ messages in thread From: Steven Rostedt @ 2006-07-14 20:05 UTC (permalink / raw) To: Andrew Morton; +Cc: Linus Torvalds, mingo, linux-arch, David Howells, LKML 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 01/02] remove set_wmb - doc update 2006-07-14 20:05 ` [PATCH 01/02] remove set_wmb - doc update Steven Rostedt @ 2006-07-15 2:22 ` Matthew Wilcox 2006-07-15 2:35 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Matthew Wilcox @ 2006-07-15 2:22 UTC (permalink / raw) To: Steven Rostedt Cc: Andrew Morton, Linus Torvalds, mingo, linux-arch, David Howells, LKML 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 01/02] remove set_wmb - doc update 2006-07-15 2:22 ` Matthew Wilcox @ 2006-07-15 2:35 ` Steven Rostedt 0 siblings, 0 replies; 15+ messages in thread From: Steven Rostedt @ 2006-07-15 2:35 UTC (permalink / raw) To: Matthew Wilcox Cc: Andrew Morton, Linus Torvalds, mingo, linux-arch, David Howells, LKML 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(); ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 02/02] remove set_wmb - arch removal 2006-07-14 17:58 ` Andrew Morton 2006-07-14 20:04 ` [PATCH 00/02] remove set_wmb Steven Rostedt 2006-07-14 20:05 ` [PATCH 01/02] remove set_wmb - doc update Steven Rostedt @ 2006-07-14 20:05 ` Steven Rostedt 2 siblings, 0 replies; 15+ messages in thread From: Steven Rostedt @ 2006-07-14 20:05 UTC (permalink / raw) To: Andrew Morton Cc: Linus Torvalds, mingo, linux-kernel, linux-arch, David Howells 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__) ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2006-07-15 19:17 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2006-07-14 13:04 [PATCH] remove volatile from nmi.c Steven Rostedt 2006-07-14 13:28 ` Nick Piggin 2006-07-14 15:23 ` Linus Torvalds 2006-07-14 16:32 ` Chase Venters 2006-07-14 16:47 ` Linus Torvalds 2006-07-14 17:00 ` Steven Rostedt 2006-07-14 17:28 ` Linus Torvalds 2006-07-14 17:38 ` Steven Rostedt 2006-07-14 17:41 ` Linus Torvalds 2006-07-14 17:58 ` Andrew Morton 2006-07-14 20:04 ` [PATCH 00/02] remove set_wmb Steven Rostedt 2006-07-14 20:05 ` [PATCH 01/02] remove set_wmb - doc update Steven Rostedt 2006-07-15 2:22 ` Matthew Wilcox 2006-07-15 2:35 ` Steven Rostedt 2006-07-14 20:05 ` [PATCH 02/02] remove set_wmb - arch removal Steven Rostedt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.