Re: [PATCH] bug in futex unqueue_me

[Christian Borntraeger <borntrae@de.ibm.com>]

On Sunday 30 July 2006 08:38, Ingo Molnar wrote:
> interesting, how is this possible? We do a spin_lock(lock_ptr), and
> taking a spinlock is an implicit barrier(). So gcc must not delay
> evaluating lock_ptr to inside the critical section. And as far as i can
> see the s390 spinlock implementation goes through an 'asm volatile'
> piece of code, which is a barrier already. So how could this have
> happened?

spin_lock is a barrier, but isnt the barrierness too late here? The compiler 
reloads the value of lock_ptr after the "if(lock_ptr)" and *before* calling 
spin_lock(lock_ptr):
     3ee:       e3 c0 b0 28 00 04       lg      %r12,40(%r11)
				q->lockptr in r12
     3f4:       b9 02 00 cc             ltgr    %r12,%r12
				load and test r12
     3f8:       a7 84 00 4b             je      48e <unqueue_me+0xc6>
				if r12 == 0 jump away
     3fc:       e3 20 b0 28 00 04       lg      %r2,40(%r11)
				q->lockptr in r2
     402:       c0 e5 00 00 00 00       brasl   %r14,402 <unqueue_me+0x3a>
                        404: R_390_PC32DBL      _spin_lock+0x2
				call spinlock (r2 is first parameter)


I really dont know why the compiler reloads lock_ptr from memory at all, but I 
will talk to our compiler guys to find out. 


> I have nothing against adding a barrier(), but we should first 
> investigate why the spin_lock() didnt act as a barrier - there might be
> other, similar bugs hiding. (we rely on spin_lock()s barrier-ness in a
> fair number of places)
See above. I think the barrier must be before "if(lock_ptr)" and not 
afterwards. 

> yes, it is always a pointer to a valid spinlock, or NULL.
> futex_requeue() can change the spinlock from one to another, and
> wake_futex() can change it to NULL. The futex unqueue_me() fastpath is
> when a futex waiter was woken - in which case it's NULL. But it can
> still be non-NULL if we timed out or a signal happened, in which case we
> may race with a wakeup or a requeue. futex_requeue() changes the
> spinlock pointer if it holds both the old and the new spinlock. So it's
> race-free as far as i can see.
Ok, looks fine then. 

-- 
Mit freundlichen Grüßen / Best Regards

Christian Borntraeger
Linux Software Engineer zSeries Linux & Virtualization