Messages in this thread | | | Date | Tue, 14 Jun 2016 14:14:24 +0100 | From | Juri Lelli <> | Subject | Re: [RFC][PATCH 6/8] sched/rtmutex: Refactor rt_mutex_setprio() |
| |
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) <peterz@infradead.org> > --- > 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
| |