Re: lockdep-related warning in kernel/sched/deadline.c::find_lock_later_rq()

[Luca Abeni <luca.abeni@unitn.it>]

Hi,

On 10/22/2015 07:35 AM, Wanpeng Li wrote:
[...]
>> Now, if I understand correctly the issue is that dl_task_timer() does:
>>     rq = task_rq_lock(p, &flags);
>> [...]
>>     if (has_pushable_dl_tasks(rq))
>>         push_dl_task(rq);
>> with task_rq_lock() that pins rq->lock and push_tl_task() that invokes
>> find_lock_later_rq() that unlocks rq->lock() while it is pinned.
>>
>> I am not sure about how to fix this issue: as a first try, I did
>> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>> index 142df26..5b1ba95 100644
>> --- a/kernel/sched/deadline.c
>> +++ b/kernel/sched/deadline.c
>> @@ -668,8 +668,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>>       * Queueing this task back might have overloaded rq, check if we need
>>       * to kick someone away.
>>       */
>> -    if (has_pushable_dl_tasks(rq))
>> +    if (has_pushable_dl_tasks(rq)) {
>> +        lockdep_unpin_lock(&rq->lock);
>>          push_dl_task(rq);
>> +        lockdep_pin_lock(&rq->lock);
>> +    }
>>  #endif
>>
>>  unlock:
>>
>> This removes the warning, but I am not sure if it is the correct fix (is it
>> valid to unpin rq->lock, here?).
>>
>> If someone can confirm that this is the correct approach, I'll test the patch a
>> little bit more and then I'll send a properly signed-off patch; otherwise, if
>> someone can suggest the correct approach I'll try it.
>
> wake_up_new_task()
>    -> __task_rq_lock()
>    -> task_woken()
>      -> push_dl/rt_tasks()
>        -> push_dl/rt_task()
>
> I think you also should consider the lockdep pin_lock in this path.
Well, I never triggered this warning for the task_woken() path, but now I
see it... Thanks!

If someone can confirm that unpinning before calling push/pull and pinning again
after the call is the correct thing to do, I'll send a proper patch taking into
account all the paths... But for the moment I am still not sure if unpinning
the lock here is ok.



			Thanks,
				Luca