Re: [PATCH 4/5] futex: Avoid taking hb lock if nothing to wakeup

Thomas Gleixner
2013-11-25T12:01:26Z

On Sat, 23 Nov 2013, Davidlohr Bueso wrote:
> On Sat, 2013-11-23 at 19:46 -0800, Linus Torvalds wrote:
> > On Sat, Nov 23, 2013 at 5:16 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > > Now the question is why we queue the waiter _AFTER_ reading the user
> > > space value. The comment in the code is pretty non sensical:
> > >
> > > * On the other hand, we insert q and release the hash-bucket only
> > > * after testing *uaddr. This guarantees that futex_wait() will NOT
> > > * absorb a wakeup if *uaddr does not match the desired values
> > > * while the syscall executes.
> > >
> > > There is no reason why we cannot queue _BEFORE_ reading the user space
> > > value. We just have to dequeue in all the error handling cases, but
> > > for the fast path it does not matter at all.
> > >
> > > CPU 0 CPU 1
> > >
> > > val = *futex;
> > > futex_wait(futex, val);
> > >
> > > spin_lock(&hb->lock);
> > >
> > > plist_add(hb, self);
> > > smp_wmb();
> > >
> > > uval = *futex;
> > > *futex = newval;
> > > futex_wake();
> > >
> > > smp_rmb();
> > > if (plist_empty(hb))
> > > return;
> > > ...
> >
> > This would seem to be a nicer approach indeed, without needing the
> > extra atomics.
>
> Yep, I think we can all agree that doing this optization without atomic
> ops is a big plus.
>
> >
> > Davidlohr, mind trying Thomas' approach?
>
> I just took a quick look and it seems pretty straightforward, but not
> without some details to consider. We basically have to redo/reorder
> futex_wait_setup(), which checks that uval == val, and
> futex_wait_queue_me(), which adds the task to the list and blocks. Now,
> both futex_wait() and futex_wait_requeue_pi() have this logic, but since
> we don't use futex_wake() to wakeup tasks on pi futex_qs, I believe it's
> ok to only change futex_wait(), while the order of the uval checking
> doesn't matter for futex_wait_requeue_pi() so it can stay as is.

There is no mechanism which prevents a futex_wake() call on the inner
futex of the wait_requeue_pi mechanism. So no, we have to change both.

futexes are no place for believe. Either you understand them
completely or you just leave them alone.

Thanks,

tglx