[parisc-linux] Re: [parisc-linux-cvs] linux-2.6 jejb

[James Bottomley]

On Fri, 2004-08-20 at 11:25, Grant Grundler wrote:
> On Fri, Aug 20, 2004 at 10:02:49AM -0400, John David Anglin wrote:
> > I'm far from an expert on this but I believe that gcc C will reorder
> > memory accesses for better scheduling if it can prove that the accesses
> > don't alias each other.
> 
> I'm trying hard to avoid confusion over "memory ordering" (a HW feature
> of alpha, ia64) and "instruction re-ordering" done by the tool chain.
> "reordering memory accesses"  != "weak memory ordering".

There are three things actually:

1. Hardware instruction execution reordering that causes visible
external memory reference reordering (what PA refers to as weakly
ordered).

2. Compiler reordering of instructions that cause (if executed in this
sequence on the processor) a visible reordering of the sequence.

3. Compiler assumptions about the validity of memory locations.

Our spinlock problem was number 3.  We need the compiler to know that
all values it loaded from external memory (i.e. references from outside
the function) before the spinlock need to be dropped and reloaded after
the spinlock.  This is what adding the mb() to our spinlock does.

Also, I was worried (although there's no evidence in the code) that gcc
may try to do overly clever optimisations with our spinlock code, since
its inlined and mostly in C.  That's why I put a mb() before and after
the spinlock code.  It prevents this compiler reordering.  This is
probably overkill, but I'd like to be sure...

> Agreed. I will assert memory barriers are not required (parisc HW is
> strongly ordered) and we have to tell gcc/binutils it can't re-order
> memory accesses across critical region boundaries (ie lock/unlock). 
> I don't know how to implement the latter and if adding "memory" to the
> ldcw/stw (lock/unlock) asm instructions is sufficient, then let's do that.

Well, no.  Adding "memory" to the clobbers list is number 3.  It's the
asm volatile that enforces number 2.

I don't think anything needs to be done to our ldcw since it's already
operating on volatile data for the spinlock.  I could be persuaded that
the only mb() we actually need in the spin_lock is the one at the end.

> >   3.3 and later are more aggressive than earlier
> > versions in this regard.
> 
> yeah - I guess I didn't understand this from the original posting in January.

Actually, the bug was showing up with 3.0.4 compiled kernels as well.

James