lkml.org 
[lkml]   [2012]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: lockdep false positive in double_lock_balance()?
    From
    Date
    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;
    }


    \
     
     \ /
      Last update: 2012-05-17 21:41    [W:0.029 / U:0.612 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site