[patch] sched: fix active load balance

[patch] sched: fix active load balance

[Siddha, Suresh B]

Recent changes to active load balance (sched2-fix-smt-scheduling-problems.patch
in -mm) is not resulting in any task movement from busiest to target_cpu.
Attached patch(ontop of -mm) fixes this issue.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>

--- linux-2.6.12-rc2-mm3/kernel/sched.c	2005-04-11 23:08:30.875103512 -0700
+++ linux/kernel/sched.c	2005-04-13 10:44:37.400829992 -0700
@@ -2131,7 +2131,7 @@
  */
 static void active_load_balance(runqueue_t *busiest_rq, int busiest_cpu)
 {
-	struct sched_domain *tmp = NULL, *sd;
+	struct sched_domain *sd;
 	runqueue_t *target_rq;
 	int target_cpu = busiest_rq->push_cpu;
 
@@ -2152,13 +2152,10 @@
 	double_lock_balance(busiest_rq, target_rq);
 
 	/* Search for an sd spanning us and the target CPU. */
-	for_each_domain(target_cpu, sd) {
+	for_each_domain(target_cpu, sd)
 		if ((sd->flags & SD_LOAD_BALANCE) &&
-			cpu_isset(busiest_cpu, sd->span)) {
-				sd = tmp;
+			cpu_isset(busiest_cpu, sd->span))
 				break;
-		}
-	}
 
 	if (unlikely(sd == NULL))
 		goto out;

Re: [patch] sched: fix active load balance

[Ingo Molnar]

* Siddha, Suresh B <suresh.b.siddha@intel.com> wrote:

> -	for_each_domain(target_cpu, sd) {
> +	for_each_domain(target_cpu, sd)
>  		if ((sd->flags & SD_LOAD_BALANCE) &&
> -			cpu_isset(busiest_cpu, sd->span)) {
> -				sd = tmp;
> +			cpu_isset(busiest_cpu, sd->span))
>  				break;
> -		}
> -	}

hm, the right fix i think is to do:

 	for_each_domain(target_cpu, tmp) {
  		if ((tmp->flags & SD_LOAD_BALANCE) &&
 			cpu_isset(busiest_cpu, tmp->span)) {
 				sd = tmp;
  				break;
 		}
 	}

because when balancing we want to match the widest-scope domain, not the 
first one. The s/tmp/sd thing was a typo i suspect.

	Ingo

Re: [patch] sched: fix active load balance

[Siddha, Suresh B]

On Wed, Apr 13, 2005 at 10:08:28PM +0200, Ingo Molnar wrote:
> 
> * Siddha, Suresh B <suresh.b.siddha@intel.com> wrote:
> 
> > -	for_each_domain(target_cpu, sd) {
> > +	for_each_domain(target_cpu, sd)
> >  		if ((sd->flags & SD_LOAD_BALANCE) &&
> > -			cpu_isset(busiest_cpu, sd->span)) {
> > -				sd = tmp;
> > +			cpu_isset(busiest_cpu, sd->span))
> >  				break;
> > -		}
> > -	}
> 
> hm, the right fix i think is to do:
> 
>  	for_each_domain(target_cpu, tmp) {
>   		if ((tmp->flags & SD_LOAD_BALANCE) &&
>  			cpu_isset(busiest_cpu, tmp->span)) {
>  				sd = tmp;
>   				break;
>  		}
>  	}

Your suggestion also looks similar to my patch. You are also breaking on the 
first one.

> because when balancing we want to match the widest-scope domain, not the 
> first one.

We want the first domain spanning both the cpu's. That is the domain where
normal load balance failed and we restore to active load balance.

thanks,
suresh

Re: [patch] sched: fix active load balance

[Nick Piggin]

Siddha, Suresh B wrote:
> On Wed, Apr 13, 2005 at 10:08:28PM +0200, Ingo Molnar wrote:
> 
>>* Siddha, Suresh B <suresh.b.siddha@intel.com> wrote:
>>
>>
>>>-	for_each_domain(target_cpu, sd) {
>>>+	for_each_domain(target_cpu, sd)
>>> 		if ((sd->flags & SD_LOAD_BALANCE) &&
>>>-			cpu_isset(busiest_cpu, sd->span)) {
>>>-				sd = tmp;
>>>+			cpu_isset(busiest_cpu, sd->span))
>>> 				break;
>>>-		}
>>>-	}
>>

Yep that was broken :(
Thanks for picking that up Suresh.

>>hm, the right fix i think is to do:
>>
>> 	for_each_domain(target_cpu, tmp) {
>>  		if ((tmp->flags & SD_LOAD_BALANCE) &&
>> 			cpu_isset(busiest_cpu, tmp->span)) {
>> 				sd = tmp;
>>  				break;
>> 		}
>> 	}
> 
> 
> Your suggestion also looks similar to my patch. You are also breaking on the 
> first one.
> 
> 
>>because when balancing we want to match the widest-scope domain, not the 
>>first one.
> 
> 
> We want the first domain spanning both the cpu's. That is the domain where
> normal load balance failed and we restore to active load balance.
> 

Yes, that's right.

-- 
SUSE Labs, Novell Inc.

Re: [patch] sched: fix active load balance

[Ingo Molnar]

* Siddha, Suresh B <suresh.b.siddha@intel.com> wrote:

> Your suggestion also looks similar to my patch. You are also breaking 
> on the first one.

yeah.

> We want the first domain spanning both the cpu's. That is the domain 
> where normal load balance failed and we restore to active load 
> balance.

indeed.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo