Messages in this thread | | | Date | Wed, 09 May 2007 15:19:48 -0700 | From | Randy Dunlap <> | Subject | Re: [PATCH] "volatile considered harmful" document |
| |
Heikki Orsila wrote: > 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: >> 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. > > 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.
>> +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." > >> +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. > >> 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. > > Ok > >> +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." > >> 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. > > I would say it directly: > > "Spinlock will force an implicit memory barrier, thus preventing > optimizations to data access." > >> +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 > >> 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". > > Unnecessary quoting, imo. Tell the same information directly without > personifying the statement.
-- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |