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

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

Hi Peter,

On Fri, 23 Oct 2015 11:50:08 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

[...]
> > >>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.
> 
> Yes, its fine, although I would like a little comment with each unpin
> to explain _why_ its fine.
> 
> So here its fine because nothing relies on rq->lock being held after
> push_dl_task(), as the next thing we do is drop it anyway.
Thanks for explaining (now I see why it makes sense :), and thanks for
the patch!
I am using it locally, and everything is working fine.


			Thanks,
				Luca


> 
> > >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.
> 
> Durr, clearly I overlooked both these when I did that. Sorry about
> that.
> 
> So how about:
> 
> ---
> Subject: sched: Add missing lockdep_unpin annotations
> 
> Luca and Wanpeng reported two missing annotations that led to false
> lockdep complaints. Add the missing annotations.
> 
> Fixes: cbce1a686700 ("sched,lockdep: Employ lock pinning")
> Reported-by: Luca Abeni <luca.abeni@unitn.it>
> Reported-by: Wanpeng Li <wanpeng.li@hotmail.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/sched/core.c     | 9 ++++++++-
>  kernel/sched/deadline.c | 9 ++++++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 1764a0f2a75b..81a74b76346c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2362,8 +2362,15 @@ void wake_up_new_task(struct task_struct *p)
>  	trace_sched_wakeup_new(p);
>  	check_preempt_curr(rq, p, WF_FORK);
>  #ifdef CONFIG_SMP
> -	if (p->sched_class->task_woken)
> +	if (p->sched_class->task_woken) {
> +		/*
> +		 * Nothing relies on rq->lock after this, so its
> fine to
> +		 * drop it.
> +		 */
> +		lockdep_unpin_lock(&rq->lock);
>  		p->sched_class->task_woken(rq, p);
> +		lockdep_pin_lock(&rq->lock);
> +	}
>  #endif
>  	task_rq_unlock(rq, p, &flags);
>  }
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fc8f01083527..cfdff233099b 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -668,8 +668,15 @@ 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)) {
> +		/*
> +		 * Nothing relies on rq->lock after this, so its
> safe to drop
> +		 * rq->lock.
> +		 */
> +		lockdep_unpin_lock(&rq->lock);
>  		push_dl_task(rq);
> +		lockdep_pin_lock(&rq->lock);
> +	}
>  #endif
>  
>  unlock: