From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752505AbcFNNOZ (ORCPT ); Tue, 14 Jun 2016 09:14:25 -0400 Received: from foss.arm.com ([217.140.101.70]:58645 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751362AbcFNNOY (ORCPT ); Tue, 14 Jun 2016 09:14:24 -0400 Date: Tue, 14 Jun 2016 14:14:24 +0100 From: Juri Lelli To: Peter Zijlstra Cc: mingo@kernel.org, tglx@linutronix.de, rostedt@goodmis.org, xlpang@redhat.com, linux-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com, jdesfossez@efficios.com, bristot@redhat.com Subject: Re: [RFC][PATCH 6/8] sched/rtmutex: Refactor rt_mutex_setprio() Message-ID: <20160614131424.GK5981@e106622-lin> References: <20160607195635.710022345@infradead.org> <20160607200215.990237037@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160607200215.990237037@infradead.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, still digesting this change, but I'll point out below why I think you are hitting a NULL ptr dereference (discussed on IRC). On 07/06/16 21:56, Peter Zijlstra wrote: > With the introduction of SCHED_DEADLINE the whole notion that priority > is a single number is gone, therefore the @prio argument to > rt_mutex_setprio() doesn't make sense anymore. > > So rework the code to pass a pi_task instead. > > Note this also fixes a problem with pi_top_task caching; previously we > would not set the pointer (call rt_mutex_update_top_task) if the > priority didn't change, this could lead to a stale pointer. > > As for the XXX, I think its fine to use pi_task->prio, because if it > differs from waiter->prio, a PI chain update is immenent. > > Signed-off-by: Peter Zijlstra (Intel) > --- > include/linux/sched/rt.h | 21 +------- > kernel/locking/rtmutex.c | 105 +++++++++++----------------------------- > kernel/locking/rtmutex_common.h | 1 > kernel/sched/core.c | 66 ++++++++++++++++++++----- > 4 files changed, 88 insertions(+), 105 deletions(-) > [...] > --- a/kernel/locking/rtmutex.c > +++ b/kernel/locking/rtmutex.c > @@ -256,61 +256,16 @@ rt_mutex_dequeue_pi(struct task_struct * > RB_CLEAR_NODE(&waiter->pi_tree_entry); > } > > -void rt_mutex_update_top_task(struct task_struct *p) > +static void rt_mutex_adjust_prio(struct task_struct *p) > { > - if (!task_has_pi_waiters(p)) { > - p->pi_top_task = NULL; > - return; > - } > + struct task_struct *pi_task = NULL; > > - p->pi_top_task = task_top_pi_waiter(p)->task; > -} > - > -/* > - * Calculate task priority from the waiter tree priority > - * > - * Return task->normal_prio when the waiter tree is empty or when > - * the waiter is not allowed to do priority boosting > - */ > -int rt_mutex_getprio(struct task_struct *task) > -{ > - if (likely(!task_has_pi_waiters(task))) > - return task->normal_prio; > + lockdep_assert_held(&p->pi_lock); > > - return min(task_top_pi_waiter(task)->prio, > - task->normal_prio); > -} > + if (!task_has_pi_waiters(p)) Shouldn't this be the other way around? if (task_has_pi_waiters(p)) pi_task = ... Best, - Juri