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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/