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

[Grant Grundler]

On Fri, Aug 20, 2004 at 12:57:55PM -0400, James Bottomley wrote:
> 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.

I consider 2 and 3 the same thing basically.
Maybe on other arches this distinction matters?
(as noted below, I think I'm still missing something here...I need
to re-read the thread in January two or three more times)

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

Ok. I missed a change to mb() (include/asm-parisc/system.h):
...
** The __asm__ op below simple prevents gcc/ld from reordering
** instructions across the mb() "call".
*/
#define mb()            __asm__ __volatile__("":::"memory");    /* barrier() */
...

mb() now has "memory" reference.
This prevents gcc from reordering the memory accesses across the mb().

The comment is incorrect for newer gcc if I understand this all corectly.
The "memory" is what prevents gcc from re-ordering instructions.

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

I think it's correct - we need the mb() before the lock and after the unlock.

>   It prevents this compiler reordering.  This is
> probably overkill, but I'd like to be sure...

I don't think so.
Putting mb() before/after each lock and then again before/after
each unlock would be overkill.

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

ok - got that.

> It's the asm volatile that enforces number 2.

sorry - I'm missing something subtle here.
I fail to see what "asm volatile" is enforcing.

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

acquire/release semantics as implemented by ia64 are probably
the minimal set. I think we need to prevent gcc from re-ordering memory
access before the acquire (lock) as well.

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

oh! I wonder how parisc ever worked some days...

thanks,
grant