Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Thu, Dec 05, 2013 at 01:21:01PM -0700, Jonathan Corbet wrote:
> On Wed,  4 Dec 2013 14:46:59 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > The situations in which ACCESS_ONCE() is required are not well documented,
> > so this commit adds some verbiage to memory-barriers.txt.
> 
> [...]
> 
> > +     But please note that the compiler is also closely watching what you
> > +     do with the value after the ACCESS_ONCE().  For example, suppose you
> > +     do the following and MAX is a preprocessor macro with the value 1:
> > +
> > +	for ((tmp = ACCESS_ONCE(a)) % MAX)
> > +		do_something_with(tmp);
> 
> That sure looks like it was meant to be "while" instead of "for"?

Good catch, fixed!

> [...]
> 
> > + (*) The compiler is within its rights to reorder memory accesses unless
> > +     you tell it not to.  For example, consider the following interaction
> > +     between process-level code and an interrupt handler:
> > +
> > +	void process_level(void)
> > +	{
> > +		msg = get_message();
> > +		flag = true;
> > +	}
> > +
> > +	void interrupt_handler(void)
> > +	{
> > +		if (flag)
> > +			process_message(msg);
> > +	}
> > +
> > +     There is nothing to prevent the the compiler from transforming
> > +     process_level() to the following, in fact, this might well be a
> > +     win for single-threaded code:
> > +
> > +	void process_level(void)
> > +	{
> > +		flag = true;
> > +		msg = get_message();
> > +	}
> > +
> > +     If the interrupt occurs between these two statement, then
> > +     interrupt_handler() might be passed a garbled msg.  Use ACCESS_ONCE()
> > +     to prevent this as follows:
> > +
> > +	void process_level(void)
> > +	{
> > +		ACCESS_ONCE(msg) = get_message();
> > +		ACCESS_ONCE(flag) = true;
> > +	}
> > +
> > +	void interrupt_handler(void)
> > +	{
> > +		if (ACCESS_ONCE(flag))
> > +			process_message(ACCESS_ONCE(msg));
> > +	}
> 
> Looking at this, I find myself wondering why you couldn't just put a
> barrier() between the two statements in process_level()?  ACCESS_ONCE()
> seems like a heavy hammer to just avoid reordering of two assignments.
> What am I missing, and what could be added here to keep the other folks as
> dense as me from missing the same thing?

You could use barrier() from an ordering viewpoint.  However,
ACCESS_ONCE() is often lighter weight than barrier().  ACCESS_ONCE()
affects only that one access, while barrier() forces the compiler to
forget pretty much anything it previously gleaned from any region of
memory, including private locations that no one else touches.

I am adding a sentence saying that pure ordering can be provided
by barrier(), though often at higher cost.

							Thanx, Paul