[PATCH] "volatile considered harmful", take 2

[PATCH] "volatile considered harmful", take 2

[Jonathan Corbet]

Who knew a documentation patch would get so many reviews?  I like it...

Anyway, here's a new version in which I attempt to respond to all the
comments that came in.  Thanks to everybody for looking it over.

jon

Steer developers away from the volatile type.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>

diff --git a/Documentation/volatile-considered-harmful.txt b/Documentation/volatile-considered-harmful.txt
new file mode 100644
index 0000000..e67745f
--- /dev/null
+++ b/Documentation/volatile-considered-harmful.txt
@@ -0,0 +1,118 @@
+Why the "volatile" type class should not be used
+------------------------------------------------
+
+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
+sometimes tempted to use it in kernel code when shared data structures are
+being used.  In other words, they have been known to treat volatile types
+as a sort of easy atomic variable, which they are not.  The use of volatile in
+kernel code is almost never correct; this document describes why.
+
+The key point to understand 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 shared data structures against
+unwanted concurrent access, which is very much a different task.  As it
+happens, once the critical sections are properly implemented, the compiler
+optimization issues which volatile was added to prevent will have been
+taken care of in a more efficient way.
+
+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, since it acts as a memory barrier, 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_data _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.  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 strongly frowned upon.  Jiffies is considered
+    to be a "stupid legacy" issue in this regard.
+
+  - Pointers to data structures in coherent memory which might be modified
+    by I/O devices can, sometimes, legitimately be volatile.  A ring buffer
+    used by a network adapter, where that adapter changes pointers to
+    indicate which descriptors have been processed, is an example of this
+    type of situation.
+
+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.  
+
+Patches to remove volatile variables are generally welcome - as long as
+they come with a justification which shows that the concurrency issues have
+been properly thought through.
+
+
+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, Heikki Orsila, and H. Peter Anvin

Re: [PATCH] "volatile considered harmful", take 2

[jimmy bahuleyan]

Jonathan Corbet wrote:
[snip..]
> +
> +  - 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 strongly frowned upon.  Jiffies is considered
> +    to be a "stupid legacy" issue in this regard.

Why is it that you consider jiffies to be a "stupid legacy"? Isn't it
natural to have a externally modified variable which is only /read/ to
be volatile? (or is jiffies supposed to be replaced with something
smarter/better :)


-jb
-- 
Tact is the art of making a point without making an enemy.

Re: [PATCH] "volatile considered harmful", take 2

[Jan Engelhardt]

On May 10 2007 14:20, Jonathan Corbet wrote:
>
>Who knew a documentation patch would get so many reviews?  I like it...

And the next thing is register-considered-harmful.txt. Running

	grep -Pr '\bregister\s+(unsigned|char|short|int|long|float|
	double|struct|union|uint|u\d+|s\d+)\b' linux-2.6.21/ | wc -l

turns up no less than 1106+2 hits.


	Jan
-- 

Re: [PATCH] "volatile considered harmful", take 2

[Satyam Sharma]

On 5/11/07, jimmy bahuleyan <knight.camelot@gmail.com> wrote:
> Jonathan Corbet wrote:
> [snip..]
> > +
> > +  - 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 strongly frowned upon.  Jiffies is considered
> > +    to be a "stupid legacy" issue in this regard.
>
> Why is it that you consider jiffies to be a "stupid legacy"?

You could find better explanations in previous threads, but to
summarize my understanding of the matter:

Because it is not humanly possible to audit the entire kernel tree to
find all usages of jiffies and fix them appropriately (i.e. "stupid
legacy reasons"). Hence, we just define jiffies as volatile.

> Isn't it natural to have a externally modified variable which is only /read/ to
> be volatile?

No, even in such a case, it would have been saner to simply define the
jiffies _without_ volatile, and only cast _accesses_ to it with the
volatile type qualifier.

Unfortunately, even this is not possible to do in all existing users
in the kernel (i.e. "stupid legacy reasons").

> (or is jiffies supposed to be replaced with something smarter/better :)

Not _replace_ jiffies. But if it were _really_ possible to do so (i.e.
assuming for a moment that we could change all kernel code in one
magic sweep), then again we wouldn't need to use volatile for jiffies
(or even use a volatile cast, IMO).

Just ensure all accesses to jiffies are protected behind an
appropriate barrier() instead, the effects of which are more clearly
defined than the vague and insufficient "volatile", and which
*guarantees* that all memory would be re-read from that point (note
that "volatile" is merely a hint to a C compiler, it comes with no
guarantees at all, which is particularly worrisome these days when
compilers are not the only entities that can re-order code -- hardware
can do so too).

However, yet again, it is _not_ possible to fix the above for all the
existing kernel code that uses jiffies (i.e. "stupid legacy reasons"),
so we just define jiffies as volatile.

My understanding, at least.

Re: [PATCH] "volatile considered harmful", take 2

[H. Peter Anvin]

Jan Engelhardt wrote:
> On May 10 2007 14:20, Jonathan Corbet wrote:
>> Who knew a documentation patch would get so many reviews?  I like it...
> 
> And the next thing is register-considered-harmful.txt. Running
> 
> 	grep -Pr '\bregister\s+(unsigned|char|short|int|long|float|
> 	double|struct|union|uint|u\d+|s\d+)\b' linux-2.6.21/ | wc -l
> 
> turns up no less than 1106+2 hits.

You forgot to exclude instances with "asm" in them.

	-hpa

Re: [PATCH] "volatile considered harmful", take 2

[Philipp Matthias Hahn]

Hello!

On Thu, May 10, 2007 at 02:20:19PM -0600, Jonathan Corbet wrote:
...
> +++ b/Documentation/volatile-considered-harmful.txt
...
> +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
s/some_data/shared_data/ ?                       ^^^^^^^^^

BYtE
Philipp
-- 
  / /  (_)__  __ ____  __ Philipp Hahn
 / /__/ / _ \/ // /\ \/ /
/____/_/_//_/\_,_/ /_/\_\ pmhahn@titan.lahn.de

Re: [PATCH] "volatile considered harmful", take 2

[Bernd Eckenfels]

In article <6143.1178828419@lwn.net> you wrote:
> +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.

Well maybe it is trivial, but I would add e.g. "all places where the
shared_data is accessed must be protected by this spinlock"

> +  - The jiffies variable is special in that it can have a different value

what about other atomic readable counters (like interface counters)?

Gruss
Bernd

Re: [PATCH] "volatile considered harmful", take 2

[Stefan Richter]

Jonathan Corbet wrote:
> +The key point to understand 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 shared data structures against
> +unwanted concurrent access, which is very much a different task.  As it
> +happens, once the critical sections are properly implemented, the compiler
> +optimization issues which volatile was added to prevent will have been
> +taken care of in a more efficient way.

The "As it happens... efficient way." sentence is hard to read:  It is
long and nested like in German, combined with several English
ambiguities (e.g. WRT noun vs. verb vs. adjective).  Maybe just leave
this sentence out, as it is redundant to later statements like "In
properly-written kernel code, volatile can only serve to slow things down".
-- 
Stefan Richter
-=====-=-=== -=-= -=-==
http://arcgraph.de/sr/

Re: [PATCH] "volatile considered harmful", take 2

[Johannes Stezenbach]

On Fri, May 11, 2007 at 02:08:54AM +0530, jimmy bahuleyan wrote:
> Jonathan Corbet wrote:
> [snip..]
> > +
> > +  - 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 strongly frowned upon.  Jiffies is considered
> > +    to be a "stupid legacy" issue in this regard.
> 
> Why is it that you consider jiffies to be a "stupid legacy"? Isn't it
> natural to have a externally modified variable which is only /read/ to
> be volatile? (or is jiffies supposed to be replaced with something
> smarter/better :)

"stupid legacy" were Linus' words. http://lwn.net/Articles/233482/

How about this:

"The jiffies variable is a special case because there are too
many places in the kernel which would have to be changed and reviewed
if the volatile would be removed from jiffies. However, the
use of volatile qualifier for jiffies is just as wrong as
it is elsewhere. Don't use jiffies as an excuse to use volatile
in your code."


Johannes

Re: [PATCH] "volatile considered harmful", take 2

[jimmy bahuleyan]

Johannes Stezenbach wrote:
> On Fri, May 11, 2007 at 02:08:54AM +0530, jimmy bahuleyan wrote:
>> Jonathan Corbet wrote:
>> [snip..]
>>> +
>>> +  - 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 strongly frowned upon.  Jiffies is considered
>>> +    to be a "stupid legacy" issue in this regard.
>> Why is it that you consider jiffies to be a "stupid legacy"? Isn't it
>> natural to have a externally modified variable which is only /read/ to
>> be volatile? (or is jiffies supposed to be replaced with something
>> smarter/better :)
> 
> "stupid legacy" were Linus' words. http://lwn.net/Articles/233482/
> 
> How about this:
> 
> "The jiffies variable is a special case because there are too
> many places in the kernel which would have to be changed and reviewed
> if the volatile would be removed from jiffies. However, the
> use of volatile qualifier for jiffies is just as wrong as
> it is elsewhere. Don't use jiffies as an excuse to use volatile
> in your code."
> 
> 
> Johannes
> 

yes this sounds better. at least to a non-kernel expert like me it makes
the meaning clear - 'that jiffies is a special case, not to be taken as
an example for other stuff'.

-jb

-- 
Tact is the art of making a point without making an enemy.

Re: [PATCH] "volatile considered harmful", take 2

[Satyam Sharma]

On 5/11/07, Johannes Stezenbach <js@linuxtv.org> wrote:
> On Fri, May 11, 2007 at 02:08:54AM +0530, jimmy bahuleyan wrote:
> > Jonathan Corbet wrote:
> > [snip..]
> > > +
> > > +  - 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 strongly frowned upon.  Jiffies is considered
> > > +    to be a "stupid legacy" issue in this regard.
> >
> > Why is it that you consider jiffies to be a "stupid legacy"? Isn't it
> > natural to have a externally modified variable which is only /read/ to
> > be volatile? (or is jiffies supposed to be replaced with something
> > smarter/better :)
>
> "stupid legacy" were Linus' words. http://lwn.net/Articles/233482/
>
> How about this:
>
> "The jiffies variable is a special case because there are too
> many places in the kernel which would have to be changed and reviewed
> if the volatile would be removed from jiffies. However, the
> use of volatile qualifier for jiffies is just as wrong as
> it is elsewhere. Don't use jiffies as an excuse to use volatile
> in your code."

Yes, or some "good" combination of the original and this one :-)

The problem with natural languages is that one is never quite satisfied
(or convinced) that one has expressed all that one wished to express,
and as nicely as one wanted to. That's often not the case with programming
languages, by contrast.

Re: [PATCH] "volatile considered harmful", take 2

[Jan Engelhardt]

On May 10 2007 14:45, H. Peter Anvin wrote:
>Jan Engelhardt wrote:
>> On May 10 2007 14:20, Jonathan Corbet wrote:
>>> Who knew a documentation patch would get so many reviews?  I like it...
>> 
>> And the next thing is register-considered-harmful.txt. Running
>> 
>> 	grep -Pr '\bregister\s+(unsigned|char|short|int|long|float|
>> 	double|struct|union|uint|u\d+|s\d+)\b' linux-2.6.21/ | wc -l
>> 
>> turns up no less than 1106+2 hits.
>
>You forgot to exclude instances with "asm" in them.

You can do that.


	Jan
-- 

Re: [PATCH] "volatile considered harmful", take 2

[H. Peter Anvin]

Jan Engelhardt wrote:
>>>
>>> turns up no less than 1106+2 hits.
>> You forgot to exclude instances with "asm" in them.
> 
> You can do that.
> 

I can.  54.2% of those hits were "asm", and therefore require the
"register" keyword.

	-hpa

Re: [PATCH] "volatile considered harmful", take 2

[H. Peter Anvin]

pradeep singh wrote:
> 
> Sorry, for my misunderstanding but i hope Jonathan actually means
> volatile harmful only in C and not while using extended asm with gcc? Or
> does you all consider volatile while using extended asm as harmful too?
> Incidentally i came to know that using volatile in such cases may be
> still be optimized by the gcc. And the correct way is to fake a side
> effect to the gcc, which can be done using "memory" clobbering directive
> in the correct place and not "m" or "+m".
> 
> Does this means to exclude volatile from extended asm also, while using
> them in kernel?
> 

We were talking about "register", not "volatile".

	-hpa