Messages in this thread Patch in this message |  | | | Subject | Re: lockdep false positive in double_lock_balance()? | | From | Peter Zijlstra <> | | Date | Thu, 17 May 2012 21:19:46 +0200 |
| |
On Wed, 2012-05-16 at 13:00 -0700, Roland Dreier wrote: > Hi scheduler hackers, > > I'm very occasionally seeing the lockdep warning below on our boxes > running 2.6.39 (PREEMPT=n, so "unfair" _double_lock_balance()). I > think I see the explanation, and it's probably not even worth fixing: > > On the unlock side, we have:
<snip>
> while on the lock side we have:
<snip>
> So it seems we have the following (purely lockdep-related) race: > > unlock: lock: > > if (unlikely(!raw_spin_trylock(&busiest->lock))) { //fail to lock > > raw_spin_unlock(&busiest->lock); > > if (busiest < this_rq) { //not true > } else > raw_spin_lock_nested(&busiest->lock, > SINGLE_DEPTH_NESTING); > > lock_set_subclass(&this_rq->lock.dep_map, 0, _RET_IP_); //too late > > where we end up trying to take a second lock with SINGLE_DEPTH_NESTING > before we've promoted our first lock to subclass 0.
*phew* you actually made me think there ;-)
Anyway, it all sounds very plausible, which is what threw me, but its wrong :-)
The race you describe exists, except that's not how lockdep works. Both cpu's would have a different task (one would hope to presume) and the held lock stack is per task. So even if busiest_rq on cpu1 (lock case) is the same lock as this_rq on cpu0 (unlock case), they're in different stacks with different states.
> So does this make sense?
Almost :-)
> Here's the actual lockdep warning:
> [89945.640512] [<ffffffff8103fa1a>] double_lock_balance+0x5a/0x90 > [89945.640568] [<ffffffff8104c546>] push_rt_task+0xc6/0x290
this is the clue.. if you look at that code you'll find the double_lock_balance() in question is the one in find_lock_lowest_rq() [yay for inlining].
Now find_lock_lowest_rq() has a bug.. it fails to use double_unlock_balance() in one exit path, if this results in a retry in push_rt_task() we'll call double_lock_balance() again, at which point we'll run into said issue.
Presumably this is all rather rare..
Something like this should fix it I think..
--- kernel/sched/rt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c index c5565c3..b649108 100644 --- a/kernel/sched/rt.c +++ b/kernel/sched/rt.c @@ -1556,7 +1556,7 @@ static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq) task_running(rq, task) || !task->on_rq)) { - raw_spin_unlock(&lowest_rq->lock); + double_unlock_balance(rq, lowest_rq); lowest_rq = NULL; break; }
|  |