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

[James Bottomley]

On Sun, 2004-08-15 at 13:45, John David Anglin wrote:
> This was in fact reported a number of months ago.  The issue as I now
> understand it is that the ldcw macro needs to contain a memory barrier,
> so memory stores/loads don't get reordered past it.  I think earlier
> versions of GCC didn't reorder past volatile, but from 3.3 on, it is
> smarter about these issues.  The macro also doesn't properly indicate
> that the lock memory location is modified, if I recall correctly.

The specific problem is in this piece of code:

if (!list_empty(list)) {
	lock;
	list_del_init();

>From looking at the disassembly, the list->next ptr was being carried
across the lock in a register, so the failing case was where we got
contention on this (i.e. deleting two sequential items from the list).

Placing a single memory barrier anywhere in the spinlock would have
prevented this.

> I think this would do the trick:
> 
> #define __ldcw(a) ({ \
>         unsigned __ret; \
> 	__asm__ __volatile__("ldcw 0(%1),%0" \
> 			     : "=r" (__ret) : "r" (a) : "memory"); \
> 	__ret; \
> })
> 
> The alignment macro is just adjusting a pointer.  GCC should be able to
> schedule this code.
> 
> The reset of the lock should be made into a macro with a memory
> barrier.  It should use a coherent store instead of just "*a = 1".
> Something like:
> 
>     __asm__ __volatile__ ("stw,ma %1,0(%0)"
> 			  : : "r" (&lock), "r" (tmp) : "memory");
> 
> This does the reset using a coherent store (PA 2.0) and indicates
> to GCC that all memory is clobbered.

Yes, that would certainly work.  However, the lock variable is volatile,
so gcc should understand the implications.

> With these changes, I think you could get rid of the explicit
> memory barriers, "mb()", that you currently have.  If you don't
> want to include the memory clobbers in these macros, then we should
> add clobbers for the lock memory location so GCC knows these locations
> are clobbered.

The two mb()'s surrounding the actual lock code looks safest to me.  It
should mean that whatever future optimisations get put into gcc nothing
can ever contaminate our spinlocks (i.e. overkill rather than too
little).

James