Re: [PATCH] - Fix memory ordering problem in wake_futex()

[Robin Holt <holt@sgi.com>]

On Fri, Dec 23, 2005 at 03:59:16PM -0600, Olof Johansson wrote:
> On Fri, Dec 23, 2005 at 03:32:16PM -0600, Jack Steiner wrote:
> 
> > On IA64, the "sync" instructions are actually part of the ld.acq ot st.rel
> > instructions that are used to set/clear spinlocks.
> [...]
> > IA64 implements fencing of ld.acq or st.rel instructions as one-directional
> > barriers.
> 
> So ia64 spin_unlock doesn't do store-store ordering across it. I'm
> surprised this is the first time this causes problems. Other architectures
> seem to order:
> 
> * sparc64 does a membar StoreStore|LoadStore
> * powerpc does lwsync or sync, depending on arch
> * alpha does an mb();
> 
> * x86 is in-order
> 
> So, sounds to me like you need to fix your lock primitives, not add
> barriers to generic code?

I don't think this is a case which is handled by the typical lock
primitives.  Here we essentially have two things being unlocked in
close succession.  The first is the wait queue, the second the futex_q.

There is nothing in the typical unlock path which would require unlocks
to be ordered with respect to each other.  However, in this case, the
futex_q expects to finish processing the wake_up_all before releasing
the lock_ptr.  That is a requirement of wake_futex and not the locking
primitives.  If wake_futex() requires it, then it should be responsible
for enforcing that requirement.

I suppose a step in the right direction would be doing a volatile store
to q->lock_ptr.  I haven't looked, but that should at least prevent the
clearing of lock_ptr until the wait queue is unlocked.

Jack, can you repeat your testing with a cast on the q->lock_ptr line to
a volatile.  After looking at it some more, shouldn't the struct futex_q{}
definition for the spinlock_t *lock_ptr be volatile?


Thanks,
Robin Holt