On Tue, Apr 05, 2016 at 04:38:12PM +0800, Xunlei Pang wrote: > On 2016/04/02 at 05:51, Peter Zijlstra wrote: > > On Fri, Apr 01, 2016 at 09:34:24PM +0800, Xunlei Pang wrote: > > > >>>> I checked the code, currently only deadline accesses the > >>>> pi_waiters/pi_waiters_leftmost > >>>> without pi_lock held via rt_mutex_get_top_task(), other cases all have > >>>> pi_lock held. > >> Any better ideas is welcome. > > Something like the below _might_ work; but its late and I haven't looked > > at the PI code in a while. This basically caches a pointer to the top > > waiter task in the running task_struct, under pi_lock and rq->lock, and > > therefore we can use it with only rq->lock held. > > > > Since the task is blocked, and cannot unblock without taking itself from > > the block chain -- which would cause rt_mutex_setprio() to set another > > top waiter task, the lifetime rules should be good. > > In rt_mutex_slowunlock(), we release pi_lock and and wait_lock first, then > wake up the top waiter, then call rt_mutex_adjust_prio(), so there is a small > window without any lock or irq disabled between the top waiter wake up > and rt_mutex_adjust_prio(), which can cause problems.
That is rt_mutex_fastunlock()'s: bool deboost = slowfs(lock, &wake_q); /* -> rt_mutex_slowunlock() */ wake_up_q(&wake_q); if (deboost) rt_mutex_adjust_prio(current); (and the IRQ enabled is irrelevant, SMP can race regardless) > For example, before calling rt_mutex_adjust_prio() to adjust the cached > pointer, > if current is preempted and the waken top waiter exited, after that, the task > is > back, and it may enter enqueue_task_dl() before entering > rt_mutex_adjust_prio(), > where the cached pointer is updated, so it will access a stale cached pointer. Hmm, so I would argue that that is a bug in any case. Its an effective priority 'leak', we should deboost before letting the booster run again. But it looks like a simple fix, simply call wake_up_q() after the deboost. The wake_q has a reference on the task so it cannot go away, which ensures any dereferences from within the DL code must still be valid. Or did I miss something (again) ? :-) --- kernel/locking/rtmutex.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c index 3e746607abe5..36eb232bd29f 100644 --- a/kernel/locking/rtmutex.c +++ b/kernel/locking/rtmutex.c @@ -1390,11 +1390,11 @@ rt_mutex_fastunlock(struct rt_mutex *lock, } else { bool deboost = slowfn(lock, &wake_q); - wake_up_q(&wake_q); - /* Undo pi boosting if necessary: */ if (deboost) rt_mutex_adjust_prio(current); + + wake_up_q(&wake_q); } }