From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752250AbbJWUKZ (ORCPT ); Fri, 23 Oct 2015 16:10:25 -0400 Received: from mail-wi0-f169.google.com ([209.85.212.169]:33199 "EHLO mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751575AbbJWUKX (ORCPT ); Fri, 23 Oct 2015 16:10:23 -0400 Date: Fri, 23 Oct 2015 22:10:16 +0200 From: Luca Abeni To: Peter Zijlstra Cc: Wanpeng Li , linux-kernel@vger.kernel.org, Ingo Molnar , Juri Lelli Subject: Re: lockdep-related warning in kernel/sched/deadline.c::find_lock_later_rq() Message-ID: <20151023221016.223e87e3@luca-1225C> In-Reply-To: <20151023095008.GY17308@twins.programming.kicks-ass.net> References: <56279107.3010602@unitn.it> <56288AF7.20602@unitn.it> <20151023095008.GY17308@twins.programming.kicks-ass.net> X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.10; i686-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Peter, On Fri, 23 Oct 2015 11:50:08 +0200 Peter Zijlstra 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 > Reported-by: Wanpeng Li > Signed-off-by: Peter Zijlstra (Intel) > --- > 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: