Messages in this thread | | | Date | Tue, 7 Apr 2015 21:47:13 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 2/3] futex: avoid double wake up in futex_wake() on -RT |
| |
On Tue, 7 Apr 2015, Sebastian Andrzej Siewior wrote: > futex_wake() wakes the waiter while holding the hb->lock. This leads to > a similar double wake up on -RT if the waiter has a higher priority than > the process perfroming the wake up.
Well, the non pi wakeup is designed in a way that the waiter side does not take the hash bucket lock.
So if you observe that hb->lock contention and the resulting PI boosting dance on RT, then it's not the wakeup/waiter exit path. That must be something like this:
T1 T2 wakeup()
-->preemption
sys_exit(futex_wait) ... sys_enter(futex_wake); lock(hb->lock);
> -/* > - * The hash bucket lock must be held when this is called. > - * Afterwards, the futex_q must not be accessed. > - */
The comment should stay here, because you are not allowed to access the futex_q after q->lock_ptr has been set to NULL and that happens in this function.
> -static void wake_futex(struct futex_q *q) > +static struct task_struct *__wake_futex(struct futex_q *q) > { > struct task_struct *p = q->task; > > if (WARN(q->pi_state || q->rt_waiter, "refusing to wake PI futex\n")) > - return; > + return NULL; > > /* > * We set q->lock_ptr = NULL _before_ we wake up the task. If > @@ -1117,6 +1113,19 @@ static void wake_futex(struct futex_q *q) > */ > smp_wmb(); > q->lock_ptr = NULL; > + return p;
...
> @@ -1256,14 +1266,23 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset) > if (!(this->bitset & bitset)) > continue; > > - wake_futex(this); > + if (nr_wake == 1) > + waiter = __wake_futex(this); > + else > + wake_futex(this); > if (++ret >= nr_wake) > break; > } > } > > spin_unlock(&hb->lock); > + > out_put_key: > + if (waiter) { > + wake_up_state(waiter, TASK_NORMAL); > + put_task_struct(waiter); > + }
This should go before out_put_key, because none of the other code pathes which jump there can set waiter.
Thanks,
tglx
| |