[PATCH][mmotm] Sched fix stale value in average load per task

[PATCH][mmotm] Sched fix stale value in average load per task

[Balbir Singh]

cpu_avg_load_per_task() returns a stale value when nr_running is 0. It returns
an older stale (caculated when nr_running was non zero) value. This patch
returns and sets rq->avg_load_per_task to zero when nr_running is 0.

Compile and boot tested on a x86_64 box.

Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

 kernel/sched.c |    2 ++
 1 file changed, 2 insertions(+)

diff -puN kernel/sched.c~sched-fix-stale-load-avg kernel/sched.c
--- linux-2.6.28-rc4/kernel/sched.c~sched-fix-stale-load-avg	2008-11-12 15:55:38.000000000 +0530
+++ linux-2.6.28-rc4-balbir/kernel/sched.c	2008-11-12 15:56:09.000000000 +0530
@@ -1430,6 +1430,8 @@ static unsigned long cpu_avg_load_per_ta
 
 	if (rq->nr_running)
 		rq->avg_load_per_task = rq->load.weight / rq->nr_running;
+	else
+		rq->avg_load_per_task = 0;
 
 	return rq->avg_load_per_task;
 }
_

-- 
	Balbir

Re: [PATCH][mmotm] Sched fix stale value in average load per task

[Ingo Molnar]

* Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> cpu_avg_load_per_task() returns a stale value when nr_running is 0. 
> It returns an older stale (caculated when nr_running was non zero) 
> value. This patch returns and sets rq->avg_load_per_task to zero 
> when nr_running is 0.
> 
> Compile and boot tested on a x86_64 box.
> 
> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> ---
> 
>  kernel/sched.c |    2 ++
>  1 file changed, 2 insertions(+)

applied to tip/sched/urgent, thanks Balbir!

i'm wondering, have you observed load-balancer misbehavior due to this 
load-average imprecision bug, on some workload? (or have you found 
this via code review)

	Ingo

Re: [PATCH][mmotm] Sched fix stale value in average load per task

[Balbir Singh]

* Balbir Singh <balbir@linux.vnet.ibm.com> [2008-11-12 16:19:00]:

> +	else
> +		rq->avg_load_per_task = 0;
>  
>  	return rq->avg_load_per_task;

On second thoughts, does this look better? (Based on Mike Galbraith's
suggestion)


cpu_avg_load_per_task() returns a stale value when nr_running is 0. It returns
an older stale (caculated when nr_running was non zero) value. This patch
returns and sets rq->avg_load_per_task to zero when nr_running is 0.

Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

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

diff -puN kernel/sched.c~sched-fix-stale-load-avg kernel/sched.c
--- linux-2.6.28-rc4/kernel/sched.c~sched-fix-stale-load-avg	2008-11-12 15:55:38.000000000 +0530
+++ linux-2.6.28-rc4-balbir/kernel/sched.c	2008-11-12 16:40:26.000000000 +0530
@@ -571,8 +571,6 @@ struct rq {
 	int cpu;
 	int online;
 
-	unsigned long avg_load_per_task;
-
 	struct task_struct *migration_thread;
 	struct list_head migration_queue;
 #endif
@@ -1428,10 +1426,10 @@ static unsigned long cpu_avg_load_per_ta
 {
 	struct rq *rq = cpu_rq(cpu);
 
-	if (rq->nr_running)
-		rq->avg_load_per_task = rq->load.weight / rq->nr_running;
-
-	return rq->avg_load_per_task;
+	if (unlikely(!rq->nr_running))
+		return 0;
+	else
+		return rq->load.weight / rq->nr_running;
 }
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
_

 

-- 
	Balbir

Re: [PATCH][mmotm] Sched fix stale value in average load per task

[Ingo Molnar]

* Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> * Balbir Singh <balbir@linux.vnet.ibm.com> [2008-11-12 16:19:00]:
> 
> > +	else
> > +		rq->avg_load_per_task = 0;
> >  
> >  	return rq->avg_load_per_task;
> 
> On second thoughts, does this look better? (Based on Mike 
> Galbraith's suggestion)

yes - but could you send it as a delta patch against your previous 
patch please? We want the simpler patch for tip/sched/urgent, as a 
v2.6.28 fix - the removal of rq->avg_load_per_task can wait until 
v2.6.29. (Please also include Mike's Ack in the patch if he acks the 
patch.)

	Ingo

Re: [PATCH][mmotm] Sched fix stale value in average load per task

[Balbir Singh]

Ingo Molnar wrote:
> * Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
>> cpu_avg_load_per_task() returns a stale value when nr_running is 0. 
>> It returns an older stale (caculated when nr_running was non zero) 
>> value. This patch returns and sets rq->avg_load_per_task to zero 
>> when nr_running is 0.
>>
>> Compile and boot tested on a x86_64 box.
>>
>> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
>> ---
>>
>>  kernel/sched.c |    2 ++
>>  1 file changed, 2 insertions(+)
> 
> applied to tip/sched/urgent, thanks Balbir!
> 
> i'm wondering, have you observed load-balancer misbehavior due to this 
> load-average imprecision bug, on some workload? (or have you found 
> this via code review)


Thanks, Ingo!

I found this issue through code review. I did not see any misbehaviour yet due
to load-average imprecision.


-- 
	Balbir

PS: FYI: I missed this email, since my name was missing in the to and cc list.
Could you check to see if there is any issue with your mailer, I'll do the same.

Re: [PATCH][mmotm] Sched fix stale value in average load per task

[Balbir Singh]

Ingo Molnar wrote:
> * Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
>> * Balbir Singh <balbir@linux.vnet.ibm.com> [2008-11-12 16:19:00]:
>>
>>> +	else
>>> +		rq->avg_load_per_task = 0;
>>>  
>>>  	return rq->avg_load_per_task;
>> On second thoughts, does this look better? (Based on Mike 
>> Galbraith's suggestion)
> 
> yes - but could you send it as a delta patch against your previous 
> patch please? We want the simpler patch for tip/sched/urgent, as a 
> v2.6.28 fix - the removal of rq->avg_load_per_task can wait until 
> v2.6.29. (Please also include Mike's Ack in the patch if he acks the 
> patch.)

Fair enough. I'll send out the delta tomorrow.

-- 
	Balbir