SMP barriers semantics

SMP barriers semantics

[Catalin Marinas]

Hi,

We have an issue with the barriers usage/implementation on ARM and I
would like some clarification.

As a background - latest ARM processors have two kinds of barriers - a
lightweight one (DMB) which basically only ensures the ordering of
accesses to the same memory type (the definition is a bit more
complicated but in the context of Linux this is a safe simplification).
The second kind of barrier is a heavyweight one (DSB) which drains the
write buffers.

Both *mb() and smp_*mb() are currently implemented with the lightweight
version (DMB) but this is not enough for coherent DMA operations where a
DSB is needed to drain the write buffer before writing to the device I/O
memory for starting the transfer. My proposal on the ARM lists was to
change mb()/wmb() to DSB but leave the smp_*mb() as a DMB.

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)?

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?

Thanks.

-- 
Catalin

Re: SMP barriers semantics

[Paul Mackerras]

On Tue, Mar 02, 2010 at 10:52:58AM +0000, Catalin Marinas wrote:

> We have an issue with the barriers usage/implementation on ARM and I
> would like some clarification.
> 
> As a background - latest ARM processors have two kinds of barriers - a
> lightweight one (DMB) which basically only ensures the ordering of
> accesses to the same memory type (the definition is a bit more
> complicated but in the context of Linux this is a safe simplification).
> The second kind of barrier is a heavyweight one (DSB) which drains the
> write buffers.
> 
> Both *mb() and smp_*mb() are currently implemented with the lightweight
> version (DMB) but this is not enough for coherent DMA operations where a
> DSB is needed to drain the write buffer before writing to the device I/O
> memory for starting the transfer. My proposal on the ARM lists was to
> change mb()/wmb() to DSB but leave the smp_*mb() as a DMB.
> 
> 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.

Paul.

Re: SMP barriers semantics

[Catalin Marinas]

On Wed, 2010-03-03 at 00:55 +0000, 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.

Thanks for clarification.

> > 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.

I cc'ed the r8169.c maintainer.

But from the architectural support perspective, we don't need to support
more than a lightweight barrier in this case.

Thanks.

-- 
Catalin

Re: SMP barriers semantics

[David Dillow]

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().

Re: SMP barriers semantics

[Russell King]

On Wed, Mar 03, 2010 at 09:23:46PM -0500, David Dillow wrote:
> On Wed, 2010-03-03 at 11:55 +1100, Paul Mackerras wrote:
> > 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().

There's a whole bunch of other drivers doing exactly the same thing -
just grep drivers/net for smp_wmb(). ;(

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: SMP barriers semantics

[Catalin Marinas]

On Thu, 2010-03-04 at 09:33 +0000, Russell King wrote:
> On Wed, Mar 03, 2010 at 09:23:46PM -0500, David Dillow wrote:
> > On Wed, 2010-03-03 at 11:55 +1100, Paul Mackerras wrote:
> > > 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().
> 
> There's a whole bunch of other drivers doing exactly the same thing -
> just grep drivers/net for smp_wmb(). ;(

Yes, but IMHO we shouldn't penalise the SMP systems by requiring a heavy
barrier rather than just fixing the drivers.

-- 
Catalin

Re: SMP barriers semantics

[Ralf Baechle]

On Wed, Mar 03, 2010 at 12:03:45PM +0000, Catalin Marinas wrote:

> > > 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.
> 
> I cc'ed the r8169.c maintainer.
> 
> But from the architectural support perspective, we don't need to support
> more than a lightweight barrier in this case.

Be afraid, very afraid when you find a non-SMP memory barrier in the
kernel.  A while ago I reviewed a number of uses throughout the kernel and
each one of them was somehow buggy - either entirely unnecessary or should
be replaced with an SMP memory barrier or was simple miss-placed.

  Ralf

Re: SMP barriers semantics

[Jamie Lokier]

Ralf Baechle wrote:
> On Wed, Mar 03, 2010 at 12:03:45PM +0000, Catalin Marinas wrote:
> > > >               /* 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.
> > 
> > I cc'ed the r8169.c maintainer.
> > 
> > But from the architectural support perspective, we don't need to support
> > more than a lightweight barrier in this case.

If the ordering relative to RTL_W16 doesn't matter, imho it would be
much clearer to move the RTL_W16() somewhere else, such as before the
comment.  The comment is quite misleading.

> Be afraid, very afraid when you find a non-SMP memory barrier in the
> kernel.  A while ago I reviewed a number of uses throughout the kernel and
> each one of them was somehow buggy - either entirely unnecessary or should
> be replaced with an SMP memory barrier or was simple miss-placed.

It's not hard to think of cases where a non-SMP barrier is necessary
outside of the kernel code API (dma_* etc.), but it would be quite
interesting if those cases never occur with real devices, or if they
can always be transformed to something else.

The RTL_W16 sample is quite interesting - even if it does not apply to
that particular driver, it's suggestive of a pattern where some device
may need to delay triggering an interrupt (on a different CPU) until a
data structure is written, or that some device may require ordered
MMIO reads and writes.  In the case of interrupts, code like that can
probably be transformed.

-- Jamie

Re: SMP barriers semantics

[Benjamin Herrenschmidt]

On Fri, 2010-03-12 at 13:31 +0100, Ralf Baechle wrote:
> 
> Be afraid, very afraid when you find a non-SMP memory barrier in the
> kernel.  A while ago I reviewed a number of uses throughout the kernel and
> each one of them was somehow buggy - either entirely unnecessary or should
> be replaced with an SMP memory barrier or was simple miss-placed. 

Yes, it's a problem that's plaguing us too.... Now that ARM is moving to
a memory model that is similar to powerpc, maybe we should revisit
things a little bit :-)

The way we sort out the issues currently on powerpc is very heavy handed
and consists of sticking full barriers in our MMIO accessors.

We would -like- to improve that but there's just too much drivers out
there that assume some kind of ordering between consistent memory
accesses and IOs and between IOs and locks.

The only "sane" way out here would thus be to define a set of MMIO
accessors with relaxed ordering semantics along with appropriate
barriers. However, when we tried to do that, we mostly failed agreeing
on what the semantics actually should be since most architectures are
going to be subtly different in that regard.

The __raw_* variants are of no use since they have other effects such as
not performing endian swapping on BE architectures. Plus we don't have
the proper explicit barriers anyways.

Maybe we can agree on a set of relaxed accessors defined specifically
with those semantics (more relaxed implies use of raw_*) :

  - order is guaranteed between MMIOs
  - no order is guaranteed between MMIOs and memory (cachable) accesses
  - no order is guaranteed between MMIOs and spinlocks
  - a read access is not guaranteed to be performed until the read value
    is actually consumed by the processor

Along with barriers along the line of (that's where we may want to
discuss a bit I believe)

  - io_to_mem_rbarrier() : orders MMIO read vs. subsequent memory read
  - mem_to_io_wbarrier() : order memory write vs. subsequent MMIO write
                           (includes spin_unlock ?)
  - io_to_mem_barrier()  : order any MMIO s. subsequent memory accesses
  - mem_to_io_barrier()  : order memory accesses vs any subsequent MMIO
  - flush_io_read(val)   : takes value from MMIO read, enforce it's
                           before completed further instructions are
                           issued, acts as compiler&execution barrier. 

What do you guys think ? I think io_to_mem_rbarrier() and
mem_to_io_wbarrier() cover most useful cases, except maybe the IO vs.
locks.

Maybe for the later (locks) we can stick to the arch is responsible for
making that happen, for example by having MMIO writes set a per-cpu flag
and having spin_unlock do a barrier when that is set, and take that out
of the list of non-guaranteed ordering semantics.

Cheers,
Ben.
 

Re: SMP barriers semantics

[Catalin Marinas]

On Wed, 2010-03-17 at 02:25 +0000, Benjamin Herrenschmidt wrote:
> On Fri, 2010-03-12 at 13:31 +0100, Ralf Baechle wrote:
> > Be afraid, very afraid when you find a non-SMP memory barrier in the
> > kernel.  A while ago I reviewed a number of uses throughout the kernel and
> > each one of them was somehow buggy - either entirely unnecessary or should
> > be replaced with an SMP memory barrier or was simple miss-placed.
> 
> Yes, it's a problem that's plaguing us too.... Now that ARM is moving to
> a memory model that is similar to powerpc, maybe we should revisit
> things a little bit :-)
> 
> The way we sort out the issues currently on powerpc is very heavy handed
> and consists of sticking full barriers in our MMIO accessors.
> 
> We would -like- to improve that but there's just too much drivers out
> there that assume some kind of ordering between consistent memory
> accesses and IOs and between IOs and locks.

I find the memory-barriers.txt document pretty much alright, apart from
some clarification on the SMP barriers with respect to MMIO accesses.

> Maybe we can agree on a set of relaxed accessors defined specifically
> with those semantics (more relaxed implies use of raw_*) :
> 
>   - order is guaranteed between MMIOs
>   - no order is guaranteed between MMIOs and memory (cachable) accesses
>   - no order is guaranteed between MMIOs and spinlocks

I would add:

  - no order is guaranteed between MMIOs and interrupts
enabling/disabling

>   - a read access is not guaranteed to be performed until the read value
>     is actually consumed by the processor

I don't think we have this issue on ARM, we have speculative memory
accesses but not lazy accesses. Other architectures may of course
differ.

> Along with barriers along the line of (that's where we may want to
> discuss a bit I believe)
> 
>   - io_to_mem_rbarrier() : orders MMIO read vs. subsequent memory read

I currently assume rmb() to be enough for this (otherwise I don't see
where else the mandatory barriers would be useful).

>   - mem_to_io_wbarrier() : order memory write vs. subsequent MMIO write

wmb()

>                            (includes spin_unlock ?)

memory-barriers.txt suggests mmiowb() in such cases.

>   - io_to_mem_barrier()  : order any MMIO s. subsequent memory accesses

mb()

>   - mem_to_io_barrier()  : order memory accesses vs any subsequent MMIO

mb()

>   - flush_io_read(val)   : takes value from MMIO read, enforce it's
>                            before completed further instructions are
>                            issued, acts as compiler&execution barrier.

Could add a mmiorb()?

Ideally, we should avoid adding more barriers to Linux since many
drivers don't seem to use them anyway.

-- 
Catalin

Re: SMP barriers semantics

[Jamie Lokier]

Benjamin Herrenschmidt wrote:
> Maybe we can agree on a set of relaxed accessors defined specifically
> with those semantics (more relaxed implies use of raw_*) :
> 
>   - order is guaranteed between MMIOs
>   - no order is guaranteed between MMIOs and spinlocks

No order between MMIOs and spinlocks will be fun :-)

>   - a read access is not guaranteed to be performed until the read value
>     is actually consumed by the processor

How do you define 'consumed'?  It's not obvious: see
read_barrier_depends() on Alpha, and elsewhere speculative read
optimisations (including by the compiler on IA64, in theory).

> Along with barriers along the line of (that's where we may want to
> discuss a bit I believe)
> 
>   - io_to_mem_rbarrier() : orders MMIO read vs. subsequent memory read
>   - mem_to_io_wbarrier() : order memory write vs. subsequent MMIO write
>                            (includes spin_unlock ?)
>   - io_to_mem_barrier()  : order any MMIO s. subsequent memory accesses
>   - mem_to_io_barrier()  : order memory accesses vs any subsequent MMIO
>   - flush_io_read(val)   : takes value from MMIO read, enforce it's
>                            before completed further instructions are
>                            issued, acts as compiler&execution barrier. 
> 
> What do you guys think ? I think io_to_mem_rbarrier() and
> mem_to_io_wbarrier() cover most useful cases, except maybe the IO vs.
> locks.

I'm thinking the current scheme with "heavy" barriers that serialise
everything is better - because it's simple to understand.  I think the
above set are likely to be accidentally misused, and even pass review
occasionally when they are wrong, even if it's just due to brain farts.

The "heavy" barrier names could change from rmb/mb/wmb to
io_rmb/io_mb/io_wmb or something.

Is there any real performance advantage expected from more
fine-grained MMIO barriers?

Another approach is a flags argument to a single macro:
barrier(BARRIER_MMIO_READ_BEFORE | BARRIER_MEM_READ_AFTER), etc.
Each arch would expand it to a barrier at least as strong as the one
requested.  C++0x atomics are going down this route, I think.

-- Jamie

Re: SMP barriers semantics

[Nick Piggin]

On Wed, Mar 17, 2010 at 01:42:43PM +0000, Jamie Lokier wrote:
> Benjamin Herrenschmidt wrote:
> > Maybe we can agree on a set of relaxed accessors defined specifically
> > with those semantics (more relaxed implies use of raw_*) :
> > 
> >   - order is guaranteed between MMIOs
> >   - no order is guaranteed between MMIOs and spinlocks
> 
> No order between MMIOs and spinlocks will be fun :-)

There isn't anyway, and things are pretty messed up in this area
already. We have mmiowb. Some architectures do a bit of mmio vs
lock synchronization. Eg. powerpc. But it doesn't do barriers on
some other locks like bit spinlocks or mutexes or rwsems or
semaphores blah blah.

When this came up I grepped a couple of drivers and found possible
problems straight away.

So IMO, we need to take all these out of lock primitives and just
increase awareness of it. Get rid of mmiowb. wmb() should be enough
to keep mmio stores inside the store to drop any lock (by definition).

Actually I think having an io_acquire_barrier() / io_release_barrier()
for the purpose of keeping ios inside locks is a good idea (paired
inside the actual lock/unlock functions). This basically gives them
a self-documenting property.


> >   - a read access is not guaranteed to be performed until the read value
> >     is actually consumed by the processor
> 
> How do you define 'consumed'?  It's not obvious: see
> read_barrier_depends() on Alpha, and elsewhere speculative read
> optimisations (including by the compiler on IA64, in theory).
> 
> > Along with barriers along the line of (that's where we may want to
> > discuss a bit I believe)
> > 
> >   - io_to_mem_rbarrier() : orders MMIO read vs. subsequent memory read
> >   - mem_to_io_wbarrier() : order memory write vs. subsequent MMIO write
> >                            (includes spin_unlock ?)
> >   - io_to_mem_barrier()  : order any MMIO s. subsequent memory accesses
> >   - mem_to_io_barrier()  : order memory accesses vs any subsequent MMIO
> >   - flush_io_read(val)   : takes value from MMIO read, enforce it's
> >                            before completed further instructions are
> >                            issued, acts as compiler&execution barrier. 
> > 
> > What do you guys think ? I think io_to_mem_rbarrier() and
> > mem_to_io_wbarrier() cover most useful cases, except maybe the IO vs.
> > locks.
> 
> I'm thinking the current scheme with "heavy" barriers that serialise
> everything is better - because it's simple to understand.  I think the
> above set are likely to be accidentally misused, and even pass review
> occasionally when they are wrong, even if it's just due to brain farts.
> 
> The "heavy" barrier names could change from rmb/mb/wmb to
> io_rmb/io_mb/io_wmb or something.
> 
> Is there any real performance advantage expected from more
> fine-grained MMIO barriers?
> 
> Another approach is a flags argument to a single macro:
> barrier(BARRIER_MMIO_READ_BEFORE | BARRIER_MEM_READ_AFTER), etc.

This is just syntax really. Like you I also prefer sticking to simpler
barriers. I would like to see any new barrier introduced on a case by
case basis with before/after numbers and allow discussion on whether it
can be avoided.

So many cases people get the simple barriers wrong...

Re: SMP barriers semantics

[Nick Piggin]

On Mon, Mar 22, 2010 at 11:02:03PM +1100, Nick Piggin wrote:
> On Wed, Mar 17, 2010 at 01:42:43PM +0000, Jamie Lokier wrote:
> > Benjamin Herrenschmidt wrote:
> > > Maybe we can agree on a set of relaxed accessors defined specifically
> > > with those semantics (more relaxed implies use of raw_*) :
> > > 
> > >   - order is guaranteed between MMIOs
> > >   - no order is guaranteed between MMIOs and spinlocks
> > 
> > No order between MMIOs and spinlocks will be fun :-)
> 
> There isn't anyway, and things are pretty messed up in this area
> already. We have mmiowb. Some architectures do a bit of mmio vs
> lock synchronization. Eg. powerpc. But it doesn't do barriers on
> some other locks like bit spinlocks or mutexes or rwsems or
> semaphores blah blah.
> 
> When this came up I grepped a couple of drivers and found possible
> problems straight away.

Ie. with drivers using a mutex to synchronise mmios, which breaks on
powerpc (even though spinlocks would work due to their hack in there).

And on some other architectures the driver would be broken whether it
uses spinlocks or not because it is not using the right barriers.

I think we have basically everything we need right now except to
have things clearly documented and consistent over architectures, and
to have mmio acquire/release to be paired with _any_ locking function
(bitops, atomics, spinlocks, mutexes, etc).

If we enforce *strong* ordering with cacheable accesses and other IO
for regular IO accessors, then a driver would not have to do anything
special for ios or locks or anything. This can be very expensive on
some archs, but IMO it is the best default because any driver that would
work on x86 using these will work on any other arch.

It is much easier for a powerpc or altix developer to take a working
but underperforming driver, identify some of the critical ios and relax
them than it is for them to try to fix a strangely broken driver (and
where the driver developer and 99% of driver testers doesn't have their
hardware).

For relaxed accessors, I think acquire/release (versus cacheable), and
then traditional mb, rmb, wmb should be pretty good start.

Anyone disagree? I would volunteer to post some patches if not...

Re: SMP barriers semantics

[Catalin Marinas]

On Mon, 2010-03-22 at 12:02 +0000, Nick Piggin wrote:
> On Wed, Mar 17, 2010 at 01:42:43PM +0000, Jamie Lokier wrote:
> > Benjamin Herrenschmidt wrote:
> > > Maybe we can agree on a set of relaxed accessors defined specifically
> > > with those semantics (more relaxed implies use of raw_*) :
> > >
> > >   - order is guaranteed between MMIOs
> > >   - no order is guaranteed between MMIOs and spinlocks
> >
> > No order between MMIOs and spinlocks will be fun :-)
> 
> There isn't anyway, and things are pretty messed up in this area
> already. We have mmiowb. Some architectures do a bit of mmio vs
> lock synchronization. Eg. powerpc. But it doesn't do barriers on
> some other locks like bit spinlocks or mutexes or rwsems or
> semaphores blah blah.
> 
> When this came up I grepped a couple of drivers and found possible
> problems straight away.
> 
> So IMO, we need to take all these out of lock primitives and just
> increase awareness of it. Get rid of mmiowb. wmb() should be enough
> to keep mmio stores inside the store to drop any lock (by definition).

I think we have different scenarios for wmb and mmiowb (my
understanding). One is when the driver writes to a coherent DMA buffer
(usually uncached) and it than need to drain the write buffer before
informing the device to start the transfer. That's where wmb() would be
used (with normal uncached memory).

The mmiowb() may need to go beyond the CPU write-buffer level into the
PCI bus etc. but only for relative ordering of the I/O accesses. The
memory-barriers.txt suggests that mmiowb(). My understanding is that
mmiowb() drains any mmio buffers while wmb() drains normal memory
buffers.

> Actually I think having an io_acquire_barrier() / io_release_barrier()
> for the purpose of keeping ios inside locks is a good idea (paired
> inside the actual lock/unlock functions). This basically gives them
> a self-documenting property.

Is there any architecture where mmio accesses are speculated? Do we need
an io_acquire_barrier()? At least on ARM, device memory access is not
restartable by definition, which means that it cannot in general be
speculated. If that's true for other architectures as well, we wouldn't
need anything more than mmiowb() (but I may be wrong here).

Thanks.

-- 
Catalin

Re: SMP barriers semantics

[Nick Piggin]

On Tue, Mar 23, 2010 at 10:24:07AM +0000, Catalin Marinas wrote:
> On Mon, 2010-03-22 at 12:02 +0000, Nick Piggin wrote:
> > On Wed, Mar 17, 2010 at 01:42:43PM +0000, Jamie Lokier wrote:
> > > Benjamin Herrenschmidt wrote:
> > > > Maybe we can agree on a set of relaxed accessors defined specifically
> > > > with those semantics (more relaxed implies use of raw_*) :
> > > >
> > > >   - order is guaranteed between MMIOs
> > > >   - no order is guaranteed between MMIOs and spinlocks
> > >
> > > No order between MMIOs and spinlocks will be fun :-)
> > 
> > There isn't anyway, and things are pretty messed up in this area
> > already. We have mmiowb. Some architectures do a bit of mmio vs
> > lock synchronization. Eg. powerpc. But it doesn't do barriers on
> > some other locks like bit spinlocks or mutexes or rwsems or
> > semaphores blah blah.
> > 
> > When this came up I grepped a couple of drivers and found possible
> > problems straight away.
> > 
> > So IMO, we need to take all these out of lock primitives and just
> > increase awareness of it. Get rid of mmiowb. wmb() should be enough
> > to keep mmio stores inside the store to drop any lock (by definition).
> 
> I think we have different scenarios for wmb and mmiowb (my
> understanding). One is when the driver writes to a coherent DMA buffer
> (usually uncached) and it than need to drain the write buffer before
> informing the device to start the transfer. That's where wmb() would be
> used (with normal uncached memory).
> 
> The mmiowb() may need to go beyond the CPU write-buffer level into the
> PCI bus etc. but only for relative ordering of the I/O accesses. The
> memory-barriers.txt suggests that mmiowb(). My understanding is that
> mmiowb() drains any mmio buffers while wmb() drains normal memory
> buffers.

No barriers are defined to drain anything, only order. wmb() is defined
to order all memory stores, so all previous stores cached and IO are
seen before all subsequent stores. And considering that we are talking
about IO, "seen" obviously means seen by the device as well as other
CPUs.

mmiowb was introduced because this was too heavy on some platforms
(specifically ia64-sn2). And basically they weakened wmb() semantics
and introduced this heavier operation to solve the problem of ordering
IO from different CPUs.

Problem with that is that you can't just redefine an existing age old
API to be weaker. And also because semantics of wmb() get much more
complex (look at the mmiowb calls added by non-SGI people and most are
wrong). And also, code that is tested and works on most common platforms
does not work on others.

What is needed is to make the default accessors strongly ordered and
so driver writers can be really dumb about it, and IO / spinlock etc
synchronization "just works".

So as I said, it's much easier for the smart powerpc programmer to spot
the suboptimal sequence of a few mmio accesses and relax them than it is
for the dumb device driver writer to debug strange reports that aren't
reproduceable on their x86.

(I don't call driver writers dumb but the easier the driver APIs are to
use the better)
 

> > Actually I think having an io_acquire_barrier() / io_release_barrier()
> > for the purpose of keeping ios inside locks is a good idea (paired
> > inside the actual lock/unlock functions). This basically gives them
> > a self-documenting property.
> 
> Is there any architecture where mmio accesses are speculated? Do we need
> an io_acquire_barrier()? At least on ARM, device memory access is not
> restartable by definition, which means that it cannot in general be
> speculated. If that's true for other architectures as well, we wouldn't
> need anything more than mmiowb() (but I may be wrong here).

I would like acquire because it looks nice -- may help with reading 
the code, and we might be able to hook debug checks into them.

But hmm, I don't know if we even need acquire/release IO barriers at
all. Might be better to just fix up wmb(), get rid of mmiowb(),
strengthen IO accessors, and then just add special case barriers as
the need arises.

Re: SMP barriers semantics

[Jamie Lokier]

Nick Piggin wrote:
> On Tue, Mar 23, 2010 at 10:24:07AM +0000, Catalin Marinas wrote:
> 
> But hmm, I don't know if we even need acquire/release IO barriers at
> all. Might be better to just fix up wmb(), get rid of mmiowb(),
> strengthen IO accessors, and then just add special case barriers as
> the need arises.

I must admit I don't understand what wmb() means at this point,
generically from the point of view of arch-independent drivers!  It's
not an inter-CPU ordering (smb_wmb is sufficient), and it doesn't
order all memory and I/O writes (otherwise why mmiowb?).  I suspect a
few people have been unsure, resulting in a bit of confusion about
what goes into different arch implementations of wmb().

For strengthening I/O accessors, do you mean the equivalent of putting
"dmb;dsb" before _and_ after the I/O write inside every call to
writel()?  (That's using ARM as an example: "dmb" means barrier, and
"dsb" means flush write buffers I think, and some ARMs need other,
rather heavier instructions such as a coprocessor instruction or even
an instruction to the L2 cache.)

Because I'm not sure if that's as light as we'd like it to be on
slower CPUs, and __raw_writel or something will get used instead by
some driver writer... leading back to needing to be very clear about
the meaning of wmb/mmiowb.

-- Jamie

Re: SMP barriers semantics

[Nick Piggin]

On Tue, Apr 06, 2010 at 04:43:21PM +0100, Jamie Lokier wrote:
> Nick Piggin wrote:
> > On Tue, Mar 23, 2010 at 10:24:07AM +0000, Catalin Marinas wrote:
> > 
> > But hmm, I don't know if we even need acquire/release IO barriers at
> > all. Might be better to just fix up wmb(), get rid of mmiowb(),
> > strengthen IO accessors, and then just add special case barriers as
> > the need arises.
> 
> I must admit I don't understand what wmb() means at this point,
> generically from the point of view of arch-independent drivers!  It's
> not an inter-CPU ordering (smb_wmb is sufficient), and it doesn't
> order all memory and I/O writes (otherwise why mmiowb?).  I suspect a
> few people have been unsure, resulting in a bit of confusion about
> what goes into different arch implementations of wmb().

No, it has forever been defined to order *everything* (and this
includes ordering as-seen from IO devices, not as-sent from CPU,
otherwise it is absolutely useless on its own).

The problem is that it was weakened and made more complex by one
particular arch, and something else added to make up for it. This simply
doesn't work because it is impossible to audit all drivers (or get
driver writers to understand new semantics).

So, wmb() really should mean what it has always meant. sn2 should be
fixed up. Relaxed schemes should be added as required.

 
> For strengthening I/O accessors, do you mean the equivalent of putting
> "dmb;dsb" before _and_ after the I/O write inside every call to
> writel()?  (That's using ARM as an example: "dmb" means barrier, and
> "dsb" means flush write buffers I think, and some ARMs need other,
> rather heavier instructions such as a coprocessor instruction or even
> an instruction to the L2 cache.)
> 
> Because I'm not sure if that's as light as we'd like it to be on
> slower CPUs, and __raw_writel or something will get used instead by
> some driver writer...

writel_relaxed, I guess. It will get used, but it should be in a small
portion of driver code, that is carefully checked.


> leading back to needing to be very clear about
> the meaning of wmb/mmiowb.

Re: SMP barriers semantics

[Catalin Marinas]

On Tue, 2010-04-06 at 15:20 +0100, Nick Piggin wrote:
> On Tue, Mar 23, 2010 at 10:24:07AM +0000, Catalin Marinas wrote:
> > On Mon, 2010-03-22 at 12:02 +0000, Nick Piggin wrote:
> > > So IMO, we need to take all these out of lock primitives and just
> > > increase awareness of it. Get rid of mmiowb. wmb() should be enough
> > > to keep mmio stores inside the store to drop any lock (by definition).
> >
> > I think we have different scenarios for wmb and mmiowb (my
> > understanding). One is when the driver writes to a coherent DMA buffer
> > (usually uncached) and it than need to drain the write buffer before
> > informing the device to start the transfer. That's where wmb() would be
> > used (with normal uncached memory).
> >
> > The mmiowb() may need to go beyond the CPU write-buffer level into the
> > PCI bus etc. but only for relative ordering of the I/O accesses. The
> > memory-barriers.txt suggests that mmiowb(). My understanding is that
> > mmiowb() drains any mmio buffers while wmb() drains normal memory
> > buffers.
> 
> No barriers are defined to drain anything, only order. wmb() is defined
> to order all memory stores, so all previous stores cached and IO are
> seen before all subsequent stores. And considering that we are talking
> about IO, "seen" obviously means seen by the device as well as other
> CPUs.

Indeed, the barriers aren't defined to drain anything, though they may
do it on specific implementations (or when "seen" actually requires
draining).

The Documentation/DMA-API.txt file mentions that the CPU write buffer
may need to be flushed after writing coherent memory but the kernel
doesn't define any primitive for doing this. Hence my assumption that
this is the job of wmb().

> What is needed is to make the default accessors strongly ordered and
> so driver writers can be really dumb about it, and IO / spinlock etc
> synchronization "just works".

On ARM, the I/O accessors are ordered with respect to device memory
accesses but not with respect to normal non-cacheable memory
(dma_alloc_coherent). If we want to make the writel etc. accessors
ordered with respect to the normal non-cacheable memory, that would be
really expensive on several ARM platforms. Apart from the CPU barrier (a
full one - DSB - to drain the write buffer), some platforms require
draining the write buffer of the L2 cache as well (by writing to other
registers to the L2 cache controller).

So I'm more in favour of having stronger semantics for wmb() and leaving
the I/O accessors semantics to only ensure device memory ordering.

-- 
Catalin

Re: SMP barriers semantics

[Jamie Lokier]

Catalin Marinas wrote:
> On ARM, the I/O accessors are ordered with respect to device memory
> accesses but not with respect to normal non-cacheable memory
> (dma_alloc_coherent). If we want to make the writel etc. accessors
> ordered with respect to the normal non-cacheable memory, that would be
> really expensive on several ARM platforms. Apart from the CPU barrier (a
> full one - DSB - to drain the write buffer), some platforms require
> draining the write buffer of the L2 cache as well (by writing to other
> registers to the L2 cache controller).

It is useful to call it non-cacheable memory if the only way to make
it device-visible is an instruction to the L2 cache controller asking
it to flush? :-)

On at least one ARM-compatible core I know of, you can disable write
buffering for a region, independent of caching.  Is it possible in
general, to make dma_alloc_coherent on ARM return unbuffered uncached memory?

> So I'm more in favour of having stronger semantics for wmb() and leaving
> the I/O accessors semantics to only ensure device memory ordering.

From the above, I'm thinking the semantics for wmb() should include:

   - Flushes any buffered writes to memory which is mapped uncached,
     but does not have to flush buffered writes to cached memory.

So if the arch always mapped uncached memory also unbuffered, wmb()
won't have to flush the CPU write buffer, which can be expensive as you say.

You probably already planned on this, but it's good to make
it explicit in any docs - if that's an agreeable semantic.

-- Jamie

Re: SMP barriers semantics

[Catalin Marinas]

On Fri, 2010-04-23 at 17:56 +0100, Jamie Lokier wrote:
> Catalin Marinas wrote:
> > On ARM, the I/O accessors are ordered with respect to device memory
> > accesses but not with respect to normal non-cacheable memory
> > (dma_alloc_coherent). If we want to make the writel etc. accessors
> > ordered with respect to the normal non-cacheable memory, that would be
> > really expensive on several ARM platforms. Apart from the CPU barrier (a
> > full one - DSB - to drain the write buffer), some platforms require
> > draining the write buffer of the L2 cache as well (by writing to other
> > registers to the L2 cache controller).
> 
> It is useful to call it non-cacheable memory if the only way to make
> it device-visible is an instruction to the L2 cache controller asking
> it to flush? :-)

It's not the cache itself but only its write buffer. Ideally, the DSB
should get through to the L2 cache as well there are platforms that
don't do this.

> On at least one ARM-compatible core I know of, you can disable write
> buffering for a region, independent of caching.  Is it possible in
> general, to make dma_alloc_coherent on ARM return unbuffered uncached memory?

On recent ARM cores, it's only the Strongly Ordered memory that can have
the write buffering disabled. But using such mapping for
dma_alloc_coherent() introduces other problems like cache attribute
aliases for a physical location (since the kernel maps the RAM as Normal
memory already). Such aliases are not allowed hence we moved to Normal
Non-cacheable for dma_alloc_coherent().

> > So I'm more in favour of having stronger semantics for wmb() and leaving
> > the I/O accessors semantics to only ensure device memory ordering.
> 
> From the above, I'm thinking the semantics for wmb() should include:
> 
>    - Flushes any buffered writes to memory which is mapped uncached,
>      but does not have to flush buffered writes to cached memory.

Yes.

> So if the arch always mapped uncached memory also unbuffered, wmb()
> won't have to flush the CPU write buffer, which can be expensive as you say.
> 
> You probably already planned on this, but it's good to make
> it explicit in any docs - if that's an agreeable semantic.

The patches for the ARM platform to make wmb() drain the write buffers
(DSB + L2 cache sync) are already in mainline. That was the only way to
allow drivers using dma_alloc_coherent() to work correctly.

-- 
Catalin

Re: SMP barriers semantics

[Jamie Lokier]

Catalin Marinas wrote:
> On recent ARM cores, it's only the Strongly Ordered memory that can have
> the write buffering disabled. But using such mapping for
> dma_alloc_coherent() introduces other problems like cache attribute
> aliases for a physical location (since the kernel maps the RAM as Normal
> memory already). Such aliases are not allowed hence we moved to Normal
> Non-cacheable for dma_alloc_coherent().

I'm surprised aliases between Normal-Cached and Normal-Uncached are ok,
while aliases between Normal-Cached and SO-Uncached are not ok.

Is it theoretically ok by the ARM specs, or just optimistic programming? :-)

If optimism, it's easy to imagine an implementation where
(unrequested) speculative reads populate the cached mapping, and then
accesses to the Normal-Uncached alias of it get a cache hit and use
that, wrongly.  Or, conversely, if it definitely does not treat that
as a cache hit, it's hard to imagine why SO-Uncached would be different.

-- Jamie

Re: SMP barriers semantics

[Catalin Marinas]

On Sat, 2010-04-24 at 02:45 +0100, Jamie Lokier wrote:
> Catalin Marinas wrote:
> > On recent ARM cores, it's only the Strongly Ordered memory that can have
> > the write buffering disabled. But using such mapping for
> > dma_alloc_coherent() introduces other problems like cache attribute
> > aliases for a physical location (since the kernel maps the RAM as Normal
> > memory already). Such aliases are not allowed hence we moved to Normal
> > Non-cacheable for dma_alloc_coherent().
> 
> I'm surprised aliases between Normal-Cached and Normal-Uncached are ok,
> while aliases between Normal-Cached and SO-Uncached are not ok.

In theory, all of the aliases are banned but the Normal cacheable vs
non-cacheable have been allowed in practice and current ARM
implementations can cope with it (such "palliative" measures have been
introduced in 2006). Other aliases aren't guaranteed to work correctly.

Normal Non-cacheable memory uses the write buffer (at both the CPU and
L2 cache level) while the Strongly Ordered doesn't. Normal memory
accesses on the same CPU check the write buffer as well since a CPU is
coherent with itself (on the D side).

-- 
Catalin