Re: SMP barriers semantics

[David Dillow <dave@thedillows.org>]

On Wed, 2010-03-03 at 11:55 +1100, Paul Mackerras wrote:
> On Tue, Mar 02, 2010 at 10:52:58AM +0000, Catalin Marinas wrote:
> > The main question - are the Linux SMP barriers supposed to have an
> > effect outside of cacheable memory accesses (i.e. ordering wrt I/O
> > accesses)?
> 
> The SMP barriers are only required to order cacheable accesses.  The
> plain (non-SMP) barriers (mb, wmb, rmb) are required to order both
> cacheable and non-cacheable accesses.
> 
> > My understanding from other comments in the kernel source is that the
> > SMP barriers are only meant or cacheable memory but there are drivers
> > that do something like below (e.g. drivers/net/r8169.c):
> > 
> > 		/* We need for force the visibility of tp->intr_mask
> > 		 * for other CPUs, as we can loose an MSI interrupt
> > 		 * and potentially wait for a retransmit timeout if we don't.
> > 		 * The posted write to IntrMask is safe, as it will
> > 		 * eventually make it to the chip and we won't loose anything
> > 		 * until it does.
> > 		 */
> > 		tp->intr_mask = 0xffff;
> > 		smp_wmb();
> > 		RTL_W16(IntrMask, tp->intr_event);
> > 
> > Is this supposed to work given the SMP barriers semantics?
> 
> Well, if the smp_wmb() is supposed to make the assignment to
> tp->intr_mask globally visible before any effects of the RTL_W16(),
> then it's buggy.  But from the comments it appears that the smp_wmb()
> might be intended to order the store to tp->intr_mask with respect to
> following cacheable stores, rather than with respect to the RTL_W16(),
> which would be OK.  I can't say without having a much closer look at
> what that driver is actually doing.

It's buggy. The code was intended to ensure the write to intr_mask was
visible to other CPUs before we told the NIC that it could generate
another interrupt. Give the definition of the barriers above, this
should be wmb() instead of smp_wmb().