Hi Peter, On Fri, 23 Oct 2015 11:50:08 +0200 Peter Zijlstra <pet...@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.ab...@unitn.it> > Reported-by: Wanpeng Li <wanpeng...@hotmail.com> > Signed-off-by: Peter Zijlstra (Intel) <pet...@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: -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/