Re: [PATCH] "volatile considered harmful" document

[Randy Dunlap <randy.dunlap@oracle.com>]

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/