Messages in this thread | | | Subject | Re: [PATCH v2 4/4] futex: Avoid taking hb lock if nothing to wakeup | From | Davidlohr Bueso <> | Date | Tue, 10 Dec 2013 09:36:36 -0800 |
| |
On Tue, 2013-12-10 at 18:15 +0100, Peter Zijlstra wrote: > --- > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -82,12 +82,13 @@ > * The waker side modifies the user space value of the futex and calls > * futex_wake(). It computes the hash bucket and acquires the hash > * bucket lock. Then it looks for waiters on that futex in the hash > - * bucket and wakes them. In scenarios where wakeups are called and no > - * tasks are blocked on a futex, taking the hb spinlock can be avoided > - * and simply return. In order for this optimization to work, ordering > - * guarantees must exist so that the waiter being added to the list is > - * acknowledged when the list is concurrently being checked by the waker, > - * avoiding scenarios like the following: > + * bucket and wakes them. > + * > + * In scenarios where wakeups are called and no tasks are blocked on a futex, > + * taking the hb spinlock can be avoided and simply return. In order for this > + * optimization to work, ordering guarantees must exist so that the waiter > + * being added to the list is acknowledged when the list is concurrently being > + * checked by the waker, avoiding scenarios like the following: > * > * CPU 0 CPU 1 > * val = *futex; > @@ -108,6 +109,7 @@ > * This would cause the waiter on CPU 0 to wait forever because it > * missed the transition of the user space value from val to newval > * and the waker did not find the waiter in the hash bucket queue. > + * > * The correct serialization ensures that a waiter either observes > * the changed user space value before blocking or is woken by a > * concurrent waker: > @@ -117,7 +119,8 @@ > * sys_futex(WAIT, futex, val); > * futex_wait(futex, val); > * > - * mb(); <-- paired with ------ > + * waiters++; > + * mb(); (A) <-- paired with -. > * | > * lock(hash_bucket(futex)); | > * | > @@ -126,22 +129,29 @@ > * | sys_futex(WAKE, futex); > * | futex_wake(futex); > * | > - * --------> mb(); > + * `-------> mb(); (B) > * if (uval == val) > * queue(); > * unlock(hash_bucket(futex)); > - * schedule(); if (!queue_empty()) > + * schedule(); if (waiters) > * lock(hash_bucket(futex)); > * wake_waiters(futex); > * unlock(hash_bucket(futex)); > * > - * The length of the list is tracked with atomic ops (hb->waiters), > - * providing the necessary memory barriers for the waiters. For the > - * waker side, however, we rely on get_futex_key_refs(), using either > - * ihold() or the atomic_inc(), for shared futexes. The former provides > - * a full mb on all architectures. For architectures that do not have an > - * implicit barrier in atomic_inc/dec, we explicitly add it - please > - * refer to futex_get_mm() and hb_waiters_inc/dec().
IMHO this text gives a nice summary instead of documenting each function with this things like '... implies MB (B)'. Anyway, I'll resend this patch with your corrections.
Thanks, Davidlohr
| |