Messages in this thread | | | Date | Wed, 9 May 2007 23:45:39 +0200 | From | "Jesper Juhl" <> | Subject | Re: [PATCH] "volatile considered harmful" document |
| |
On 09/05/07, Jonathan Corbet <corbet@lwn.net> wrote: > OK, here's an updated version of the volatile document - as a plain text > file this time. It drops a new file in Documentation/, but might it be > better as an addition to CodingStyle? > IMHO this is better as a sepperate document rather than an adition to CodingStyle. The use of volatile is not a style issue, it's a correctness issue, so it doesn't belong in the CodingStyle document.
> Comments welcome, > Find a few below :)
> jon > > Tell kernel developers why they shouldn't use volatile. > > Signed-off-by: Jonathan Corbet <corbet@lwn.net> > > diff -ruNp linux-2.6/Documentation/volatile.txt volatile/Documentation/volatile.txt
Might I suggest a different filename: volatile-considered-harmful.txt
> --- linux-2.6/Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 > +++ volatile/Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 > @@ -0,0 +1,127 @@ > +Why the "volatile" type class should not be used > +------------------------------------------------ > + > +The C Programming Language, Second Edition (copyright 1988, still known as > +"the new C book") has the following to say about the volatile keyword: > + > + The purpose of volatile is to force an implementation to suppress > + optimization that could otherwise occur. For example, for a > + machine with memory-mapped input/output, a pointer to a device > + register might be declared as a pointer to volatile, in > + order to prevent the compiler from removing apparently redundant > + references through the pointer. > + > +C programmers have often taken volatile to mean that the variable could be > +changed outside of the current thread of execution; as a result, they are
you write: "... that the variable could be changed outside of the current thread of execution ..."
I suggest: "... that the variable could be changed outside of the current thread of execution - a sort of simple atomic variable ..."
> +sometimes tempted to use it in kernel code when shared data structures are > +being used. Andrew Morton recently called out[1] use of volatile in a > +submitted patch, saying: > + > + The volatiles are a worry - volatile is said to be > + basically-always-wrong in-kernel, although we've never managed to > + document why, and i386 cheerfully uses it in readb() and friends. > + > +In response, Randy Dunlap pulled together some email from Linus[2] on the > +topic and suggested that we could maybe "document why." Here is the > +result. > + > +The point that Linus often makes with regard to volatile is that > +its purpose is to suppress optimization, which is almost never what one > +really wants to do. In the kernel, one must protect accesses to data > +against race conditions, which is very much a different task. > + > +Like volatile, the kernel primitives which make concurrent access to data > +safe (spinlocks, mutexes, memory barriers, etc.) are designed to prevent > +unwanted optimization. If they are being used properly, there will be no > +need to use volatile as well. If volatile is still necessary, there is > +almost certainly a bug in the code somewhere. In properly-written kernel > +code, volatile can only serve to slow things down. > + > +Consider a typical block of kernel code: > + > + spin_lock(&the_lock); > + do_something_on(&shared_data); > + do_something_else_with(&shared_data); > + spin_unlock(&the_lock); > + > +If all the code follows the locking rules, the value of shared_data cannot > +change unexpectedly while the_lock is held. Any other code which might > +want to play with that data will be waiting on the lock. The spinlock > +primitives act as memory barriers - they are explicitly written to do so - > +meaning that data accesses will not be optimized across them. So the > +compiler might think it knows what will be in some_data, but the > +spin_lock() call will force it to forget anything it knows. There will be > +no optimization problems with accesses to that data. > + > +If shared_data were declared volatile, the locking would > +still be necessary. But the compiler would also be prevented from > +optimizing access to shared _within_ the critical section, > +when we know that nobody else can be working with it. While the lock is > +held, shared_data is not volatile. This is why Linus says: > + > + Also, more importantly, "volatile" is on the wrong _part_ of the > + whole system. In C, it's "data" that is volatile, but that is > + insane. Data isn't volatile - _accesses_ are volatile. So it may > + make sense to say "make this particular _access_ be careful", but > + not "make all accesses to this data use some random strategy". > + > +When dealing with shared data, proper locking makes volatile unnecessary - > +and potentially harmful. > + > +The volatile storage class was originally meant for memory-mapped I/O > +registers. Within the kernel, register accesses, too, should be protected > +by locks, but one also does not want the compiler "optimizing" register > +accesses within a critical section. But, within the kernel, I/O memory > +accesses are always done through accessor functions; accessing I/O memory > +directly through pointers is frowned upon and does not work on all > +architectures. Those accessors are written to prevent unwanted > +optimization, so, once again, volatile is unnecessary. > + > +Another situation where one might be tempted to use volatile is > +when the processor is busy-waiting on the value of a variable. The right > +way to perform a busy wait is: > + > + while (my_variable != what_i_want) > + cpu_relax(); > + > +The cpu_relax() call can lower CPU power consumption or yield to a > +hyperthreaded twin processor; it also happens to serve as a memory barrier, > +so, once again, volatile is unnecessary. Of course, busy-waiting is > +generally an anti-social act to begin with. > + > +There are still a few rare situations where volatile makes sense in the > +kernel: > + > + - The above-mentioned accessor functions might use volatile on > + architectures where direct I/O memory access does work. Essentially, > + each accessor call becomes a little critical section on its own and > + ensures that the access happens as expected by the programmer. > + > + - Inline assembly code which changes memory, but which has no other > + visible side effects, risks being deleted by GCC. Adding the volatile > + keyword to asm statements will prevent this removal. > + > + - The jiffies variable is special in that it can have a different value > + every time it is referenced, but it can be read without any special > + locking. So jiffies can be volatile, but the addition of other > + variables of this type is frowned upon. Jiffies is considered to be a
suggestion: "frowned strongly upon"
> + "stupid legacy" issue in this regard. > + > +For most code, none of the above justifications for volatile > +apply. As a result, the use of volatile is likely to be seen as a > +bug and will bring additional scrutiny to the code. Developers who are > +tempted to use volatile should take a step back and think about > +what they are truly trying to accomplish. > +
Suggested addition :
Patches that remove volatile from current code (provided there's a good explanation of why the volatile can be removed and how the bug it was hiding has been dealt with) are a good thing.
Friends don't let friends use volatile.
> +NOTES > +----- > + > +[1] http://lwn.net/Articles/233481/ > +[2] http://lwn.net/Articles/233482/ > + > +CREDITS > +------- > + > +Original impetus and research by Randy Dunlap > +Written by Jonathan Corbet > +Improvements via coments from Satyam Sharma Johannes Stezenbach
-- Jesper Juhl <jesper.juhl@gmail.com> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |