Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1369218 > unrolled thread

[PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline tasks

Back to article view | Back to linux.kernel

Contents

  [PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline tasks Xunlei Pang <xlpang@redhat.com> - 2016-04-01 13:10 +0200
    Re: [PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline tasks Peter Zijlstra <peterz@infradead.org> - 2016-04-01 13:40 +0200
      Re: [PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline tasks Xunlei Pang <xpang@redhat.com> - 2016-04-01 14:30 +0200
        Re: [PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline tasks Peter Zijlstra <peterz@infradead.org> - 2016-04-01 15:20 +0200
          Re: [PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline tasks Xunlei Pang <xpang@redhat.com> - 2016-04-01 15:40 +0200
            Re: [PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline tasks Peter Zijlstra <peterz@infradead.org> - 2016-04-02 00:00 +0200
              Re: [PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline tasks Xunlei Pang <xpang@redhat.com> - 2016-04-02 12:30 +0200

#1369218 — [PATCH] sched/deadline/rtmutex: Fix a PI crash for deadline tasks

I found a kernel crash while playing with deadline PI rtmutex.

    BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
    IP: [<ffffffff810eeb8f>] rt_mutex_get_top_task+0x1f/0x30
    PGD 232a75067 PUD 230947067 PMD 0
    Oops: 0000 [#1] SMP
    CPU: 1 PID: 10994 Comm: a.out Not tainted

    Call Trace:
    [<ffffffff810cf8aa>] ? enqueue_task_dl+0x2a/0x320
    [<ffffffff810b658c>] enqueue_task+0x2c/0x80
    [<ffffffff810ba763>] activate_task+0x23/0x30
    [<ffffffff810d0ab5>] pull_dl_task+0x1d5/0x260
    [<ffffffff810d0be6>] pre_schedule_dl+0x16/0x20
    [<ffffffff8164e783>] __schedule+0xd3/0x900
    [<ffffffff8164efd9>] schedule+0x29/0x70
    [<ffffffff8165035b>] __rt_mutex_slowlock+0x4b/0xc0
    [<ffffffff81650501>] rt_mutex_slowlock+0xd1/0x190
    [<ffffffff810eeb33>] rt_mutex_timed_lock+0x53/0x60
    [<ffffffff810ecbfc>] futex_lock_pi.isra.18+0x28c/0x390
    [<ffffffff810cfa15>] ? enqueue_task_dl+0x195/0x320
    [<ffffffff810d0bac>] ? prio_changed_dl+0x6c/0x90
    [<ffffffff810ed8b0>] do_futex+0x190/0x5b0
    [<ffffffff810edd50>] SyS_futex+0x80/0x180
    [<ffffffff8165a089>] system_call_fastpath+0x16/0x1b
    RIP  [<ffffffff810eeb8f>] rt_mutex_get_top_task+0x1f/0x30

This is because rt_mutex_enqueue_pi() and rt_mutex_dequeue_pi()
are only protected by pi_lock when operating pi waiters, while
rt_mutex_get_top_task() will access them with rq lock held but
not holding pi_lock.

It's hard for rt_mutex_get_top_task() to hold pi_lock, so the
patch ensures rt_mutex_enqueue_pi() and rt_mutex_dequeue_pi()
lock rq when operating "pi_waiters" and "pi_waiters_leftmost".
We need this iff lock owner has the deadline priority.

Signed-off-by: Xunlei Pang <xlpang@redhat.com>
---
 include/linux/sched/deadline.h |  3 +++
 kernel/locking/rtmutex.c       | 18 ++++++++++++++++++
 kernel/sched/deadline.c        | 17 +++++++++++++++++
 3 files changed, 38 insertions(+)

diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
index 9089a2a..3083f6b 100644
--- a/include/linux/sched/deadline.h
+++ b/include/linux/sched/deadline.h
@@ -26,4 +26,7 @@ static inline bool dl_time_before(u64 a, u64 b)
 	return (s64)(a - b) < 0;
 }
 
+extern void *dl_pi_waiters_lock(struct task_struct *p);
+extern void dl_pi_waiters_unlock(void *lockdata);
+
 #endif /* _SCHED_DEADLINE_H */
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 3e74660..0fb247a 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -224,6 +224,7 @@ rt_mutex_enqueue_pi(struct task_struct *task, struct rt_mutex_waiter *waiter)
 	struct rb_node *parent = NULL;
 	struct rt_mutex_waiter *entry;
 	int leftmost = 1;
+	void *dl_lockdata = NULL;
 
 	while (*link) {
 		parent = *link;
@@ -236,24 +237,38 @@ rt_mutex_enqueue_pi(struct task_struct *task, struct rt_mutex_waiter *waiter)
 		}
 	}
 
+	if (dl_task(task))
+		dl_lockdata = dl_pi_waiters_lock(task);
+
 	if (leftmost)
 		task->pi_waiters_leftmost = &waiter->pi_tree_entry;
 
 	rb_link_node(&waiter->pi_tree_entry, parent, link);
 	rb_insert_color(&waiter->pi_tree_entry, &task->pi_waiters);
+
+	if (dl_lockdata)
+		dl_pi_waiters_unlock(dl_lockdata);
 }
 
 static void
 rt_mutex_dequeue_pi(struct task_struct *task, struct rt_mutex_waiter *waiter)
 {
+	void *dl_lockdata = NULL;
+
 	if (RB_EMPTY_NODE(&waiter->pi_tree_entry))
 		return;
 
+	if (dl_task(task))
+		dl_lockdata = dl_pi_waiters_lock(task);
+
 	if (task->pi_waiters_leftmost == &waiter->pi_tree_entry)
 		task->pi_waiters_leftmost = rb_next(&waiter->pi_tree_entry);
 
 	rb_erase(&waiter->pi_tree_entry, &task->pi_waiters);
 	RB_CLEAR_NODE(&waiter->pi_tree_entry);
+
+	if (dl_lockdata)
+		dl_pi_waiters_unlock(dl_lockdata);
 }
 
 /*
@@ -271,6 +286,9 @@ int rt_mutex_getprio(struct task_struct *task)
 		   task->normal_prio);
 }
 
+/*
+ * rq->lock of @task must be held.
+ */
 struct task_struct *rt_mutex_get_top_task(struct task_struct *task)
 {
 	if (likely(!task_has_pi_waiters(task)))
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index a3048fa..7b8aa93 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -926,6 +926,23 @@ static void dequeue_dl_entity(struct sched_dl_entity *dl_se)
 	__dequeue_dl_entity(dl_se);
 }
 
+/*
+ * dl_pi_waiters_lock()/dl_pi_waiters_unlock() are needed by
+ * rt_mutex_enqueue_pi() and rt_mutex_dequeue_pi() to protect
+ * PI waiters accessed by rt_mutex_get_top_task().
+ */
+void *dl_pi_waiters_lock(struct task_struct *p)
+{
+	lockdep_assert_held(&p->pi_lock);
+
+	return __task_rq_lock(p);
+}
+
+void dl_pi_waiters_unlock(void *lockdata)
+{
+	__task_rq_unlock((struct rq *)lockdata);
+}
+
 static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 {
 	struct task_struct *pi_task = rt_mutex_get_top_task(p);
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1369238

On Fri, Apr 01, 2016 at 07:00:18PM +0800, Xunlei Pang wrote:
> I found a kernel crash while playing with deadline PI rtmutex.
> 
>     BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
>     IP: [<ffffffff810eeb8f>] rt_mutex_get_top_task+0x1f/0x30
>     PGD 232a75067 PUD 230947067 PMD 0
>     Oops: 0000 [#1] SMP
>     CPU: 1 PID: 10994 Comm: a.out Not tainted
> 
>     Call Trace:
>     [<ffffffff810cf8aa>] ? enqueue_task_dl+0x2a/0x320
>     [<ffffffff810b658c>] enqueue_task+0x2c/0x80
>     [<ffffffff810ba763>] activate_task+0x23/0x30
>     [<ffffffff810d0ab5>] pull_dl_task+0x1d5/0x260
>     [<ffffffff810d0be6>] pre_schedule_dl+0x16/0x20
>     [<ffffffff8164e783>] __schedule+0xd3/0x900
>     [<ffffffff8164efd9>] schedule+0x29/0x70
>     [<ffffffff8165035b>] __rt_mutex_slowlock+0x4b/0xc0
>     [<ffffffff81650501>] rt_mutex_slowlock+0xd1/0x190
>     [<ffffffff810eeb33>] rt_mutex_timed_lock+0x53/0x60
>     [<ffffffff810ecbfc>] futex_lock_pi.isra.18+0x28c/0x390
>     [<ffffffff810cfa15>] ? enqueue_task_dl+0x195/0x320
>     [<ffffffff810d0bac>] ? prio_changed_dl+0x6c/0x90
>     [<ffffffff810ed8b0>] do_futex+0x190/0x5b0
>     [<ffffffff810edd50>] SyS_futex+0x80/0x180
>     [<ffffffff8165a089>] system_call_fastpath+0x16/0x1b
>     RIP  [<ffffffff810eeb8f>] rt_mutex_get_top_task+0x1f/0x30
> 
> This is because rt_mutex_enqueue_pi() and rt_mutex_dequeue_pi()
> are only protected by pi_lock when operating pi waiters, while
> rt_mutex_get_top_task() will access them with rq lock held but
> not holding pi_lock.
> 
> It's hard for rt_mutex_get_top_task() to hold pi_lock, so the
> patch ensures rt_mutex_enqueue_pi() and rt_mutex_dequeue_pi()
> lock rq when operating "pi_waiters" and "pi_waiters_leftmost".
> We need this iff lock owner has the deadline priority.

How is this deadline specific, those functions you modify are
deadline/rt agnostic.

[toc] | [prev] | [next] | [standalone]

#1369251

On 2016/04/01 at 19:38, Peter Zijlstra wrote:
> On Fri, Apr 01, 2016 at 07:00:18PM +0800, Xunlei Pang wrote:
>> I found a kernel crash while playing with deadline PI rtmutex.
>>
>>     BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
>>     IP: [<ffffffff810eeb8f>] rt_mutex_get_top_task+0x1f/0x30
>>     PGD 232a75067 PUD 230947067 PMD 0
>>     Oops: 0000 [#1] SMP
>>     CPU: 1 PID: 10994 Comm: a.out Not tainted
>>
>>     Call Trace:
>>     [<ffffffff810cf8aa>] ? enqueue_task_dl+0x2a/0x320
>>     [<ffffffff810b658c>] enqueue_task+0x2c/0x80
>>     [<ffffffff810ba763>] activate_task+0x23/0x30
>>     [<ffffffff810d0ab5>] pull_dl_task+0x1d5/0x260
>>     [<ffffffff810d0be6>] pre_schedule_dl+0x16/0x20
>>     [<ffffffff8164e783>] __schedule+0xd3/0x900
>>     [<ffffffff8164efd9>] schedule+0x29/0x70
>>     [<ffffffff8165035b>] __rt_mutex_slowlock+0x4b/0xc0
>>     [<ffffffff81650501>] rt_mutex_slowlock+0xd1/0x190
>>     [<ffffffff810eeb33>] rt_mutex_timed_lock+0x53/0x60
>>     [<ffffffff810ecbfc>] futex_lock_pi.isra.18+0x28c/0x390
>>     [<ffffffff810cfa15>] ? enqueue_task_dl+0x195/0x320
>>     [<ffffffff810d0bac>] ? prio_changed_dl+0x6c/0x90
>>     [<ffffffff810ed8b0>] do_futex+0x190/0x5b0
>>     [<ffffffff810edd50>] SyS_futex+0x80/0x180
>>     [<ffffffff8165a089>] system_call_fastpath+0x16/0x1b
>>     RIP  [<ffffffff810eeb8f>] rt_mutex_get_top_task+0x1f/0x30
>>
>> This is because rt_mutex_enqueue_pi() and rt_mutex_dequeue_pi()
>> are only protected by pi_lock when operating pi waiters, while
>> rt_mutex_get_top_task() will access them with rq lock held but
>> not holding pi_lock.
>>
>> It's hard for rt_mutex_get_top_task() to hold pi_lock, so the
>> patch ensures rt_mutex_enqueue_pi() and rt_mutex_dequeue_pi()
>> lock rq when operating "pi_waiters" and "pi_waiters_leftmost".
>> We need this iff lock owner has the deadline priority.
> How is this deadline specific, those functions you modify are
> deadline/rt agnostic.

I checked the code, currently only deadline accesses the pi_waiters/pi_waiters_leftmost
without pi_lock held via rt_mutex_get_top_task(), other cases all have pi_lock held.

So adding the condition.

Regards,
Xunlei

[toc] | [prev] | [next] | [standalone]

#1369302

On 1 April 2016 14:23:58 CEST, Xunlei Pang <xpang@redhat.com> wrote:

>>> We need this iff lock owner has the deadline priority.
>> How is this deadline specific, those functions you modify are
>> deadline/rt agnostic.
>
>I checked the code, currently only deadline accesses the
>pi_waiters/pi_waiters_leftmost
>without pi_lock held via rt_mutex_get_top_task(), other cases all have
>pi_lock held.
>
>So adding the condition.

How does that not suggest fixing the deadline code?
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1369325

On 2016/04/01 at 21:12, Peter Zijlstra wrote:
>
> On 1 April 2016 14:23:58 CEST, Xunlei Pang <xpang@redhat.com> wrote:
>
>>>> We need this iff lock owner has the deadline priority.
>>> How is this deadline specific, those functions you modify are
>>> deadline/rt agnostic.
>> I checked the code, currently only deadline accesses the
>> pi_waiters/pi_waiters_leftmost
>> without pi_lock held via rt_mutex_get_top_task(), other cases all have
>> pi_lock held.
>>
>> So adding the condition.
> How does that not suggest fixing the deadline code?

I did tried that at first, but found it very hard when processing
pull_dl_task(push_dl_task can crash as well) like:
   double_lock_balance(this_rq, src_rq);
   p = pick_earliest_pushable_dl_task(src_rq, this_cpu);
  /*  and for each @p, we must hold its pi_lock,
       doing this once rq is locked will cause deadlock. */

Ditto for enqueue_task_dl()->rt_mutex_get_top_task(), as rq is locked.

If we unlock rq first and then lock pi_lock, this may cause other problems
due to unlocking rq.

Any better ideas is welcome.

Regards,
Xunlei

[toc] | [prev] | [next] | [standalone]

#1369626

On Fri, Apr 01, 2016 at 09:34:24PM +0800, Xunlei Pang wrote:

> >> I checked the code, currently only deadline accesses the
> >> pi_waiters/pi_waiters_leftmost
> >> without pi_lock held via rt_mutex_get_top_task(), other cases all have
> >> pi_lock held.

> Any better ideas is welcome.

Something like the below _might_ work; but its late and I haven't looked
at the PI code in a while. This basically caches a pointer to the top
waiter task in the running task_struct, under pi_lock and rq->lock, and
therefore we can use it with only rq->lock held.

Since the task is blocked, and cannot unblock without taking itself from
the block chain -- which would cause rt_mutex_setprio() to set another
top waiter task, the lifetime rules should be good.

Having this top waiter pointer around might also be useful to further
implement bandwidth inheritance or such, but I've not thought about that
too much.

Lots of code deleted because we move the entire task->prio mapping muck
into the scheduler, because we now pass a pi_task, not a prio.

---
 include/linux/sched.h    |  1 +
 include/linux/sched/rt.h | 20 +-------------------
 kernel/locking/rtmutex.c | 45 +++++----------------------------------------
 kernel/sched/core.c      | 34 +++++++++++++++++++++++-----------
 kernel/sched/deadline.c  |  2 +-
 5 files changed, 31 insertions(+), 71 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 52c4847b05e2..30169f38cb24 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1615,6 +1615,7 @@ struct task_struct {
 
 	/* Protection of the PI data structures: */
 	raw_spinlock_t pi_lock;
+	struct task_struct *pi_task;
 
 	struct wake_q_node wake_q;
 
diff --git a/include/linux/sched/rt.h b/include/linux/sched/rt.h
index a30b172df6e1..0a8f071b16e3 100644
--- a/include/linux/sched/rt.h
+++ b/include/linux/sched/rt.h
@@ -16,31 +16,13 @@ static inline int rt_task(struct task_struct *p)
 }
 
 #ifdef CONFIG_RT_MUTEXES
-extern int rt_mutex_getprio(struct task_struct *p);
-extern void rt_mutex_setprio(struct task_struct *p, int prio);
-extern int rt_mutex_get_effective_prio(struct task_struct *task, int newprio);
-extern struct task_struct *rt_mutex_get_top_task(struct task_struct *task);
+extern void rt_mutex_setprio(struct task_struct *p, struct task_struct *pi_task);
 extern void rt_mutex_adjust_pi(struct task_struct *p);
 static inline bool tsk_is_pi_blocked(struct task_struct *tsk)
 {
 	return tsk->pi_blocked_on != NULL;
 }
 #else
-static inline int rt_mutex_getprio(struct task_struct *p)
-{
-	return p->normal_prio;
-}
-
-static inline int rt_mutex_get_effective_prio(struct task_struct *task,
-					      int newprio)
-{
-	return newprio;
-}
-
-static inline struct task_struct *rt_mutex_get_top_task(struct task_struct *task)
-{
-	return NULL;
-}
 # define rt_mutex_adjust_pi(p)		do { } while (0)
 static inline bool tsk_is_pi_blocked(struct task_struct *tsk)
 {
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 3e746607abe5..13b6b5922d3c 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -257,53 +257,18 @@ rt_mutex_dequeue_pi(struct task_struct *task, struct rt_mutex_waiter *waiter)
 }
 
 /*
- * 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;
-
-	return min(task_top_pi_waiter(task)->prio,
-		   task->normal_prio);
-}
-
-struct task_struct *rt_mutex_get_top_task(struct task_struct *task)
-{
-	if (likely(!task_has_pi_waiters(task)))
-		return NULL;
-
-	return task_top_pi_waiter(task)->task;
-}
-
-/*
- * Called by sched_setscheduler() to get the priority which will be
- * effective after the change.
- */
-int rt_mutex_get_effective_prio(struct task_struct *task, int newprio)
-{
-	if (!task_has_pi_waiters(task))
-		return newprio;
-
-	if (task_top_pi_waiter(task)->task->prio <= newprio)
-		return task_top_pi_waiter(task)->task->prio;
-	return newprio;
-}
-
-/*
  * Adjust the priority of a task, after its pi_waiters got modified.
  *
  * This can be both boosting and unboosting. task->pi_lock must be held.
  */
 static void __rt_mutex_adjust_prio(struct task_struct *task)
 {
-	int prio = rt_mutex_getprio(task);
+	struct task_struct *pi_task = task;
+
+	if (unlikely(task_has_pi_waiters(task)))
+		pi_task = task_top_pi_waiter(task)->task;
 
-	if (task->prio != prio || dl_prio(prio))
-		rt_mutex_setprio(task, prio);
+	rt_mutex_setprio(task, pi_task);
 }
 
 /*
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 8b489fcac37b..7d7e3a0eaeb0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3392,7 +3392,7 @@ EXPORT_SYMBOL(default_wake_function);
 /*
  * rt_mutex_setprio - set the current priority of a task
  * @p: task
- * @prio: prio value (kernel-internal form)
+ * @pi_task: top waiter, donating state
  *
  * This function changes the 'effective' priority of a task. It does
  * not touch ->normal_prio like __setscheduler().
@@ -3400,13 +3400,20 @@ EXPORT_SYMBOL(default_wake_function);
  * Used by the rt_mutex code to implement priority inheritance
  * logic. Call site only calls if the priority of the task changed.
  */
-void rt_mutex_setprio(struct task_struct *p, int prio)
+void rt_mutex_setprio(struct task_struct *p, struct task_struct *pi_task)
 {
-	int oldprio, queued, running, queue_flag = DEQUEUE_SAVE | DEQUEUE_MOVE;
-	struct rq *rq;
+	int prio, oldprio, queued, running, queue_flag = DEQUEUE_SAVE | DEQUEUE_MOVE;
 	const struct sched_class *prev_class;
+	struct rq *rq;
 
-	BUG_ON(prio > MAX_PRIO);
+	/*
+	 * For FIFO/RR we simply donate prio; for DL things are
+	 * more interesting.
+	 */
+	/* XXX used to be waiter->prio, not waiter->task->prio */
+	prio = min(pi_task->prio, p->normal_prio);
+	if (p->prio == prio && !dl_prio(prio))
+		return;
 
 	rq = __task_rq_lock(p);
 
@@ -3442,6 +3449,10 @@ void rt_mutex_setprio(struct task_struct *p, int prio)
 	if (running)
 		put_prev_task(rq, p);
 
+	if (pi_task == p)
+		pi_task = NULL;
+	p->pi_task = pi_task;
+
 	/*
 	 * Boosting condition are:
 	 * 1. -rt task is running and holds mutex A
@@ -3452,7 +3463,6 @@ void rt_mutex_setprio(struct task_struct *p, int prio)
 	 *          running task
 	 */
 	if (dl_prio(prio)) {
-		struct task_struct *pi_task = rt_mutex_get_top_task(p);
 		if (!dl_prio(p->normal_prio) ||
 		    (pi_task && dl_entity_preempt(&pi_task->dl, &p->dl))) {
 			p->dl.dl_boosted = 1;
@@ -3727,10 +3737,9 @@ static void __setscheduler(struct rq *rq, struct task_struct *p,
 	 * Keep a potential priority boosting if called from
 	 * sched_setscheduler().
 	 */
-	if (keep_boost)
-		p->prio = rt_mutex_get_effective_prio(p, normal_prio(p));
-	else
-		p->prio = normal_prio(p);
+	p->prio = normal_prio(p);
+	if (keep_boost && p->pi_task)
+		p->prio = min(p->prio, p->pi_task->prio);
 
 	if (dl_prio(p->prio))
 		p->sched_class = &dl_sched_class;
@@ -4017,7 +4026,10 @@ static int __sched_setscheduler(struct task_struct *p,
 		 * the runqueue. This will be done when the task deboost
 		 * itself.
 		 */
-		new_effective_prio = rt_mutex_get_effective_prio(p, newprio);
+		new_effective_prio = newprio;
+		if (p->pi_task)
+			new_effective_prio = min(new_effective_prio, p->pi_task->prio);
+
 		if (new_effective_prio == oldprio)
 			queue_flags &= ~DEQUEUE_MOVE;
 	}
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index affd97ec9f65..6c5aa4612eb6 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -932,7 +932,7 @@ static void dequeue_dl_entity(struct sched_dl_entity *dl_se)
 
 static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 {
-	struct task_struct *pi_task = rt_mutex_get_top_task(p);
+	struct task_struct *pi_task = p->pi_task;
 	struct sched_dl_entity *pi_se = &p->dl;
 
 	/*

[toc] | [prev] | [next] | [standalone]

#1369914

On 2016/04/02 at 05:51, Peter Zijlstra wrote:
> On Fri, Apr 01, 2016 at 09:34:24PM +0800, Xunlei Pang wrote:
>
>>>> I checked the code, currently only deadline accesses the
>>>> pi_waiters/pi_waiters_leftmost
>>>> without pi_lock held via rt_mutex_get_top_task(), other cases all have
>>>> pi_lock held.
>> Any better ideas is welcome.
> Something like the below _might_ work; but its late and I haven't looked
> at the PI code in a while. This basically caches a pointer to the top
> waiter task in the running task_struct, under pi_lock and rq->lock, and
> therefore we can use it with only rq->lock held.
>
> Since the task is blocked, and cannot unblock without taking itself from
> the block chain -- which would cause rt_mutex_setprio() to set another
> top waiter task, the lifetime rules should be good.
>
> Having this top waiter pointer around might also be useful to further
> implement bandwidth inheritance or such, but I've not thought about that
> too much.
>
> Lots of code deleted because we move the entire task->prio mapping muck
> into the scheduler, because we now pass a pi_task, not a prio.

I'll try to understand your patch, and do a test later. Thanks!

Regards,
Xunlei

> ---
>  include/linux/sched.h    |  1 +
>  include/linux/sched/rt.h | 20 +-------------------
>  kernel/locking/rtmutex.c | 45 +++++----------------------------------------
>  kernel/sched/core.c      | 34 +++++++++++++++++++++++-----------
>  kernel/sched/deadline.c  |  2 +-
>  5 files changed, 31 insertions(+), 71 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 52c4847b05e2..30169f38cb24 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1615,6 +1615,7 @@ struct task_struct {
>  
>  	/* Protection of the PI data structures: */
>  	raw_spinlock_t pi_lock;
> +	struct task_struct *pi_task;
>  
>  	struct wake_q_node wake_q;
>  
> diff --git a/include/linux/sched/rt.h b/include/linux/sched/rt.h
> index a30b172df6e1..0a8f071b16e3 100644
> --- a/include/linux/sched/rt.h
> +++ b/include/linux/sched/rt.h
> @@ -16,31 +16,13 @@ static inline int rt_task(struct task_struct *p)
>  }
>  
>  #ifdef CONFIG_RT_MUTEXES
> -extern int rt_mutex_getprio(struct task_struct *p);
> -extern void rt_mutex_setprio(struct task_struct *p, int prio);
> -extern int rt_mutex_get_effective_prio(struct task_struct *task, int newprio);
> -extern struct task_struct *rt_mutex_get_top_task(struct task_struct *task);
> +extern void rt_mutex_setprio(struct task_struct *p, struct task_struct *pi_task);
>  extern void rt_mutex_adjust_pi(struct task_struct *p);
>  static inline bool tsk_is_pi_blocked(struct task_struct *tsk)
>  {
>  	return tsk->pi_blocked_on != NULL;
>  }
>  #else
> -static inline int rt_mutex_getprio(struct task_struct *p)
> -{
> -	return p->normal_prio;
> -}
> -
> -static inline int rt_mutex_get_effective_prio(struct task_struct *task,
> -					      int newprio)
> -{
> -	return newprio;
> -}
> -
> -static inline struct task_struct *rt_mutex_get_top_task(struct task_struct *task)
> -{
> -	return NULL;
> -}
>  # define rt_mutex_adjust_pi(p)		do { } while (0)
>  static inline bool tsk_is_pi_blocked(struct task_struct *tsk)
>  {
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index 3e746607abe5..13b6b5922d3c 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -257,53 +257,18 @@ rt_mutex_dequeue_pi(struct task_struct *task, struct rt_mutex_waiter *waiter)
>  }
>  
>  /*
> - * 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;
> -
> -	return min(task_top_pi_waiter(task)->prio,
> -		   task->normal_prio);
> -}
> -
> -struct task_struct *rt_mutex_get_top_task(struct task_struct *task)
> -{
> -	if (likely(!task_has_pi_waiters(task)))
> -		return NULL;
> -
> -	return task_top_pi_waiter(task)->task;
> -}
> -
> -/*
> - * Called by sched_setscheduler() to get the priority which will be
> - * effective after the change.
> - */
> -int rt_mutex_get_effective_prio(struct task_struct *task, int newprio)
> -{
> -	if (!task_has_pi_waiters(task))
> -		return newprio;
> -
> -	if (task_top_pi_waiter(task)->task->prio <= newprio)
> -		return task_top_pi_waiter(task)->task->prio;
> -	return newprio;
> -}
> -
> -/*
>   * Adjust the priority of a task, after its pi_waiters got modified.
>   *
>   * This can be both boosting and unboosting. task->pi_lock must be held.
>   */
>  static void __rt_mutex_adjust_prio(struct task_struct *task)
>  {
> -	int prio = rt_mutex_getprio(task);
> +	struct task_struct *pi_task = task;
> +
> +	if (unlikely(task_has_pi_waiters(task)))
> +		pi_task = task_top_pi_waiter(task)->task;
>  
> -	if (task->prio != prio || dl_prio(prio))
> -		rt_mutex_setprio(task, prio);
> +	rt_mutex_setprio(task, pi_task);
>  }
>  
>  /*
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 8b489fcac37b..7d7e3a0eaeb0 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3392,7 +3392,7 @@ EXPORT_SYMBOL(default_wake_function);
>  /*
>   * rt_mutex_setprio - set the current priority of a task
>   * @p: task
> - * @prio: prio value (kernel-internal form)
> + * @pi_task: top waiter, donating state
>   *
>   * This function changes the 'effective' priority of a task. It does
>   * not touch ->normal_prio like __setscheduler().
> @@ -3400,13 +3400,20 @@ EXPORT_SYMBOL(default_wake_function);
>   * Used by the rt_mutex code to implement priority inheritance
>   * logic. Call site only calls if the priority of the task changed.
>   */
> -void rt_mutex_setprio(struct task_struct *p, int prio)
> +void rt_mutex_setprio(struct task_struct *p, struct task_struct *pi_task)
>  {
> -	int oldprio, queued, running, queue_flag = DEQUEUE_SAVE | DEQUEUE_MOVE;
> -	struct rq *rq;
> +	int prio, oldprio, queued, running, queue_flag = DEQUEUE_SAVE | DEQUEUE_MOVE;
>  	const struct sched_class *prev_class;
> +	struct rq *rq;
>  
> -	BUG_ON(prio > MAX_PRIO);
> +	/*
> +	 * For FIFO/RR we simply donate prio; for DL things are
> +	 * more interesting.
> +	 */
> +	/* XXX used to be waiter->prio, not waiter->task->prio */
> +	prio = min(pi_task->prio, p->normal_prio);
> +	if (p->prio == prio && !dl_prio(prio))
> +		return;
>  
>  	rq = __task_rq_lock(p);
>  
> @@ -3442,6 +3449,10 @@ void rt_mutex_setprio(struct task_struct *p, int prio)
>  	if (running)
>  		put_prev_task(rq, p);
>  
> +	if (pi_task == p)
> +		pi_task = NULL;
> +	p->pi_task = pi_task;
> +
>  	/*
>  	 * Boosting condition are:
>  	 * 1. -rt task is running and holds mutex A
> @@ -3452,7 +3463,6 @@ void rt_mutex_setprio(struct task_struct *p, int prio)
>  	 *          running task
>  	 */
>  	if (dl_prio(prio)) {
> -		struct task_struct *pi_task = rt_mutex_get_top_task(p);
>  		if (!dl_prio(p->normal_prio) ||
>  		    (pi_task && dl_entity_preempt(&pi_task->dl, &p->dl))) {
>  			p->dl.dl_boosted = 1;
> @@ -3727,10 +3737,9 @@ static void __setscheduler(struct rq *rq, struct task_struct *p,
>  	 * Keep a potential priority boosting if called from
>  	 * sched_setscheduler().
>  	 */
> -	if (keep_boost)
> -		p->prio = rt_mutex_get_effective_prio(p, normal_prio(p));
> -	else
> -		p->prio = normal_prio(p);
> +	p->prio = normal_prio(p);
> +	if (keep_boost && p->pi_task)
> +		p->prio = min(p->prio, p->pi_task->prio);
>  
>  	if (dl_prio(p->prio))
>  		p->sched_class = &dl_sched_class;
> @@ -4017,7 +4026,10 @@ static int __sched_setscheduler(struct task_struct *p,
>  		 * the runqueue. This will be done when the task deboost
>  		 * itself.
>  		 */
> -		new_effective_prio = rt_mutex_get_effective_prio(p, newprio);
> +		new_effective_prio = newprio;
> +		if (p->pi_task)
> +			new_effective_prio = min(new_effective_prio, p->pi_task->prio);
> +
>  		if (new_effective_prio == oldprio)
>  			queue_flags &= ~DEQUEUE_MOVE;
>  	}
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index affd97ec9f65..6c5aa4612eb6 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -932,7 +932,7 @@ static void dequeue_dl_entity(struct sched_dl_entity *dl_se)
>  
>  static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
>  {
> -	struct task_struct *pi_task = rt_mutex_get_top_task(p);
> +	struct task_struct *pi_task = p->pi_task;
>  	struct sched_dl_entity *pi_se = &p->dl;
>  
>  	/*

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web