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

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

[Luca Abeni]

Hi all,

after fixing task migrations for SCHED_DEADLINE, I started to see some
lockdep-related warnings that look like this:
[  794.428081] WARNING: CPU: 1 PID: 0 at /home/luca/Src/GRUB/linux-reclaiming/kernel/locking/lockdep.c:3407 lock_release+0x3f4/0x440()
[  794.428439] releasing a pinned lock
[  794.428439] Modules linked in:
[  794.428439] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.3.0-rc5+ #32
[  794.428439] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS Bochs 01/01/2011
[  794.428439]  ffffffff81c946d0 ffff880007b03cc8 ffffffff8135abd2 ffff880007b03d10
[  794.428439]  ffff880007b03d00 ffffffff810546f1 ffff880007b159d8 ffffffff81096663
[  794.428439]  ffff88000754c558 0000000000000001 ffff88000754bd80 ffff880007b03d60
[  794.428439] Call Trace:
[  794.428439]  <IRQ>  [<ffffffff8135abd2>] dump_stack+0x4b/0x69
[  794.428439]  [<ffffffff810546f1>] warn_slowpath_common+0x81/0xc0
[  794.428439]  [<ffffffff81096663>] ? find_lock_later_rq+0x103/0x180
[  794.428439]  [<ffffffff81054777>] warn_slowpath_fmt+0x47/0x50
[  794.428439]  [<ffffffff8109feb4>] lock_release+0x3f4/0x440
[  794.428439]  [<ffffffff8194215a>] _raw_spin_unlock+0x1a/0x30
[  794.428439]  [<ffffffff81096663>] find_lock_later_rq+0x103/0x180
[  794.428439]  [<ffffffff81096741>] push_dl_task.part.33+0x61/0x190
[  794.428439]  [<ffffffff81096a94>] dl_task_timer+0x194/0x2e0
[  794.428439]  [<ffffffff81096900>] ? task_woken_dl+0x60/0x60
[  794.428439]  [<ffffffff810c3a87>] __hrtimer_run_queues+0x107/0x470
[  794.428439]  [<ffffffff810c3f82>] ? hrtimer_interrupt+0x82/0x1b0
[  794.428439]  [<ffffffff810c3fa6>] hrtimer_interrupt+0xa6/0x1b0
[  794.428439]  [<ffffffff81039570>] local_apic_timer_interrupt+0x30/0x60
[  794.428439]  [<ffffffff81039ee8>] smp_apic_timer_interrupt+0x38/0x50
[  794.428439]  [<ffffffff819436c4>] apic_timer_interrupt+0x84/0x90
[  794.428439]  <EOI>  [<ffffffff8100ea3b>] ? default_idle+0x1b/0x140
[  794.428439]  [<ffffffff8100ea39>] ? default_idle+0x19/0x140
[  794.428439]  [<ffffffff8100f25a>] arch_cpu_idle+0xa/0x10
[  794.428439]  [<ffffffff81097e45>] default_idle_call+0x25/0x40
[  794.428439]  [<ffffffff810981d0>] cpu_startup_entry+0x310/0x390
[  794.428439]  [<ffffffff810380c3>] start_secondary+0xf3/0x100
[  794.428439] ---[ end trace 1d35bf076299f814 ]---
(this happended on KVM, but I can trigger similar warnings on real hardware too).

This was not happening before my last patch, because push_dl_task() never migrated
any task, and always returned before calling find_lock_later_rq()...

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.



			Thanks,
				Luca

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

[Wanpeng Li]

On 10/21/15 9:20 PM, Luca Abeni wrote:
> Hi all,
>
> after fixing task migrations for SCHED_DEADLINE, I started to see some
> lockdep-related warnings that look like this:
> [  794.428081] WARNING: CPU: 1 PID: 0 at 
> /home/luca/Src/GRUB/linux-reclaiming/kernel/locking/lockdep.c:3407 
> lock_release+0x3f4/0x440()
> [  794.428439] releasing a pinned lock
> [  794.428439] Modules linked in:
> [  794.428439] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.3.0-rc5+ #32
> [  794.428439] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), 
> BIOS Bochs 01/01/2011
> [  794.428439]  ffffffff81c946d0 ffff880007b03cc8 ffffffff8135abd2 
> ffff880007b03d10
> [  794.428439]  ffff880007b03d00 ffffffff810546f1 ffff880007b159d8 
> ffffffff81096663
> [  794.428439]  ffff88000754c558 0000000000000001 ffff88000754bd80 
> ffff880007b03d60
> [  794.428439] Call Trace:
> [  794.428439]  <IRQ>  [<ffffffff8135abd2>] dump_stack+0x4b/0x69
> [  794.428439]  [<ffffffff810546f1>] warn_slowpath_common+0x81/0xc0
> [  794.428439]  [<ffffffff81096663>] ? find_lock_later_rq+0x103/0x180
> [  794.428439]  [<ffffffff81054777>] warn_slowpath_fmt+0x47/0x50
> [  794.428439]  [<ffffffff8109feb4>] lock_release+0x3f4/0x440
> [  794.428439]  [<ffffffff8194215a>] _raw_spin_unlock+0x1a/0x30
> [  794.428439]  [<ffffffff81096663>] find_lock_later_rq+0x103/0x180
> [  794.428439]  [<ffffffff81096741>] push_dl_task.part.33+0x61/0x190
> [  794.428439]  [<ffffffff81096a94>] dl_task_timer+0x194/0x2e0
> [  794.428439]  [<ffffffff81096900>] ? task_woken_dl+0x60/0x60
> [  794.428439]  [<ffffffff810c3a87>] __hrtimer_run_queues+0x107/0x470
> [  794.428439]  [<ffffffff810c3f82>] ? hrtimer_interrupt+0x82/0x1b0
> [  794.428439]  [<ffffffff810c3fa6>] hrtimer_interrupt+0xa6/0x1b0
> [  794.428439]  [<ffffffff81039570>] local_apic_timer_interrupt+0x30/0x60
> [  794.428439]  [<ffffffff81039ee8>] smp_apic_timer_interrupt+0x38/0x50
> [  794.428439]  [<ffffffff819436c4>] apic_timer_interrupt+0x84/0x90
> [  794.428439]  <EOI>  [<ffffffff8100ea3b>] ? default_idle+0x1b/0x140
> [  794.428439]  [<ffffffff8100ea39>] ? default_idle+0x19/0x140
> [  794.428439]  [<ffffffff8100f25a>] arch_cpu_idle+0xa/0x10
> [  794.428439]  [<ffffffff81097e45>] default_idle_call+0x25/0x40
> [  794.428439]  [<ffffffff810981d0>] cpu_startup_entry+0x310/0x390
> [  794.428439]  [<ffffffff810380c3>] start_secondary+0xf3/0x100
> [  794.428439] ---[ end trace 1d35bf076299f814 ]---
> (this happended on KVM, but I can trigger similar warnings on real 
> hardware too).
>
> This was not happening before my last patch, because push_dl_task() 
> never migrated
> any task, and always returned before calling find_lock_later_rq()...
>
> 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.

Regards,
Wanpeng Li

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

[Luca Abeni]

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

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

[Peter Zijlstra]

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:

[tip:sched/urgent] sched/core: Add missing lockdep_unpin() annotations

[tip-bot for Peter Zijlstra]

Commit-ID:  0aaafaabfcba8aa991913cd3280a5dbf7f111a2a
Gitweb:     http://git.kernel.org/tip/0aaafaabfcba8aa991913cd3280a5dbf7f111a2a
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 23 Oct 2015 11:50:08 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 23 Oct 2015 12:02:10 +0200

sched/core: Add missing lockdep_unpin() annotations

Luca and Wanpeng reported two missing annotations that led to
false lockdep complaints. Add the missing annotations.

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>
Cc: Juri Lelli <juri.lelli@arm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: cbce1a686700 ("sched,lockdep: Employ lock pinning")
Link: http://lkml.kernel.org/r/20151023095008.GY17308@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.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 5bd7d60..bcd214e 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2366,8 +2366,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 142df26..8b0a15e 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:

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

[Luca Abeni]

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: