[PATCH] Fix smpnice high priority task hopping problem

[PATCH] Fix smpnice high priority task hopping problem

[Peter Williams]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

Suresh B. Siddha has reported:

"on a lightly loaded system, this will result in higher priority job 
hopping around from one processor to another processor.. This is because 
of the code in find_busiest_group() which assumes that SCHED_LOAD_SCALE 
represents a unit process load and with nice_to_bias calculations this 
is no longer true (in the presence of non nice-0 tasks)"

Analysis of this problem as revealed that the smpnice code results in 
the weighted load being larger than 1 and this triggers the active load 
balancing code.  However, in active_load_balance(), the migration thread 
fails to take into account itself when deciding if there are any tasks 
to be migrated from its run queue. I.e. even if there is only one other 
task on the run queue other than itself it will still migrate that other 
task.

The attached patch fixes that anomaly.

Signed-off-by: Peter Williams <pwil3058@bigpond.com.au>

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

[-- Attachment #2: fix-smpnice-task-hopping-problem --]
[-- Type: text/plain, Size: 471 bytes --]

Index: MM-2.6.X/kernel/sched.c
===================================================================
--- MM-2.6.X.orig/kernel/sched.c	2006-02-16 10:51:52.000000000 +1100
+++ MM-2.6.X/kernel/sched.c	2006-02-16 11:02:45.000000000 +1100
@@ -2406,7 +2406,7 @@ static void active_load_balance(runqueue
 	runqueue_t *target_rq;
 	int target_cpu = busiest_rq->push_cpu;
 
-	if (busiest_rq->nr_running <= 1)
+	if (busiest_rq->nr_running <= 2)
 		/* no task to move */
 		return;
 

Re: [PATCH] Fix smpnice high priority task hopping problem

[Siddha, Suresh B]

Andrew, Please don't apply this patch. This breaks the existing HT
(and multi-core) scheduler optimizations.

Peter, on a DP system with HT, if we have only two runnable processes
and they end up running on the two threads of the same package, 
with your patch, migration thread will never move one of those processes 
to the idle package..

To fix my reported problem, we need to make sure that find_busiest_group()
doesn't find an imbalance..

thanks,
suresh

On Thu, Feb 16, 2006 at 11:39:34AM +1100, Peter Williams wrote:
> Suresh B. Siddha has reported:
> 
> "on a lightly loaded system, this will result in higher priority job 
> hopping around from one processor to another processor.. This is because 
> of the code in find_busiest_group() which assumes that SCHED_LOAD_SCALE 
> represents a unit process load and with nice_to_bias calculations this 
> is no longer true (in the presence of non nice-0 tasks)"
> 
> Analysis of this problem as revealed that the smpnice code results in 
> the weighted load being larger than 1 and this triggers the active load 
> balancing code.  However, in active_load_balance(), the migration thread 
> fails to take into account itself when deciding if there are any tasks 
> to be migrated from its run queue. I.e. even if there is only one other 
> task on the run queue other than itself it will still migrate that other 
> task.
> 
> The attached patch fixes that anomaly.
> 
> Signed-off-by: Peter Williams <pwil3058@bigpond.com.au>
> 
> Peter
> -- 
> Peter Williams                                   pwil3058@bigpond.net.au
> 
> "Learning, n. The kind of ignorance distinguishing the studious."
>   -- Ambrose Bierce

> Index: MM-2.6.X/kernel/sched.c
> ===================================================================
> --- MM-2.6.X.orig/kernel/sched.c	2006-02-16 10:51:52.000000000 +1100
> +++ MM-2.6.X/kernel/sched.c	2006-02-16 11:02:45.000000000 +1100
> @@ -2406,7 +2406,7 @@ static void active_load_balance(runqueue
>  	runqueue_t *target_rq;
>  	int target_cpu = busiest_rq->push_cpu;
>  
> -	if (busiest_rq->nr_running <= 1)
> +	if (busiest_rq->nr_running <= 2)
>  		/* no task to move */
>  		return;
>  

Re: [PATCH] Fix smpnice high priority task hopping problem

[Peter Williams]

Siddha, Suresh B wrote:
> Andrew, Please don't apply this patch. This breaks the existing HT
> (and multi-core) scheduler optimizations.
> 
> Peter, on a DP system with HT, if we have only two runnable processes
> and they end up running on the two threads of the same package, 
> with your patch, migration thread will never move one of those processes 
> to the idle package..

On a normal system, would either of them be moved anyway?

> 
> To fix my reported problem, we need to make sure that find_busiest_group()
> doesn't find an imbalance..

I disagree.  If this causes a problem with your "optimizations" then I 
think that you need to fix the "optimizations".

There's a rational argument (IMHO) that this patch should be applied 
even in the absence of the smpnice patches as it prevents 
active_load_balance() doing unnecessary work.  If this isn't good for 
hypo threading then hypo threading is a special case and needs to handle 
it as such.

> 
> thanks,
> suresh
> 
> On Thu, Feb 16, 2006 at 11:39:34AM +1100, Peter Williams wrote:
> 
>>Suresh B. Siddha has reported:
>>
>>"on a lightly loaded system, this will result in higher priority job 
>>hopping around from one processor to another processor.. This is because 
>>of the code in find_busiest_group() which assumes that SCHED_LOAD_SCALE 
>>represents a unit process load and with nice_to_bias calculations this 
>>is no longer true (in the presence of non nice-0 tasks)"
>>
>>Analysis of this problem as revealed that the smpnice code results in 
>>the weighted load being larger than 1 and this triggers the active load 
>>balancing code.  However, in active_load_balance(), the migration thread 
>>fails to take into account itself when deciding if there are any tasks 
>>to be migrated from its run queue. I.e. even if there is only one other 
>>task on the run queue other than itself it will still migrate that other 
>>task.
>>
>>The attached patch fixes that anomaly.
>>
>>Signed-off-by: Peter Williams <pwil3058@bigpond.com.au>
>>
>>Peter
>>-- 
>>Peter Williams                                   pwil3058@bigpond.net.au
>>
>>"Learning, n. The kind of ignorance distinguishing the studious."
>>  -- Ambrose Bierce
> 
> 
>>Index: MM-2.6.X/kernel/sched.c
>>===================================================================
>>--- MM-2.6.X.orig/kernel/sched.c	2006-02-16 10:51:52.000000000 +1100
>>+++ MM-2.6.X/kernel/sched.c	2006-02-16 11:02:45.000000000 +1100
>>@@ -2406,7 +2406,7 @@ static void active_load_balance(runqueue
>> 	runqueue_t *target_rq;
>> 	int target_cpu = busiest_rq->push_cpu;
>> 
>>-	if (busiest_rq->nr_running <= 1)
>>+	if (busiest_rq->nr_running <= 2)
>> 		/* no task to move */
>> 		return;
>> 


-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH] Fix smpnice high priority task hopping problem

[Peter Williams]

Peter Williams wrote:
> Siddha, Suresh B wrote:
> 
>> Andrew, Please don't apply this patch. This breaks the existing HT
>> (and multi-core) scheduler optimizations.
>>
>> Peter, on a DP system with HT, if we have only two runnable processes
>> and they end up running on the two threads of the same package, with 
>> your patch, migration thread will never move one of those processes to 
>> the idle package..
> 
> 
> On a normal system, would either of them be moved anyway?
> 
>>
>> To fix my reported problem, we need to make sure that 
>> find_busiest_group()
>> doesn't find an imbalance..
> 
> 
> I disagree.  If this causes a problem with your "optimizations" then I 
> think that you need to fix the "optimizations".
> 
> There's a rational argument (IMHO) that this patch should be applied 
> even in the absence of the smpnice patches as it prevents 
> active_load_balance() doing unnecessary work.  If this isn't good for 
> hypo threading then hypo threading is a special case and needs to handle 
> it as such.

OK.  The good news is that (my testing shows that) the "sched: fix 
smpnice abnormal nice anomalies" fixes the imbalance problem and the 
consequent CPU hopping.

BUT I still think that this patch (modified if necessary to handle any 
HT special cases) should be applied.  On a normal system, it will (as 
I've already said) stop active_load_balance() from doing a lot of 
unnecessary work INCLUDING holding the run queue locks for TWO run 
queues for no good reason.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH] Fix smpnice high priority task hopping problem

[Siddha, Suresh B]

On Fri, Feb 17, 2006 at 01:30:43PM +1100, Peter Williams wrote:
> On a normal system, would either of them be moved anyway?

Possible. Because when the migration thread runs it moves the current running
task out of the processor and the checks in can_migrate_task() like
"sd->nr_balance_failed > sd->cache_nice_tries" can result in cache hot task
move to the idle package.. This is a round about way and we should not depend
on this behavior..

> 
> > 
> > To fix my reported problem, we need to make sure that find_busiest_group()
> > doesn't find an imbalance..
> 
> I disagree.  If this causes a problem with your "optimizations" then I 
> think that you need to fix the "optimizations".
> 
> There's a rational argument (IMHO) that this patch should be applied 
> even in the absence of the smpnice patches as it prevents 
> active_load_balance() doing unnecessary work.  If this isn't good for 
> hypo threading then hypo threading is a special case and needs to handle 
> it as such.

active load balance is designed only with HT optimizations in mind. And now
multi-core optimizations also use this active load balance. No one else uses
active load balance.

thanks,
suresh

Re: [PATCH] Fix smpnice high priority task hopping problem

[Siddha, Suresh B]

On Fri, Feb 17, 2006 at 01:51:46PM +1100, Peter Williams wrote:
> Peter Williams wrote:
> > There's a rational argument (IMHO) that this patch should be applied 
> > even in the absence of the smpnice patches as it prevents 
> > active_load_balance() doing unnecessary work.  If this isn't good for 
> > hypo threading then hypo threading is a special case and needs to handle 
> > it as such.
> 
> OK.  The good news is that (my testing shows that) the "sched: fix 
> smpnice abnormal nice anomalies" fixes the imbalance problem and the 
> consequent CPU hopping.

Thats because find_busiest_group() is no longer showing the imbalance :)
Anyhow if I get time I will review this patch before I start my vacation.
Otherwise I assume Nick and Ingo will review this closely..

> BUT I still think that this patch (modified if necessary to handle any 
> HT special cases) should be applied.  On a normal system, it will (as 
> I've already said) stop active_load_balance() from doing a lot of 
> unnecessary work INCLUDING holding the run queue locks for TWO run 
> queues for no good reason.

Please see my earlier response to this..

thanks,
suresh

Re: [PATCH] Fix smpnice high priority task hopping problem

[Peter Williams]

Siddha, Suresh B wrote:
> On Fri, Feb 17, 2006 at 01:30:43PM +1100, Peter Williams wrote:
> 
>>On a normal system, would either of them be moved anyway?
> 
> 
> Possible. Because when the migration thread runs it moves the current running
> task out of the processor and the checks in can_migrate_task() like
> "sd->nr_balance_failed > sd->cache_nice_tries" can result in cache hot task
> move to the idle package.. This is a round about way and we should not depend
> on this behavior..

So why does it need to be retained?

> 
> 
>>>To fix my reported problem, we need to make sure that find_busiest_group()
>>>doesn't find an imbalance..
>>
>>I disagree.  If this causes a problem with your "optimizations" then I 
>>think that you need to fix the "optimizations".
>>
>>There's a rational argument (IMHO) that this patch should be applied 
>>even in the absence of the smpnice patches as it prevents 
>>active_load_balance() doing unnecessary work.  If this isn't good for 
>>hypo threading then hypo threading is a special case and needs to handle 
>>it as such.
> 
> 
> active load balance is designed only with HT optimizations in mind. And now
> multi-core optimizations also use this active load balance. No one else uses
> active load balance.

I can see nothing in the source code that will cause 
active_load_balance() to be only run on hypo threaded systems.  Could 
you please provide some pointers to the mechanism that does this.

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: [PATCH] Fix smpnice high priority task hopping problem

[Peter Williams]

Siddha, Suresh B wrote:
> On Fri, Feb 17, 2006 at 01:51:46PM +1100, Peter Williams wrote:
> 
>>Peter Williams wrote:
>>
>>>There's a rational argument (IMHO) that this patch should be applied 
>>>even in the absence of the smpnice patches as it prevents 
>>>active_load_balance() doing unnecessary work.  If this isn't good for 
>>>hypo threading then hypo threading is a special case and needs to handle 
>>>it as such.
>>
>>OK.  The good news is that (my testing shows that) the "sched: fix 
>>smpnice abnormal nice anomalies" fixes the imbalance problem and the 
>>consequent CPU hopping.
> 
> 
> Thats because find_busiest_group() is no longer showing the imbalance :)
> Anyhow if I get time I will review this patch before I start my vacation.
> Otherwise I assume Nick and Ingo will review this closely..
> 
> 
>>BUT I still think that this patch (modified if necessary to handle any 
>>HT special cases) should be applied.  On a normal system, it will (as 
>>I've already said) stop active_load_balance() from doing a lot of 
>>unnecessary work INCLUDING holding the run queue locks for TWO run 
>>queues for no good reason.
> 
> 
> Please see my earlier response to this..

I saw nothing there to convince me that this patch isn't worthwhile. 
Perhaps a better explanation would help me?

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce