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.480214...@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.
