[PATCH] sched/deadline: Fix bad accounting of nr_running

[PATCH] sched/deadline: Fix bad accounting of nr_running

[Steven Rostedt]

My test suite was locking up hard when enabling mmiotracer. This was due
to the mmiotracer placing all but one CPU offline. I found this out
when I was able to reproduce the bug with just my stress-cpu-hotplug
test. This bug baffled me because it would not always trigger, and
would only trigger on the first run after boot up. The
stress-cpu-hotplug test would crash hard the first run, or never crash
at all. But a new reboot may cause it to crash on the first run again.

I spent all week bisecting this, as I couldn't find a consistent
reproducer. I finally narrowed it down to the sched deadline patches,
and even more peculiar, to the commit that added the sched
deadline boot up self test to the latency tracer. Then it dawned on me
to what the bug was.

All it took was to run a task under sched deadline to screw up the CPU
hot plugging. This explained why it would lock up only on the first run
of the stress-cpu-hotplug test. The bug happened when the boot up self
test of the schedule latency tracer would test a deadline task. The
deadline task would corrupt something that would cause CPU hotplug to
fail. If it didn't corrupt it, the stress test would always work
(there's no other sched deadline tasks that would run to cause
problems). If it did corrupt on boot up, the first test would lockup
hard.

I proved this theory by running my deadline test program on another box,
and then run the stress-cpu-hotplug test, and it would now consistently
lock up. I could run stress-cpu-hotplug over and over with no problem,
but once I ran the deadline test, the next run of the
stress-cpu-hotplug would lock hard.

After adding lots of tracing to the code, I found the cause. The
function tracer showed that migrate_tasks() was stuck in an infinite
loop, where rq->nr_running never equaled 1 to break out of it. When I
added a trace_printk() to see what that number was, it was 335 and
never decrementing!

Looking at the deadline code I found:

static void __dequeue_task_dl(struct rq *rq, struct task_struct *p, int
flags) {
	dequeue_dl_entity(&p->dl);
	dequeue_pushable_dl_task(rq, p);
}

static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int
flags) {
	update_curr_dl(rq);
	__dequeue_task_dl(rq, p, flags);

	dec_nr_running(rq);
}

And this:

	if (dl_runtime_exceeded(rq, dl_se)) {
		__dequeue_task_dl(rq, curr, 0);
		if (likely(start_dl_timer(dl_se, curr->dl.dl_boosted)))
			dl_se->dl_throttled = 1;
		else
			enqueue_task_dl(rq, curr, ENQUEUE_REPLENISH);

		if (!is_leftmost(curr, &rq->dl))
			resched_task(curr);
	}

Notice how we call __dequeue_task_dl() and in the else case we
call enqueue_task_dl()? Also notice that dequeue_task_dl() has
underscores where enqueue_task_dl() does not. The enqueue_task_dl()
calls inc_nr_running(rq), but __dequeue_task_dl() does not. This is
where we get nr_running out of sync.

By moving the dec_nr_running() from dequeue_task_dl() to
__dequeue_task_dl(), everything works again. That is, I can run the
deadline test program and then run the stress-cpu-hotplug() and all
would be fine.

For reference on my test programs:

  http://rostedt.homelinux.com/private/stress-cpu-hotplug
  http://rostedt.homelinux.com/private/deadline.c

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 0dd5e09..84c2454 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -844,14 +844,14 @@ static void __dequeue_task_dl(struct rq *rq,
struct task_struct *p, int flags) {
 	dequeue_dl_entity(&p->dl);
 	dequeue_pushable_dl_task(rq, p);
+
+	dec_nr_running(rq);
 }
 
 static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int
flags) {
 	update_curr_dl(rq);
 	__dequeue_task_dl(rq, p, flags);
-
-	dec_nr_running(rq);
 }
 
 /*

Re: [PATCH] sched/deadline: Fix bad accounting of nr_running

[Peter Zijlstra]

On Fri, Feb 14, 2014 at 11:59:46PM -0500, Steven Rostedt wrote:
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 0dd5e09..84c2454 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -844,14 +844,14 @@ static void __dequeue_task_dl(struct rq *rq,
> struct task_struct *p, int flags) {
>  	dequeue_dl_entity(&p->dl);
>  	dequeue_pushable_dl_task(rq, p);
> +
> +	dec_nr_running(rq);
>  }
>  
>  static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int
> flags) {
>  	update_curr_dl(rq);
>  	__dequeue_task_dl(rq, p, flags);
> -
> -	dec_nr_running(rq);
>  }
>  
>  /*

That patch is so line-wrapped... tskk!

Re: [PATCH] sched/deadline: Fix bad accounting of nr_running

[Steven Rostedt]

On Sat, 15 Feb 2014 10:52:07 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Feb 14, 2014 at 11:59:46PM -0500, Steven Rostedt wrote:
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index 0dd5e09..84c2454 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -844,14 +844,14 @@ static void __dequeue_task_dl(struct rq *rq,
> > struct task_struct *p, int flags) {
> >  	dequeue_dl_entity(&p->dl);
> >  	dequeue_pushable_dl_task(rq, p);
> > +
> > +	dec_nr_running(rq);
> >  }
> >  
> >  static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int
> > flags) {
> >  	update_curr_dl(rq);
> >  	__dequeue_task_dl(rq, p, flags);
> > -
> > -	dec_nr_running(rq);
> >  }
> >  
> >  /*
> 
> That patch is so line-wrapped... tskk!

Damn it, That's claws mail. It inserts files (patches) fine, but if you
go and modify something, it decides to "autowrap" the entire file.
That's one of the things I need to fix when I get the time (I've
downloaded the source as this frustrates the hell out of me). The
autowrapping is the thing that I hate most about this client.

I'll send out a new patch later :-/

-- Steve

[PATCH v2] sched/deadline: Fix bad accounting of nr_running

[Steven Rostedt]

My test suite was locking up hard when enabling mmiotracer. This was due
to the mmiotracer placing all but one CPU offline. I found this out
when I was able to reproduce the bug with just my stress-cpu-hotplug
test. This bug baffled me because it would not always trigger, and
would only trigger on the first run after boot up. The
stress-cpu-hotplug test would crash hard the first run, or never crash
at all. But a new reboot may cause it to crash on the first run again.

I spent all week bisecting this, as I couldn't find a consistent
reproducer. I finally narrowed it down to the sched deadline patches,
and even more peculiar, to the commit that added the sched
deadline boot up self test to the latency tracer. Then it dawned on me
to what the bug was.

All it took was to run a task under sched deadline to screw up the CPU
hot plugging. This explained why it would lock up only on the first run
of the stress-cpu-hotplug test. The bug happened when the boot up self
test of the schedule latency tracer would test a deadline task. The
deadline task would corrupt something that would cause CPU hotplug to
fail. If it didn't corrupt it, the stress test would always work
(there's no other sched deadline tasks that would run to cause
problems). If it did corrupt on boot up, the first test would lockup
hard.

I proved this theory by running my deadline test program on another box,
and then run the stress-cpu-hotplug test, and it would now consistently
lock up. I could run stress-cpu-hotplug over and over with no problem,
but once I ran the deadline test, the next run of the
stress-cpu-hotplug would lock hard.

After adding lots of tracing to the code, I found the cause. The
function tracer showed that migrate_tasks() was stuck in an infinite
loop, where rq->nr_running never equaled 1 to break out of it. When I
added a trace_printk() to see what that number was, it was 335 and
never decrementing!

Looking at the deadline code I found:

static void __dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags) {
	dequeue_dl_entity(&p->dl);
	dequeue_pushable_dl_task(rq, p);
}

static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags) {
	update_curr_dl(rq);
	__dequeue_task_dl(rq, p, flags);

	dec_nr_running(rq);
}

And this:

	if (dl_runtime_exceeded(rq, dl_se)) {
		__dequeue_task_dl(rq, curr, 0);
		if (likely(start_dl_timer(dl_se, curr->dl.dl_boosted)))
			dl_se->dl_throttled = 1;
		else
			enqueue_task_dl(rq, curr, ENQUEUE_REPLENISH);

		if (!is_leftmost(curr, &rq->dl))
			resched_task(curr);
	}

Notice how we call __dequeue_task_dl() and in the else case we
call enqueue_task_dl()? Also notice that dequeue_task_dl() has
underscores where enqueue_task_dl() does not. The enqueue_task_dl()
calls inc_nr_running(rq), but __dequeue_task_dl() does not. This is
where we get nr_running out of sync.

By moving the dec_nr_running() from dequeue_task_dl() to
__dequeue_task_dl(), everything works again. That is, I can run the
deadline test program and then run the stress-cpu-hotplug() and all
would be fine.

For reference on my test programs:

  http://rostedt.homelinux.com/private/stress-cpu-hotplug
  http://rostedt.homelinux.com/private/deadline.c

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---

v2. cleaned up claws mail line wrapping crap

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 0dd5e09..84c2454 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -844,14 +844,14 @@ static void __dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 {
 	dequeue_dl_entity(&p->dl);
 	dequeue_pushable_dl_task(rq, p);
+
+	dec_nr_running(rq);
 }
 
 static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 {
 	update_curr_dl(rq);
 	__dequeue_task_dl(rq, p, flags);
-
-	dec_nr_running(rq);
 }
 
 /*

Re: [PATCH] sched/deadline: Fix bad accounting of nr_running

[Juri Lelli]

Hi,

On 02/15/2014 05:59 AM, Steven Rostedt wrote:
> My test suite was locking up hard when enabling mmiotracer. This was due
> to the mmiotracer placing all but one CPU offline. I found this out
> when I was able to reproduce the bug with just my stress-cpu-hotplug
> test. This bug baffled me because it would not always trigger, and
> would only trigger on the first run after boot up. The
> stress-cpu-hotplug test would crash hard the first run, or never crash
> at all. But a new reboot may cause it to crash on the first run again.
> 
> I spent all week bisecting this, as I couldn't find a consistent
> reproducer. I finally narrowed it down to the sched deadline patches,
> and even more peculiar, to the commit that added the sched
> deadline boot up self test to the latency tracer. Then it dawned on me
> to what the bug was.
> 
> All it took was to run a task under sched deadline to screw up the CPU
> hot plugging. This explained why it would lock up only on the first run
> of the stress-cpu-hotplug test. The bug happened when the boot up self
> test of the schedule latency tracer would test a deadline task. The
> deadline task would corrupt something that would cause CPU hotplug to
> fail. If it didn't corrupt it, the stress test would always work
> (there's no other sched deadline tasks that would run to cause
> problems). If it did corrupt on boot up, the first test would lockup
> hard.
> 
> I proved this theory by running my deadline test program on another box,
> and then run the stress-cpu-hotplug test, and it would now consistently
> lock up. I could run stress-cpu-hotplug over and over with no problem,
> but once I ran the deadline test, the next run of the
> stress-cpu-hotplug would lock hard.
> 
> After adding lots of tracing to the code, I found the cause. The
> function tracer showed that migrate_tasks() was stuck in an infinite
> loop, where rq->nr_running never equaled 1 to break out of it. When I
> added a trace_printk() to see what that number was, it was 335 and
> never decrementing!
> 
> Looking at the deadline code I found:
> 
> static void __dequeue_task_dl(struct rq *rq, struct task_struct *p, int
> flags) {
> 	dequeue_dl_entity(&p->dl);
> 	dequeue_pushable_dl_task(rq, p);
> }
> 
> static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int
> flags) {
> 	update_curr_dl(rq);
> 	__dequeue_task_dl(rq, p, flags);
> 
> 	dec_nr_running(rq);
> }
> 
> And this:
> 
> 	if (dl_runtime_exceeded(rq, dl_se)) {
> 		__dequeue_task_dl(rq, curr, 0);
> 		if (likely(start_dl_timer(dl_se, curr->dl.dl_boosted)))
> 			dl_se->dl_throttled = 1;
> 		else
> 			enqueue_task_dl(rq, curr, ENQUEUE_REPLENISH);
> 
> 		if (!is_leftmost(curr, &rq->dl))
> 			resched_task(curr);
> 	}
> 
> Notice how we call __dequeue_task_dl() and in the else case we
> call enqueue_task_dl()? Also notice that dequeue_task_dl() has
> underscores where enqueue_task_dl() does not. The enqueue_task_dl()
> calls inc_nr_running(rq), but __dequeue_task_dl() does not. This is
> where we get nr_running out of sync.
> 

Right. I'd add another place that could cause this misalignment:

static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
{
[snip]
        dl_se->dl_throttled = 0;
        if (p->on_rq) {
                enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
                if (task_has_dl_policy(rq->curr))
                        check_preempt_curr_dl(rq, p, 0);
                else
                        resched_task(rq->curr);
[snip]
}

This is called when the replenishment timer for a throttled task fires,
and we have to queue it back on the dl_rq with recharged parameters.
Best test for this bug is to run a while(1) task with SCHED_DEADLINE
(using for example https://github.com/jlelli/schedtool-dl). This causes
a lot of throttle/replenish events and causes nr_running to explode.
All is ok with this fix.

> By moving the dec_nr_running() from dequeue_task_dl() to
> __dequeue_task_dl(), everything works again. That is, I can run the
> deadline test program and then run the stress-cpu-hotplug() and all
> would be fine.
> 

Rationale for this odd behavior is that, when a task is throttled, it
is removed only from the dl_rq, but we keep it on_rq (as this is not
a "full dequeue", that is the task is not actually sleeping). But, it
is also true that, while throttled a task behaves like it is sleeping
(e.g., its timer will fire on a new CPU if the old one is dead). So,
Steven's fix sounds also semantically correct.

Thanks!

Best,

- Juri

> For reference on my test programs:
> 
>   http://rostedt.homelinux.com/private/stress-cpu-hotplug
>   http://rostedt.homelinux.com/private/deadline.c
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 0dd5e09..84c2454 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -844,14 +844,14 @@ static void __dequeue_task_dl(struct rq *rq,
> struct task_struct *p, int flags) {
>  	dequeue_dl_entity(&p->dl);
>  	dequeue_pushable_dl_task(rq, p);
> +
> +	dec_nr_running(rq);
>  }
>  
>  static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int
> flags) {
>  	update_curr_dl(rq);
>  	__dequeue_task_dl(rq, p, flags);
> -
> -	dec_nr_running(rq);
>  }
>  
>  /*
> 

[PATCH v3] sched/deadline: Fix bad accounting of nr_running

[Steven Rostedt]

 
> Rationale for this odd behavior is that, when a task is throttled, it
> is removed only from the dl_rq, but we keep it on_rq (as this is not
> a "full dequeue", that is the task is not actually sleeping). But, it
> is also true that, while throttled a task behaves like it is sleeping
> (e.g., its timer will fire on a new CPU if the old one is dead). So,
> Steven's fix sounds also semantically correct.

Actually, it seems that I was hitting it again, but this time getting a
negative number. OK, after looking at the code a bit more, I think we
should update the runqueue nr_running only when the task is officially
enqueued and dequeued, and all accounting within, will not touch that
number.

----
My test suite was locking up hard when enabling mmiotracer. This was due
to the mmiotracer placing all but one CPU offline. I found this out
when I was able to reproduce the bug with just my stress-cpu-hotplug
test. This bug baffled me because it would not always trigger, and
would only trigger on the first run after boot up. The
stress-cpu-hotplug test would crash hard the first run, or never crash
at all. But a new reboot may cause it to crash on the first run again.

I spent all week bisecting this, as I couldn't find a consistent
reproducer. I finally narrowed it down to the sched deadline patches,
and even more peculiar, to the commit that added the sched
deadline boot up self test to the latency tracer. Then it dawned on me
to what the bug was.

All it took was to run a task under sched deadline to screw up the CPU
hot plugging. This explained why it would lock up only on the first run
of the stress-cpu-hotplug test. The bug happened when the boot up self
test of the schedule latency tracer would test a deadline task. The
deadline task would corrupt something that would cause CPU hotplug to
fail. If it didn't corrupt it, the stress test would always work
(there's no other sched deadline tasks that would run to cause
problems). If it did corrupt on boot up, the first test would lockup
hard.

I proved this theory by running my deadline test program on another box,
and then run the stress-cpu-hotplug test, and it would now consistently
lock up. I could run stress-cpu-hotplug over and over with no problem,
but once I ran the deadline test, the next run of the
stress-cpu-hotplug would lock hard.

After adding lots of tracing to the code, I found the cause. The
function tracer showed that migrate_tasks() was stuck in an infinite
loop, where rq->nr_running never equaled 1 to break out of it. When I
added a trace_printk() to see what that number was, it was 335 and
never decrementing!

Looking at the deadline code I found:

static void __dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags) {
	dequeue_dl_entity(&p->dl);
	dequeue_pushable_dl_task(rq, p);
}

static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags) {
	update_curr_dl(rq);
	__dequeue_task_dl(rq, p, flags);

	dec_nr_running(rq);
}

And this:

	if (dl_runtime_exceeded(rq, dl_se)) {
		__dequeue_task_dl(rq, curr, 0);
		if (likely(start_dl_timer(dl_se, curr->dl.dl_boosted)))
			dl_se->dl_throttled = 1;
		else
			enqueue_task_dl(rq, curr, ENQUEUE_REPLENISH);

		if (!is_leftmost(curr, &rq->dl))
			resched_task(curr);
	}

Notice how we call __dequeue_task_dl() and in the else case we
call enqueue_task_dl()? Also notice that dequeue_task_dl() has
underscores where enqueue_task_dl() does not. The enqueue_task_dl()
calls inc_nr_running(rq), but __dequeue_task_dl() does not. This is
where we get nr_running out of sync.

By only updating the nr_running when the task is officially enqueued or
dequeued by the core scheduler, we can prevent the accounting of that
number from going out of sync.

Link: http://lkml.kernel.org/r/20140214235946.60a89b65@gandalf.local.home

For reference on my test programs:

  http://rostedt.homelinux.com/private/stress-cpu-hotplug
  http://rostedt.homelinux.com/private/deadline.c

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
v3. Only update nr_running in enqueue and dequeue of core scheduler calls

v2. cleaned up claws mail line wrapping crap

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 0dd5e09..27cb282 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -238,7 +238,7 @@ void dec_dl_migration(struct sched_dl_entity *dl_se, struct dl_rq *dl_rq)
 
 #endif /* CONFIG_SMP */
 
-static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags);
+static void __enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags);
 static void __dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags);
 static void check_preempt_curr_dl(struct rq *rq, struct task_struct *p,
 				  int flags);
@@ -510,7 +510,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 	update_rq_clock(rq);
 	dl_se->dl_throttled = 0;
 	if (p->on_rq) {
-		enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
+		__enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
 		if (task_has_dl_policy(rq->curr))
 			check_preempt_curr_dl(rq, p, 0);
 		else
@@ -608,7 +608,7 @@ static void update_curr_dl(struct rq *rq)
 		if (likely(start_dl_timer(dl_se, curr->dl.dl_boosted)))
 			dl_se->dl_throttled = 1;
 		else
-			enqueue_task_dl(rq, curr, ENQUEUE_REPLENISH);
+			__enqueue_task_dl(rq, curr, ENQUEUE_REPLENISH);
 
 		if (!is_leftmost(curr, &rq->dl))
 			resched_task(curr);
@@ -809,7 +809,7 @@ static void dequeue_dl_entity(struct sched_dl_entity *dl_se)
 	__dequeue_dl_entity(dl_se);
 }
 
-static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
+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 sched_dl_entity *pi_se = &p->dl;
@@ -836,7 +836,11 @@ static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 
 	if (!task_current(rq, p) && p->nr_cpus_allowed > 1)
 		enqueue_pushable_dl_task(rq, p);
+}
 
+static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
+{
+	__enqueue_task_dl(rq, p, flags);
 	inc_nr_running(rq);
 }
 

Re: [PATCH v3] sched/deadline: Fix bad accounting of nr_running

[Peter Zijlstra]

On Tue, Feb 18, 2014 at 09:50:12PM -0500, Steven Rostedt wrote:
>  
> > Rationale for this odd behavior is that, when a task is throttled, it
> > is removed only from the dl_rq, but we keep it on_rq (as this is not
> > a "full dequeue", that is the task is not actually sleeping). But, it
> > is also true that, while throttled a task behaves like it is sleeping
> > (e.g., its timer will fire on a new CPU if the old one is dead). So,
> > Steven's fix sounds also semantically correct.
> 
> Actually, it seems that I was hitting it again, but this time getting a
> negative number. OK, after looking at the code a bit more, I think we
> should update the runqueue nr_running only when the task is officially
> enqueued and dequeued, and all accounting within, will not touch that
> number.

But if the task is throttled it should still very much decrement the
number. There's places that very much rely on nr_running be exactly the
number of runnable tasks.

Re: [PATCH v3] sched/deadline: Fix bad accounting of nr_running

[Juri Lelli]

On 02/19/2014 09:46 AM, Peter Zijlstra wrote:
> On Tue, Feb 18, 2014 at 09:50:12PM -0500, Steven Rostedt wrote:
>>  
>>> Rationale for this odd behavior is that, when a task is throttled, it
>>> is removed only from the dl_rq, but we keep it on_rq (as this is not
>>> a "full dequeue", that is the task is not actually sleeping). But, it
>>> is also true that, while throttled a task behaves like it is sleeping
>>> (e.g., its timer will fire on a new CPU if the old one is dead). So,
>>> Steven's fix sounds also semantically correct.
>>
>> Actually, it seems that I was hitting it again, but this time getting a
>> negative number. OK, after looking at the code a bit more, I think we
>> should update the runqueue nr_running only when the task is officially
>> enqueued and dequeued, and all accounting within, will not touch that
>> number.

This is a different way to get the same result (mildly tested on my box):

---
 kernel/sched/deadline.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 0dd5e09..675dad3 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -837,7 +837,8 @@ static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
        if (!task_current(rq, p) && p->nr_cpus_allowed > 1)
                enqueue_pushable_dl_task(rq, p);

-       inc_nr_running(rq);
+       if (!(flags & ENQUEUE_REPLENISH))
+               inc_nr_running(rq);
 }

 static void __dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags)
--

We touch nr_running only when we don't enqueue back as a consequence
of a replenishment.

> 
> But if the task is throttled it should still very much decrement the
> number. There's places that very much rely on nr_running be exactly the
> number of runnable tasks.
> 

This is a different thing, and V2 seemed to implement this behavior
(that's why I said it looked semantically correct).

Thanks,

- Juri

Re: [PATCH v3] sched/deadline: Fix bad accounting of nr_running

[Juri Lelli]

On 02/19/2014 11:32 AM, Juri Lelli wrote:
> On 02/19/2014 09:46 AM, Peter Zijlstra wrote:
>> On Tue, Feb 18, 2014 at 09:50:12PM -0500, Steven Rostedt wrote:
>>>  
>>>> Rationale for this odd behavior is that, when a task is throttled, it
>>>> is removed only from the dl_rq, but we keep it on_rq (as this is not
>>>> a "full dequeue", that is the task is not actually sleeping). But, it
>>>> is also true that, while throttled a task behaves like it is sleeping
>>>> (e.g., its timer will fire on a new CPU if the old one is dead). So,
>>>> Steven's fix sounds also semantically correct.
>>>
>>> Actually, it seems that I was hitting it again, but this time getting a
>>> negative number. OK, after looking at the code a bit more, I think we
>>> should update the runqueue nr_running only when the task is officially
>>> enqueued and dequeued, and all accounting within, will not touch that
>>> number.
> 
> This is a different way to get the same result (mildly tested on my box):
> 
> ---
>  kernel/sched/deadline.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 0dd5e09..675dad3 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -837,7 +837,8 @@ static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
>         if (!task_current(rq, p) && p->nr_cpus_allowed > 1)
>                 enqueue_pushable_dl_task(rq, p);
> 
> -       inc_nr_running(rq);
> +       if (!(flags & ENQUEUE_REPLENISH))
> +               inc_nr_running(rq);
>  }
> 
>  static void __dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags)
> --
> 
> We touch nr_running only when we don't enqueue back as a consequence
> of a replenishment.
> 
>>
>> But if the task is throttled it should still very much decrement the
>> number. There's places that very much rely on nr_running be exactly the
>> number of runnable tasks.
>>
> 
> This is a different thing, and V2 seemed to implement this behavior
> (that's why I said it looked semantically correct).
> 

So, both my last approach and Steven's V2 were causing nr_running to
become negative, as they double decrement it when dequeuing a task that
also exceeded its budget.

What follows seems to solve the issue, and correcly account for throttled
tasks as !nr_running.

---
 kernel/sched/deadline.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 0dd5e09..b819577 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -717,6 +717,7 @@ void inc_dl_tasks(struct sched_dl_entity *dl_se, struct dl_rq *dl_rq)

        WARN_ON(!dl_prio(prio));
        dl_rq->dl_nr_running++;
+       inc_nr_running(rq_of_dl_rq(dl_rq));

        inc_dl_deadline(dl_rq, deadline);
        inc_dl_migration(dl_se, dl_rq);
@@ -730,6 +731,7 @@ void dec_dl_tasks(struct sched_dl_entity *dl_se, struct dl_rq *dl_rq)
        WARN_ON(!dl_prio(prio));
        WARN_ON(!dl_rq->dl_nr_running);
        dl_rq->dl_nr_running--;
+       dec_nr_running(rq_of_dl_rq(dl_rq));

        dec_dl_deadline(dl_rq, dl_se->deadline);
        dec_dl_migration(dl_se, dl_rq);
@@ -836,8 +838,6 @@ static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)

        if (!task_current(rq, p) && p->nr_cpus_allowed > 1)
                enqueue_pushable_dl_task(rq, p);
-
-       inc_nr_running(rq);
 }

 static void __dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags)
@@ -850,8 +850,6 @@ static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 {
        update_curr_dl(rq);
        __dequeue_task_dl(rq, p, flags);
-
-       dec_nr_running(rq);
 }

 /*
-- 
1.7.9.5

Steven, could you test it?

Thanks,

- Juri

Re: [PATCH v3] sched/deadline: Fix bad accounting of nr_running

[Steven Rostedt]

On Wed, 19 Feb 2014 14:14:54 +0100
Juri Lelli <juri.lelli@gmail.com> wrote:

> Steven, could you test it?

OK, I tested it on my x86_64 box that was also causing troubles.

Please fix up the whitespace issues and send a formal patch to Peter.

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Tested-by: Steven Rostedt <rostedt@goodmis.org>
Added-bys-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve