Skip to contentSkip to conversation

"volatile considered harmful" document

PATCH In review Jonathan Corbet 11 participants 13 messages May 9, 2007 – May 16, 2007 Documentation

lore
Share & tools
lore

Jonathan Corbet

author maintainer
#0 lore raw

PATCH 1/1 "volatile considered harmful" document

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?

Comments welcome,

jon

Tell kernel developers why they shouldn't use volatile.

Documentation/volatile.txt +127 -0
diff -ruNp linux-2.6/Documentation/volatile.txt volatile/Documentation/volatile.txt
@@ -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
+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
+    "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.
+
+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 
#1 lore raw
Quoted 4 lines · Jonathan Corbet

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.

Quoted 2 lines

Comments welcome,

Find a few below :)

Quoted 7 lines

jon

Tell kernel developers why they shouldn't use volatile.

diff -ruNp linux-2.6/Documentation/volatile.txt volatile/Documentation/volatile.txt

Might I suggest a different filename: volatile-considered-harmful.txt

Quoted 8 lines
Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 renamed from Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 +5 -0
@@ -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:

Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 line 15

Quoted 10 lines
Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 renamed from Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 +10 -0
+
+       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 ..."

Quoted 82 lines
Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 renamed from Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 +82 -0
+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

Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 line 107

Quoted 10 lines
Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 renamed from Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 +10 -0
+    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"

Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 line 115

Quoted 8 lines
Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 renamed from Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 +8 -0
+    "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.

Quoted 12 lines
Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 renamed from Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 +12 -0
+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
Signature · 3 lines
Jesper Juhl <[email protected]>
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
#2 lore raw
Quoted 8 lines · Jesper Juhl
Quoted 4 lines · Jonathan Corbet

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.

Yes, definitely.

Quoted 3 lines
Quoted 1 line

diff -ruNp linux-2.6/Documentation/volatile.txt volatile/Documentation/volatile.txt

Might I suggest a different filename: volatile-considered-harmful.txt

Yes, I feel it's important to have a filename that strongly states the dim view its contents take on its subject (the "volatile" type qualifier in this case). Sort of like stable-api-nonsense.txt vs stable-api.txt

Quoted 22 lines
Quoted 8 lines
Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 renamed from Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 +5 -0
@@ -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:

Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 line 15

Quoted 10 lines
Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 renamed from Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 +10 -0
+
+       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 ..."

Quoted 2 lines

I suggest: "... that the variable could be changed outside of the current thread of execution - a sort of simple atomic variable ..."

I'm not so sure here. Why would any C programmer (worth his weight in salt) think that volatile objects are automatically _atomic_? At worst, the mistake someone might make would be to _implement_ locking primitives using volatile. "that the variable could be changed outside of the current thread of execution" sounds sufficient to me, and after all, that is exactly what volatile hints to the compiler.

Quoted 9 lines

Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 line 21

Quoted 6 lines
Documentation/volatile.txt 2007-05-09 14:56:40.000000000 -0600 renamed from Documentation/volatile.txt 1969-12-31 17:00:00.000000000 -0700 +6 -0
+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 :

Quoted 5 lines

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.

Yes, this addition would be nice :-)

#3 lore raw
Quoted 2 lines · Satyam Sharma
Quoted 1 line · Jesper Juhl

On 09/05/07, Jonathan Corbet <[email protected]> wrote:

<snip>

Quoted 15 lines
Quoted 15 lines

Documentation/volatile.txt line 15

Quoted 11 lines

+"the new C book") has the following to say about the volatile keyword:

Documentation/volatile.txt +10 -0
+
+       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 ..."

Quoted 5 lines
Quoted 2 lines

I suggest: "... that the variable could be changed outside of the current thread of execution - a sort of simple atomic variable ..."

I'm not so sure here. Why would any C programmer (worth his weight in salt) think that volatile objects are automatically _atomic_? At

I honestly don't really know, but I've encountered that confusion a few times. Both from friends who (for some reason) believed that and from documents on the web that implied it, aparently it's a common confusion - a few examples:

http://lists.freebsd.org/pipermail/freebsd-perl/2004-June/000124.html
"... volatile (atomic) fixes the problem. ..."

http://blogs.msdn.com/ricom/archive/2006/04/28/586406.aspx
"That's the point of the volatile keyword. It makes sure that
the line "dict = d;" is atomic."

http://forum.java.sun.com/thread.jspa?threadID=5126877&start=0
"A volatile variable is also guaranteed to be read or written
as an atomic operation ..." (yes, this link talks about Java, which I don't know, but if java volatile means atomic, that might explain why some people assume the same for C).

In any case, it's not an uncommon belief, so I just thought it made sense to also make that little note.

Quoted 5 lines

worst, the mistake someone might make would be to _implement_ locking primitives using volatile. "that the variable could be changed outside of the current thread of execution" sounds sufficient to me, and after all, that is exactly what volatile hints to the compiler.

Signature · 3 lines
Jesper Juhl <[email protected]>
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
#4 lore raw
Quoted 24 lines · Jesper Juhl
Quoted 10 lines · Satyam Sharma
Quoted 6 lines · Jesper Juhl

[snip]
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 ..."

I'm not so sure here. Why would any C programmer (worth his weight in salt) think that volatile objects are automatically _atomic_? At

I honestly don't really know, but I've encountered that confusion a few times. Both from friends who (for some reason) believed that and from documents on the web that implied it, aparently it's a common confusion - a few examples:

http://lists.freebsd.org/pipermail/freebsd-perl/2004-June/000124.html
"... volatile (atomic) fixes the problem. ..."

http://blogs.msdn.com/ricom/archive/2006/04/28/586406.aspx
"That's the point of the volatile keyword. It makes sure that
the line "dict = d;" is atomic."

Quoted 5 lines

http://forum.java.sun.com/thread.jspa?threadID=5126877&start=0
"A volatile variable is also guaranteed to be read or written
as an atomic operation ..." (yes, this link talks about Java, which I don't know, but if java volatile means atomic, that might explain why some people assume the same for C).

Perl / Microsoft / Java programmers are probably not worth their weight in salt anyway :-)

I'm not an expert in any of the above platforms either, so don't know if the semantics of "volatile" in those other languages are different from that in C -- and this document clearly applies to only the kernel (and thus C). But if this volatile == atomic disease is indeed common among _C_ programmers too, then your suggested addition would make sense.

#5 lore raw
In reply to Jonathan Corbet

Imo, the best style to relay information is by directly stating facts. I'm going to try to suggest some improvements on this..

Documentation/volatile.txt line 26

Quoted 10 lines · Jonathan Corbet

Andrew Morton recently called out[1] use of volatile in a

Documentation/volatile.txt +9 -0
+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.

I think previous text is unnecessary. Just tell the reasons why volatile is bad and what should be used instead, no need to quote people.

Documentation/volatile.txt line 31

Quoted 4 lines
Documentation/volatile.txt +4 -0
+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.  

Again, I would write this in non-personal way:

"Volatile is often used to prevent optimization, but it is not the behavior that is actually wanted. Access to data must be protected and handled with kernel provided synchronization, mutual exclusion and barriers tools. These tools make volatile useless."

Documentation/volatile.txt

Quoted 4 lines
Documentation/volatile.txt +4 -0
+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.

The previous suggestion takes care of explaining that, remove this.

Quoted 4 lines

If volatile is still necessary, there is

Documentation/volatile.txt +3 -0
+almost certainly a bug in the code somewhere.  In properly-written kernel
+code, volatile can only serve to slow things down.
+

Documentation/volatile.txt

Quoted 10 lines
Documentation/volatile.txt +10 -0
+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.

Ok

Review

Quoted 3 lines
(quoted diff) +3 -0
+The spinlock
+primitives act as memory barriers - they are explicitly written to do so -
+meaning that data accesses will not be optimized across them.

"Spinlock primitives will serialise execution of code regions, and memory barrier functions must be used inside spinlocks to force loads and stores."

Documentation/volatile.txt line 54

Quoted 4 lines

So the

Documentation/volatile.txt +3 -0
+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.

I would say it directly:

"Spinlock will force an implicit memory barrier, thus preventing optimizations to data access."

Documentation/volatile.txt

Quoted 5 lines
Documentation/volatile.txt +5 -0
+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.

ok

Documentation/volatile.txt line 66

Quoted 7 lines

This is why Linus says:

Documentation/volatile.txt +6 -0
+
+	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".

Unnecessary quoting, imo. Tell the same information directly without personifying the statement.

Signature · 3 lines
Heikki Orsila			Barbie's law:
[email protected]		"Math is hard, let's go shopping!"
http://www.iki.fi/shd
#6 lore raw
Quoted 4 lines · Heikki Orsila

Imo, the best style to relay information is by directly stating facts. I'm going to try to suggest some improvements on this..

On Wed, May 09, 2007 at 03:05:44PM -0600, Jonathan Corbet wrote:

Quoted 12 lines

Documentation/volatile.txt line 26

Quoted 10 lines

Andrew Morton recently called out[1] use of volatile in a

Documentation/volatile.txt +9 -0
+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.

I think previous text is unnecessary. Just tell the reasons why volatile is bad and what should be used instead, no need to quote people.

Ack, the doc. history isn't needed.

Quoted 66 lines

Documentation/volatile.txt line 31

Quoted 4 lines
Documentation/volatile.txt +4 -0
+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.  

Again, I would write this in non-personal way:

"Volatile is often used to prevent optimization, but it is not the
behavior that is actually wanted. Access to data must be protected and handled with kernel provided synchronization, mutual exclusion and barriers tools. These tools make volatile useless."

Documentation/volatile.txt

Quoted 4 lines
Documentation/volatile.txt +4 -0
+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.

The previous suggestion takes care of explaining that, remove this.

Quoted 4 lines

If volatile is still necessary, there is

Documentation/volatile.txt +3 -0
+almost certainly a bug in the code somewhere.  In properly-written kernel
+code, volatile can only serve to slow things down.
+

Documentation/volatile.txt

Quoted 10 lines
Documentation/volatile.txt +10 -0
+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.

Ok

Review

Quoted 3 lines
(quoted diff) +3 -0
+The spinlock
+primitives act as memory barriers - they are explicitly written to do so -
+meaning that data accesses will not be optimized across them.

"Spinlock primitives will serialise execution of code regions, and memory barrier functions must be used inside spinlocks to force loads and stores."

Documentation/volatile.txt line 54

Quoted 4 lines

So the

Documentation/volatile.txt +3 -0
+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.

I would say it directly:

"Spinlock will force an implicit memory barrier, thus preventing optimizations to data access."

Documentation/volatile.txt

Quoted 5 lines
Documentation/volatile.txt +5 -0
+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.

ok

Documentation/volatile.txt line 66

Quoted 7 lines

This is why Linus says:

Documentation/volatile.txt +6 -0
+
+	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".

Unnecessary quoting, imo. Tell the same information directly without personifying the statement.

Signature · 2 lines
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
#7 lore raw
In reply to Jonathan Corbet
Quoted 4 lines · Jonathan Corbet

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?
...

---

I think the size of this file is OK as a separate file, but much too big for CodingStyle. It would be good to also write a pithy paragraph to go into CodingStyle to convey the rule and point at this file.

scott

#8 lore raw
In reply to Jonathan Corbet

Documentation/volatile.txt line 72

Quoted 2 lines · Jonathan Corbet
Documentation/volatile.txt +2 -0
+The volatile storage class was originally meant for memory-mapped I/O
+registers.  Within the kernel, register accesses, too, should be protected

I don't think it deserves to be added in documentation, but just for
reference: in userspace "volatile" is needed in signals (posix mandates some variables to be volatile, as API, not as funtionality). I don't know if this was also on the original signal handling.

Anyway user space APIs are not kernel problem ;-)

ciao
cate

#9 lore raw
In reply to Jonathan Corbet
Quoted 5 lines · Jonathan Corbet

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?

Comments welcome,

I have found one use of volatile which I consider legitimate: pointers to data structures read or written by I/O devices in coherent memory (typically pci_coherent memory.) This is local to device drivers, but as far as I can tell, the use of volatile here is legitimate, although arguably it will be redundant in > 90% of all cases due to the incidental presence of other memory barriers.

In Ethernet drivers, for example, it is common for the network card to maintain a pointer in host memory the the latest descriptor written; you will generally have a loop of the form:

	while ((this_pointer = *pointer_ptr) > my_last_pointer) {
		for (pkt = my_last_pointer; pkt < this_pointer; pkt++)
			receeive_packet(pkt);
		my_last_pointer = this_pointer;
	}

pointer_p can then be a volatile pointer into said coherent memory.

-hpa

#10 lore raw
Quoted 11 lines

In Ethernet drivers, for example, it is common for the network card to maintain a pointer in host memory the the latest descriptor written; you will generally have a loop of the form:

	while ((this_pointer = *pointer_ptr) > my_last_pointer) {
		for (pkt = my_last_pointer; pkt < this_pointer; pkt++)
			receeive_packet(pkt);
		my_last_pointer = this_pointer;
	}

pointer_p can then be a volatile pointer into said coherent memory.

True but you can happily use rmb/wmb for this which are clearer and fit the rest of the Linux model better.

Alan

#11 lore raw
In reply to Jonathan Corbet
Quoted 7 lines · Jonathan Corbet
Documentation/volatile.txt +7 -0
+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.

Documentation/volatile.txt line 108

Quoted 10 lines
Documentation/volatile.txt +10 -0
+
+  - 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
+    "stupid legacy" issue in this regard.

It would seem that any variable which is (a) subject to change by other threads or hardware, and (b) the value of which is going to be used without writing the variable, would be a valid use for volatile.

Signature · 3 lines
Bill Davidsen <[email protected]>
  "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot
#12 lore raw
In reply to Jonathan Corbet

Documentation/volatile.txt line 127

Quoted 7 lines · Jonathan Corbet

+CREDITS
+-------

Documentation/volatile.txt +4 -0
+
+Original impetus and research by Randy Dunlap
+Written by Jonathan Corbet
+Improvements via coments from Satyam Sharma Johannes Stezenbach

Just a small spelling mistake: coments should be comments.