Message-ID: <20131031043258.GQ4126@linux.vnet.ibm.com>
Date:	Wed, 30 Oct 2013 21:32:58 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Victor Kaplansky <VICTORK@...ibm.com>
Cc:	Anton Blanchard <anton@...ba.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux PPC dev <linuxppc-dev@...abs.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	Michael Ellerman <michael@...erman.id.au>,
	Michael Neuling <mikey@...ling.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: perf events ring buffer memory barrier on powerpc

On Wed, Oct 30, 2013 at 03:28:54PM +0200, Victor Kaplansky wrote:
> "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> wrote on 10/30/2013
> 11:27:25 AM:
> 
> > If you were to back up that insistence with a description of the
> orderings
> > you are relying on, why other orderings are not important, and how the
> > important orderings are enforced, I might be tempted to pay attention
> > to your opinion.
> >
> >                      Thanx, Paul
> 
> NP, though, I feel too embarrassed to explain things about memory barriers
> when
> one of the authors of Documentation/memory-barriers.txt is on cc: list ;-)
> 
> Disclaimer: it is anyway impossible to prove lack of *any* problem.

If you want to play the "omit memory barriers" game, then proving a
negative is in fact the task before you.

> Having said that, lets look into an example in
> Documentation/circular-buffers.txt:

And the correctness of this code has been called into question.  :-(
An embarrassingly long time ago -- I need to get this either proven
or fixed.

> > THE PRODUCER
> > ------------
> >
> > The producer will look something like this:
> >
> >       spin_lock(&producer_lock);
> >
> >       unsigned long head = buffer->head;
> >       unsigned long tail = ACCESS_ONCE(buffer->tail);
> >
> >       if (CIRC_SPACE(head, tail, buffer->size) >= 1) {
> >               /* insert one item into the buffer */
> >               struct item *item = buffer[head];
> >
> >               produce_item(item);
> >
> >               smp_wmb(); /* commit the item before incrementing the head
> */
> >
> >               buffer->head = (head + 1) & (buffer->size - 1);
> >
> >               /* wake_up() will make sure that the head is committed
> before
> >                * waking anyone up */
> >               wake_up(consumer);
> >       }
> >
> >       spin_unlock(&producer_lock);
> 
> We can see that authors of the document didn't put any memory barrier
> after "buffer->tail" read and before "produce_item(item)" and I think they
> have
> a good reason.
> 
> Lets consider an imaginary smp_mb() right before "produce_item(item);".
> Such a barrier will ensure that -
> 
>     - the memory read on "buffer->tail" is completed
> 	before store to memory pointed by "item" is committed.
> 
> However, the store to "buffer->tail" anyway cannot be completed before
> conditional
> branch implied by "if ()" is proven to execute body statement of the if().
> And the
> latter cannot be proven before read of "buffer->tail" is completed.
> 
> Lets see this other way. Lets imagine that somehow a store to the data
> pointed by "item"
> is completed before we read "buffer->tail". That would mean, that the store
> was completed
> speculatively. But speculative execution of conditional stores is
> prohibited by C/C++ standard,
> otherwise any conditional store at any random place of code could pollute
> shared memory.

Before C/C++11, the closest thing to such a prohibition is use of
volatile, for example, ACCESS_ONCE().  Even in C/C++11, you have to
use atomics to get anything resembing this prohibition.

If you just use normal variables, the compiler is within its rights
to transform something like the following:

	if (a)
		b = 1;
	else
		b = 42;

Into:

	b = 42;
	if (a)
		b = 1;

Many other similar transformations are permitted.  Some are used to all
vector instructions to be used -- the compiler can do a write with an
overly wide vector instruction, then clean up the clobbered variables
later, if it wishes.  Again, if the variables are not marked volatile,
or, in C/C++11, atomic.

> On the other hand, if compiler or processor can prove that condition in
> above if() is going
> to be true (or if speculative store writes the same value as it was before
> write), the
> speculative store *is* allowed. In this case we should not be bothered by
> the fact that
> memory pointed by "item" is written before a read from "buffer->tail" is
> completed.

The compilers don't always know as much as they might about the underlying
hardware's memory model.  Of course, if this code is architecture specific,
it can avoid DEC Alpha's fun and games, which could also violate your
assumptions in the above paragraph:

	http://www.openvms.compaq.com/wizard/wiz_2637.html

Anyway, proving or fixing the code in Documentation/circular-buffers.txt
has been on my list for too long, so I will take a closer look at it.

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists