[PATCH] sched/deadline: Update total bandwidth when adding new task

[PATCH] sched/deadline: Update total bandwidth when adding new task

[Steven Rostedt]

While debugging the crash with the bad nr_running accounting, I hit
another bug where, after running my sched deadline test, I was getting
failures to take a CPU offline. It was giving me a -EBUSY error.

Adding a bunch of trace_printk()s around, I found that the cpu
notifier that called sched_cpu_inactive() was returning a failure. The
overflow value was coming up negative?

Adding more trace_printk()s, I found that task_dead_dl() function was
subtracting the exact amount that was keeping the CPU from going
offline. I then realized that the task_dead_dl() was updating the
total_bw for the task that was going away, but there was nothing that
added to the total_bw when the task came alive. If total_bw is not
zero for a CPU, it will keep that CPU from going offline.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b46131e..17f4830 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3178,6 +3178,7 @@ static void
 __setparam_dl(struct task_struct *p, const struct sched_attr *attr)
 {
 	struct sched_dl_entity *dl_se = &p->dl;
+	struct dl_bw *dl_b = dl_bw_of(task_cpu(p));
 
 	init_dl_task_timer(dl_se);
 	dl_se->dl_runtime = attr->sched_runtime;
@@ -3187,6 +3188,7 @@ __setparam_dl(struct task_struct *p, const struct sched_attr *attr)
 	dl_se->dl_bw = to_ratio(dl_se->dl_period, dl_se->dl_runtime);
 	dl_se->dl_throttled = 0;
 	dl_se->dl_new = 1;
+	__dl_add(dl_b, dl_se->dl_bw);
 }
 
 /* Actually do priority change: must hold pi & rq lock. */

Re: [PATCH] sched/deadline: Update total bandwidth when adding new task

[Juri Lelli]

On 02/19/2014 03:56 AM, Steven Rostedt wrote:
> While debugging the crash with the bad nr_running accounting, I hit
> another bug where, after running my sched deadline test, I was getting
> failures to take a CPU offline. It was giving me a -EBUSY error.
> 
> Adding a bunch of trace_printk()s around, I found that the cpu
> notifier that called sched_cpu_inactive() was returning a failure. The
> overflow value was coming up negative?
> 
> Adding more trace_printk()s, I found that task_dead_dl() function was
> subtracting the exact amount that was keeping the CPU from going
> offline. I then realized that the task_dead_dl() was updating the
> total_bw for the task that was going away, but there was nothing that
> added to the total_bw when the task came alive. 

We call __dl_add() from dl_overflow(), right before calling __setscheduler().

> If total_bw is not
> zero for a CPU, it will keep that CPU from going offline.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b46131e..17f4830 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3178,6 +3178,7 @@ static void
>  __setparam_dl(struct task_struct *p, const struct sched_attr *attr)
>  {
>  	struct sched_dl_entity *dl_se = &p->dl;
> +	struct dl_bw *dl_b = dl_bw_of(task_cpu(p));
>  
>  	init_dl_task_timer(dl_se);
>  	dl_se->dl_runtime = attr->sched_runtime;
> @@ -3187,6 +3188,7 @@ __setparam_dl(struct task_struct *p, const struct sched_attr *attr)
>  	dl_se->dl_bw = to_ratio(dl_se->dl_period, dl_se->dl_runtime);
>  	dl_se->dl_throttled = 0;
>  	dl_se->dl_new = 1;
> +	__dl_add(dl_b, dl_se->dl_bw);
>  }

This is intended to set params for a task that is going to become -deadline. I
don't think it is the right place to update dl_b. And it seems you add it twice
(inside dl_overflow and inside __setscheduler).

I performed some tests, and it seems that dl_b is updated correctly, once when
the task is started and once when task_dead_dl() is called.

That said, I'm still testing with your stress-cpu-hotplug.

Thanks,

- Juri

>  
>  /* Actually do priority change: must hold pi & rq lock. */
> 

[PATCH] sched/deadline: Fix overflow to handle period==0 and deadline!=0

[Steven Rostedt]

While debugging the crash with the bad nr_running accounting, I hit
another bug where, after running my sched deadline test, I was getting
failures to take a CPU offline. It was giving me a -EBUSY error.

Adding a bunch of trace_printk()s around, I found that the cpu
notifier that called sched_cpu_inactive() was returning a failure. The
overflow value was coming up negative?

Talking this over with Juri, the problem is that the total_bw update was
suppose to be made by dl_overflow() which, during my tests, seemed to
not be called. Adding more trace_printk()s, it wasn't that it wasn't
called, but it exited out right away with the check of new_bw being
equal to p->dl.dl_bw. The new_bw calculates the ratio between period and
runtime. The bug is that if you set a deadline, you do not need to set
a period if you plan on the period being equal to the deadline. That
is, if period is zero and deadline is not, then the system call should
set the period to be equal to the deadline. This is done elsewhere in
the code.

The fix is easy, check if period is set, and if it is not, then use the
deadline.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b46131e..2491448 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1952,7 +1952,7 @@ static int dl_overflow(struct task_struct *p, int policy,
 {
 
 	struct dl_bw *dl_b = dl_bw_of(task_cpu(p));
-	u64 period = attr->sched_period;
+	u64 period = attr->sched_period ?: attr->sched_deadline;
 	u64 runtime = attr->sched_runtime;
 	u64 new_bw = dl_policy(policy) ? to_ratio(period, runtime) : 0;
 	int cpus, err = -1;

Re: [PATCH] sched/deadline: Fix overflow to handle period==0 and deadline!=0

[Juri Lelli]

On 02/19/2014 07:53 PM, Steven Rostedt wrote:
> While debugging the crash with the bad nr_running accounting, I hit
> another bug where, after running my sched deadline test, I was getting
> failures to take a CPU offline. It was giving me a -EBUSY error.
> 
> Adding a bunch of trace_printk()s around, I found that the cpu
> notifier that called sched_cpu_inactive() was returning a failure. The
> overflow value was coming up negative?
> 
> Talking this over with Juri, the problem is that the total_bw update was
> suppose to be made by dl_overflow() which, during my tests, seemed to
> not be called. Adding more trace_printk()s, it wasn't that it wasn't
> called, but it exited out right away with the check of new_bw being
> equal to p->dl.dl_bw. The new_bw calculates the ratio between period and
> runtime. The bug is that if you set a deadline, you do not need to set
> a period if you plan on the period being equal to the deadline. That
> is, if period is zero and deadline is not, then the system call should
> set the period to be equal to the deadline. This is done elsewhere in
> the code.
> 
> The fix is easy, check if period is set, and if it is not, then use the
> deadline.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b46131e..2491448 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1952,7 +1952,7 @@ static int dl_overflow(struct task_struct *p, int policy,
>  {
>  
>  	struct dl_bw *dl_b = dl_bw_of(task_cpu(p));
> -	u64 period = attr->sched_period;
> +	u64 period = attr->sched_period ?: attr->sched_deadline;
>  	u64 runtime = attr->sched_runtime;
>  	u64 new_bw = dl_policy(policy) ? to_ratio(period, runtime) : 0;
>  	int cpus, err = -1;
> 

Thanks!

- Juri

[tip:sched/urgent] sched/deadline: Fix overflow to handle period= =0 and deadline!=0

[tip-bot for Steven Rostedt]

Commit-ID:  4df1638cfaf9b2b7ad993979a41965acab9cd156
Gitweb:     http://git.kernel.org/tip/4df1638cfaf9b2b7ad993979a41965acab9cd156
Author:     Steven Rostedt <rostedt@goodmis.org>
AuthorDate: Wed, 19 Feb 2014 13:53:35 -0500
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 21 Feb 2014 21:27:09 +0100

sched/deadline: Fix overflow to handle period==0 and deadline!=0

While debugging the crash with the bad nr_running accounting, I hit
another bug where, after running my sched deadline test, I was getting
failures to take a CPU offline. It was giving me a -EBUSY error.

Adding a bunch of trace_printk()s around, I found that the cpu
notifier that called sched_cpu_inactive() was returning a failure. The
overflow value was coming up negative?

Talking this over with Juri, the problem is that the total_bw update was
suppose to be made by dl_overflow() which, during my tests, seemed to
not be called. Adding more trace_printk()s, it wasn't that it wasn't
called, but it exited out right away with the check of new_bw being
equal to p->dl.dl_bw. The new_bw calculates the ratio between period and
runtime. The bug is that if you set a deadline, you do not need to set
a period if you plan on the period being equal to the deadline. That
is, if period is zero and deadline is not, then the system call should
set the period to be equal to the deadline. This is done elsewhere in
the code.

The fix is easy, check if period is set, and if it is not, then use the
deadline.

Cc: Juri Lelli <juri.lelli@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20140219135335.7e74abd4@gandalf.local.home
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/sched/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b46131e..2491448 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1952,7 +1952,7 @@ static int dl_overflow(struct task_struct *p, int policy,
 {
 
 	struct dl_bw *dl_b = dl_bw_of(task_cpu(p));
-	u64 period = attr->sched_period;
+	u64 period = attr->sched_period ?: attr->sched_deadline;
 	u64 runtime = attr->sched_runtime;
 	u64 new_bw = dl_policy(policy) ? to_ratio(period, runtime) : 0;
 	int cpus, err = -1;