sched/rt: Fix rq's cpupri leak while enqueue/dequeue child RT entities

PATCH In review Kirill Tkhai 3 participants 7 messages Nov 27, 2013 – Dec 18, 2013

Subsystems (2)

Stable branch Scheduler

lore

Share & tools

lore

Kirill Tkhai

author

2013-11-27 15:59 UTC #0 lore raw

PATCH 1/1 sched/rt: Fix rq's cpupri leak while enqueue/dequeue child RT entities

This patch touches RT group scheduling case.

Functions inc_rt_prio_smp() and dec_rt_prio_smp() change (global) rq's priority, while rt_rq passed to them may be not the top-level rt_rq. This is wrong, because changing of priority on a child level does not guarantee that the priority is the highest all over the rq. So, this leak makes RT balancing unusable.

The short example: the task having the highest priority among all rq's RT tasks (no one other task has the same priority) are waking on a throttle rt_rq. The rq's cpupri is set to the task's priority equivalent, but real rq->rt.highest_prio.curr is less.

The patch below fixes the problem.

It looks like all version have this bug, so I CC'ed stable mailing list.

Signed-off-by Kirill Tkhai <[email protected]>

CC Ingo Molnar <[email protected]>

CC Peter Zijlstra <[email protected]>

CC Steven Rostedt <[email protected]>

CC [email protected]

---
 kernel/sched/rt.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

kernel/sched/rt.c +14 -0

index 7d57275..1c40655 100644
@@ -901,6 +901,13 @@ inc_rt_prio_smp(struct rt_rq *rt_rq, int prio, int prev_prio)
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
+#ifdef CONFIG_RT_GROUP_SCHED
+	/*
+	 * Change rq's cpupri only if rt_rq is the top queue.
+	 */
+	if (&rq->rt != rt_rq)
+		return;
+#endif
 	if (rq->online && prio < prev_prio)
 		cpupri_set(&rq->rd->cpupri, rq->cpu, prio);
 }

@@ -910,6 +917,13 @@ dec_rt_prio_smp(struct rt_rq *rt_rq, int prio, int prev_prio)
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
+#ifdef CONFIG_RT_GROUP_SCHED
+	/*
+	 * Change rq's cpupri only if rt_rq is the top queue.
+	 */
+	if (&rq->rt != rt_rq)
+		return;
+#endif
 	if (rq->online && rt_rq->highest_prio.curr != prev_prio)
 		cpupri_set(&rq->rd->cpupri, rq->cpu, rt_rq->highest_prio.curr);
 }

Kirill Tkhai

author

2013-12-12 10:30 UTC #1 lore raw

Ping!

27.11.2013, 19:59, "Kirill Tkhai" <[email protected]>:

Quoted 56 lines

This patch touches RT group scheduling case.
Functions inc_rt_prio_smp() and dec_rt_prio_smp() change (global) rq's priority, while rt_rq passed to them may be not the top-level rt_rq. This is wrong, because changing of priority on a child level does not guarantee that the priority is the highest all over the rq. So, this leak makes RT balancing unusable.
The short example: the task having the highest priority among all rq's RT tasks (no one other task has the same priority) are waking on a throttle rt_rq. The rq's cpupri is set to the task's priority equivalent, but real rq->rt.highest_prio.curr is less.
The patch below fixes the problem.
It looks like all version have this bug, so I CC'ed stable mailing list.
Signed-off-by Kirill Tkhai <[email protected]>
CC Ingo Molnar <[email protected]>
CC Peter Zijlstra <[email protected]>
CC Steven Rostedt <[email protected]>
CC [email protected]
---
kernel/sched/rt.c | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
kernel/sched/rt.c +14 -0
index 7d57275..1c40655 100644
@@ -901,6 +901,13 @@ inc_rt_prio_smp(struct rt_rq *rt_rq, int prio, int prev_prio)
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
+#ifdef CONFIG_RT_GROUP_SCHED
+	/*
+	 * Change rq's cpupri only if rt_rq is the top queue.
+	 */
+	if (&rq->rt != rt_rq)
+		return;
+#endif
 	if (rq->online && prio < prev_prio)
 		cpupri_set(&rq->rd->cpupri, rq->cpu, prio);
 }
@@ -910,6 +917,13 @@ dec_rt_prio_smp(struct rt_rq *rt_rq, int prio, int prev_prio)
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
+#ifdef CONFIG_RT_GROUP_SCHED
+	/*
+	 * Change rq's cpupri only if rt_rq is the top queue.
+	 */
+	if (&rq->rt != rt_rq)
+		return;
+#endif
 	if (rq->online && rt_rq->highest_prio.curr != prev_prio)
 		cpupri_set(&rq->rd->cpupri, rq->cpu, rt_rq->highest_prio.curr);
 }

Peter Zijlstra

maintainer

2013-12-13 15:42 UTC #2 lore raw

In reply to Kirill Tkhai

Quoted 7 lines · Kirill Tkhai

This patch touches RT group scheduling case.
Functions inc_rt_prio_smp() and dec_rt_prio_smp() change (global) rq's priority, while rt_rq passed to them may be not the top-level rt_rq. This is wrong, because changing of priority on a child level does not guarantee that the priority is the highest all over the rq. So, this leak makes RT balancing unusable.

Quoted 8 lines

The short example: the task having the highest priority among all rq's RT tasks (no one other task has the same priority) are waking on a throttle rt_rq. The rq's cpupri is set to the task's priority equivalent, but real rq->rt.highest_prio.curr is less.
The patch below fixes the problem.
It looks like all version have this bug, so I CC'ed stable mailing list.

Yeah, I think this is right.

cpupri stuff should indeed only be changed for the top level group.

Kirill Tkhai

author

2013-12-17 12:02 UTC #3 lore raw

13.12.2013, 19:42, "Peter Zijlstra" <[email protected]>:

Quoted 14 lines · Kirill Tkhai

Quoted 13 lines

О©╫This patch touches RT group scheduling case.

О©╫Functions inc_rt_prio_smp() and dec_rt_prio_smp() change (global) rq's priority,
О©╫while rt_rq passed to them may be not the top-level rt_rq. This is wrong, because
О©╫changing of priority on a child level does not guarantee that the priority is
О©╫the highest all over the rq. So, this leak makes RT balancing unusable.

О©╫The short example: the task having the highest priority among all rq's RT tasks
О©╫(no one other task has the same priority) are waking on a throttle rt_rq.
О©╫The rq's cpupri is set to the task's priority equivalent, but real
О©╫rq->rt.highest_prio.curr is less.

Quoted 7 lines

Quoted 3 lines
О©╫The patch below fixes the problem.
О©╫It looks like all version have this bug, so I CC'ed stable mailing list.
Yeah, I think this is right.
cpupri stuff should indeed only be changed for the top level group.

Ingo, are you going to apply this patch? Or will you give any comments?

Peter Zijlstra

maintainer

2013-12-17 12:46 UTC #4 lore raw

Quoted 19 lines · Kirill Tkhai

13.12.2013, 19:42, "Peter Zijlstra" <[email protected]>:
Quoted 16 lines · Kirill Tkhai
Quoted 15 lines
ï¿½This patch touches RT group scheduling case.
ï¿½Functions inc_rt_prio_smp() and dec_rt_prio_smp() change (global) rq's priority, ï¿½while rt_rq passed to them may be not the top-level rt_rq. This is wrong, because ï¿½changing of priority on a child level does not guarantee that the priority is ï¿½the highest all over the rq. So, this leak makes RT balancing unusable.
ï¿½The short example: the task having the highest priority among all rq's RT tasks ï¿½(no one other task has the same priority) are waking on a throttle rt_rq. ï¿½The rq's cpupri is set to the task's priority equivalent, but real ï¿½rq->rt.highest_prio.curr is less.
ï¿½The patch below fixes the problem.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
ï¿½It looks like all version have this bug, so I CC'ed stable mailing list.
Yeah, I think this is right.
cpupri stuff should indeed only be changed for the top level group.
Ingo, are you going to apply this patch? Or will you give any comments?

I queued it, Ingo should get it through me somewhere today if all things go well.

Thanks

Kirill Tkhai

author

2013-12-17 13:08 UTC #5 lore raw

17.12.2013, 16:47, "Peter Zijlstra" <[email protected]>:

Quoted 21 lines · Kirill Tkhai

Quoted 20 lines

О©╫13.12.2013, 19:42, "Peter Zijlstra" <[email protected]>:
> О©╫On Wed, Nov 27, 2013 at 07:59:13PM +0400, Kirill Tkhai wrote:
>> О©╫О©╫This patch touches RT group scheduling case.

Quoted 16 lines

Quoted 6 lines

О©╫О©╫Functions inc_rt_prio_smp() and dec_rt_prio_smp() change (global) rq's priority,
О©╫О©╫while rt_rq passed to them may be not the top-level rt_rq. This is wrong, because
О©╫О©╫changing of priority on a child level does not guarantee that the priority is
О©╫О©╫the highest all over the rq. So, this leak makes RT balancing unusable.

Quoted 8 lines

О©╫О©╫The short example: the task having the highest priority among all rq's RT tasks
О©╫О©╫(no one other task has the same priority) are waking on a throttle rt_rq.
О©╫О©╫The rq's cpupri is set to the task's priority equivalent, but real
О©╫О©╫rq->rt.highest_prio.curr is less.
О©╫О©╫The patch below fixes the problem.
О©╫О©╫It looks like all version have this bug, so I CC'ed stable mailing list.

Quoted 7 lines

Quoted 2 lines
Quoted 1 line
О©╫cpupri stuff should indeed only be changed for the top level group.
О©╫Ingo, are you going to apply this patch? Or will you give any comments?
I queued it, Ingo should get it through me somewhere today if all things go well.
Thanks

Thanks, Peter

tip-bot for Kirill Tkhai

merge bot

2013-12-18 10:32 UTC #6 lore raw

[tip:sched/core] sched/rt: Fix rq's cpupri leak while enqueue/ dequeue child RT entities

In reply to Kirill Tkhai

sched/rt: Fix rq's cpupri leak while enqueue/dequeue child RT entities

Commit-ID:  757dfcaa41844595964f1220f1d33182dae49976
Gitweb:     http://git.kernel.org/tip/757dfcaa41844595964f1220f1d33182dae49976
Author:     Kirill Tkhai <[email protected]>
AuthorDate: Wed, 27 Nov 2013 19:59:13 +0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 17 Dec 2013 15:08:44 +0100

sched/rt: Fix rq's cpupri leak while enqueue/dequeue child RT entities

This patch touches the RT group scheduling case.

The patch below fixes the problem.

Signed-off-by Kirill Tkhai <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

CC Steven Rostedt <[email protected]>

CC [email protected]

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/rt.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

kernel/sched/rt.c +14 -0

index 7d57275..1c40655 100644
@@ -901,6 +901,13 @@ inc_rt_prio_smp(struct rt_rq *rt_rq, int prio, int prev_prio)
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
+#ifdef CONFIG_RT_GROUP_SCHED
+	/*
+	 * Change rq's cpupri only if rt_rq is the top queue.
+	 */
+	if (&rq->rt != rt_rq)
+		return;
+#endif
 	if (rq->online && prio < prev_prio)
 		cpupri_set(&rq->rd->cpupri, rq->cpu, prio);
 }

@@ -910,6 +917,13 @@ dec_rt_prio_smp(struct rt_rq *rt_rq, int prio, int prev_prio)
 {
 	struct rq *rq = rq_of_rt_rq(rt_rq);
 
+#ifdef CONFIG_RT_GROUP_SCHED
+	/*
+	 * Change rq's cpupri only if rt_rq is the top queue.
+	 */
+	if (&rq->rt != rt_rq)
+		return;
+#endif
 	if (rq->online && rt_rq->highest_prio.curr != prev_prio)
 		cpupri_set(&rq->rd->cpupri, rq->cpu, rt_rq->highest_prio.curr);
 }