Re: [PATCH] "volatile considered harmful", take 2
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
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 -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
On 5/11/07, Johannes Stezenbach <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
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/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
In article <[EMAIL PROTECTED]> you wrote: > +Consider a typical block of kernel code: > + > +spin_lock(_lock); > +do_something_on(_data); > +do_something_else_with(_data); > +spin_unlock(_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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
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(_lock); > +do_something_on(_data); ^^^ > +do_something_else_with(_data); ^^^ > +spin_unlock(_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 / /__/ / _ \/ // /\ \/ / //_/_//_/\_,_/ /_/\_\ [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
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 / /__/ / _ \/ // /\ \/ / //_/_//_/\_,_/ /_/\_\ [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
In article [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
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/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
On 5/11/07, Johannes Stezenbach [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
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 -- - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
On 5/11/07, jimmy bahuleyan <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
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 -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] "volatile considered harmful", take 2
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] "volatile considered harmful", take 2
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 <[EMAIL PROTECTED]> diff --git a/Documentation/volatile-considered-harmful.txt b/Documentation/volatile-considered-harmful.txt new file mode 100644 index 000..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(_lock); +do_something_on(_data); +do_something_else_with(_data); +spin_unlock(_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
[PATCH] volatile considered harmful, take 2
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 [EMAIL PROTECTED] diff --git a/Documentation/volatile-considered-harmful.txt b/Documentation/volatile-considered-harmful.txt new file mode 100644 index 000..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
Re: [PATCH] volatile considered harmful, take 2
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
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 -- - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
On 5/11/07, jimmy bahuleyan [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] volatile considered harmful, take 2
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
