| Date | Fri, 24 Mar 2017 14:16:22 -0700 | From | Darren Hart <> | Subject | Re: [PATCH -v6 02/13] futex: Use smp_store_release() in mark_wake_futex() |
| |
On Wed, Mar 22, 2017 at 11:35:49AM +0100, Peter Zijlstra wrote: > Since the futex_q can dissapear the instruction after assigning NULL, > this really should be a RELEASE barrier. That stops loads from hitting > dead memory too. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
I reviewed this carefully in the previous thread, confirming that despite the move to wake queues, spurious wakeups can still lead to the situration Peter describes. As such:
Reviewed-by: Darren Hart (VMware) <dvhart@infradead.org>
My only suggestion would be to clarify the language in the preceding comment to make that obvious, as well as clarify which plist_del it is referring to since it has been moved under the __unqueue_futex. I can do that as a follow-on though.
> --- > kernel/futex.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -1288,8 +1288,7 @@ static void mark_wake_futex(struct wake_ > * memory barrier is required here to prevent the following > * store to lock_ptr from getting ahead of the plist_del. > */ > - smp_wmb(); > - q->lock_ptr = NULL; > + smp_store_release(&q->lock_ptr, NULL); > } > > static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *top_waiter, > > >
-- Darren Hart VMware Open Source Technology Center
|