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

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Oct 22, 2015 at 09:06:31AM +0200, Luca Abeni wrote:

> >>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.

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.

> >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: