[lkml]   [2014]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] futex: avoid race between requeue and wake
On Tue, Apr 8, 2014 at 9:20 AM, Linus Torvalds
<> wrote:
> However, one exception to this is "requeue_futex()". Which is in fact
> the test-case that Jan points to. There, when we move a futex from one
> hash bucket to another, we do the increment inside the spinlock.
> So I think the change is correct, although the commit message might
> need a bit of improvement.

Hmm. And let's think about that powerpc race, where "spin_is_locked()"
can be false when somebody is waiting to get the lock..

We're good on the *normal* paths, exactly because we do that
hb_waiters_inc() before getting the lock (which also forces a memory
barrier), so the spin_is_locked() test is unnecessary and we have no

But now Jan's patch is re-introducing spin_is_locked(), so let's think
about what that means for the requeue_futex() case. Especially since
the two accesses in

spin_is_locked(&hb->lock) || atomic_read(&hb->waiters)

are not ordered any way wrt each other (they are both just
unconstrained reads). How does this all work? This is making me

Moving the hb_waiters_inc() to before taking the spinlock (inside
double_lock_hb() or whatever) would make me more comfortable wrt this
whole situation, if only because now the rules would be the same for
that requeue case as for adding the futex in the first place.

But maybe somebody can just show that Jan's simpler patch is
sufficient for that case for some fundamental reason that I
overlooked. In which case I would just want a comment.

It's a day for misquoting Star Wars: "Help me, Davidlohr Bueso. You
are my only hope"

Although comments from others would be good too. PeterZ? He wasn't
originally cc'd, but was involved with the original ordering
discussion. Adding..


 \ /
  Last update: 2014-04-08 20:01    [W:0.110 / U:2.416 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site