lkml.org 
[lkml]   [2017]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -v6 08/13] futex: Pull rt_mutex_futex_unlock() out from under hb->lock
On Wed, Apr 05, 2017 at 04:52:25PM -0700, Darren Hart wrote:
> On Wed, Mar 22, 2017 at 11:35:55AM +0100, Peter Zijlstra wrote:
> > There's a number of 'interesting' problems, all caused by holding
> > hb->lock while doing the rt_mutex_unlock() equivalient.
> >
> > Notably:
> >
> > - a PI inversion on hb->lock; and,
> >
> > - a DL crash because of pointer instability.
>
> A DL crash? What is this? Can you elaborate a bit?

See here:

https://lkml.kernel.org/r/20170323145606.480214279@infradead.org


> > @@ -1380,48 +1387,40 @@ static void mark_wake_futex(struct wake_
> > smp_store_release(&q->lock_ptr, NULL);
> > }
> >
> > -static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *top_waiter,
> > - struct futex_hash_bucket *hb)
> > +/*
> > + * Caller must hold a reference on @pi_state.
> > + */
> > +static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_pi_state *pi_state)
> > {
> > - struct task_struct *new_owner;
> > - struct futex_pi_state *pi_state = top_waiter->pi_state;
> > u32 uninitialized_var(curval), newval;
> > + struct task_struct *new_owner;
> > + bool deboost = false;
> > DEFINE_WAKE_Q(wake_q);
> > - bool deboost;
>
> Nit: Based on what I've seen from Thomas and others, I ask for declarations in
> decreasing order of line length. So deboost should have stayed where it was.

Hurm, yeah I mostly do that. No idea what went wrong there.

> >
> > /*
> > @@ -2232,7 +2229,8 @@ static int fixup_pi_state_owner(u32 __us
> > /*
> > * We are here either because we stole the rtmutex from the
> > * previous highest priority waiter or we are the highest priority
> > - * waiter but failed to get the rtmutex the first time.
> > + * waiter but have failed to get the rtmutex the first time.
> > + *
> > * We have to replace the newowner TID in the user space variable.
> > * This must be atomic as we have to preserve the owner died bit here.
> > *
> > @@ -2249,7 +2247,7 @@ static int fixup_pi_state_owner(u32 __us
> > if (get_futex_value_locked(&uval, uaddr))
> > goto handle_fault;
> >
> > - while (1) {
> > + for (;;) {
>
> As far as I'm aware, there is no difference and both are used throughout the
> kernel (with the while version having 50% more instances). Is there more to this
> than personal preference?

Nope. Only that. I think I played around with the loop at one point and
this is all that remained of that.

> > newval = (uval & FUTEX_OWNER_DIED) | newtid;
> >
> > if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval))
> > @@ -2345,6 +2343,10 @@ static int fixup_owner(u32 __user *uaddr
> > /*
> > * Got the lock. We might not be the anticipated owner if we
> > * did a lock-steal - fix up the PI-state in that case:
> > + *
> > + * We can safely read pi_state->owner without holding wait_lock
> > + * because we now own the rt_mutex, only the owner will attempt
> > + * to change it.
>
> This seems to contradict the Serialization and lifetime rules:
>
> + * pi_mutex->wait_lock:
> + *
> + * {uval, pi_state}
> + *
> + * (and pi_mutex 'obviously')
>
> It would seem that simply holding pi_mutex is sufficient for serialization on
> pi_state->owner then.

Not a contradiction; just a very specific special case. If current is
the owner of a lock, said owner will not be going anywhere.

> > +
> > + /*
> > + * Grab a reference on the pi_state and drop hb->lock.
> > + *
> > + * The reference ensures pi_state lives, dropping the hb->lock
> > + * is tricky.. wake_futex_pi() will take rt_mutex::wait_lock to
> > + * close the races against futex_lock_pi(), but in case of
> > + * _any_ fail we'll abort and retry the whole deal.
>
> s/fail/failure/

I don't think that survives the patch-set. That is, I cannot find it in
the current code.

\
 
 \ /
  Last update: 2017-04-06 14:43    [W:0.081 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site