Messages in this thread | | | From | Christian Borntraeger <> | Subject | Re: [PATCH] bug in futex unqueue_me | Date | Mon, 31 Jul 2006 10:04:15 +0200 |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |