[PATCH] sched: fix evaluation of skip_for_load in move_tasks()

[PATCH] sched: fix evaluation of skip_for_load in move_tasks()

[Peter Williams]

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

Problem:

In the patch 
sched-avoid-unnecessarily-moving-highest-priority-task-move_tasks.patch
I got a the sense of a boolean expression wrong when assigning a value 
to skip_for_load.  The expression should have been negated before being 
assigned.

Additionally, busiest_best_prio_seen is being set when tasks are moved 
instead of when they are skipped which will cause problems when the 
current task does not have prio=busiest_best_prio.

Solution:

Negate the expression and apply de Marcos rule to simplify it and move 
the setting of busiest_best_prio_seen.

This patch is on top of 
sched-avoid-unnecessarily-moving-highest-priority-task-move_tasks.patch

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

-- 
Peter Williams                                   pwil3058@bigpond.net.au

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

[-- Attachment #2: smpnice-fix-busiest_best_prio_seen --]
[-- Type: text/plain, Size: 1526 bytes --]

Index: MM-2.6.17-rc1-mm3/kernel/sched.c
===================================================================
--- MM-2.6.17-rc1-mm3.orig/kernel/sched.c	2006-04-25 12:53:39.000000000 +1000
+++ MM-2.6.17-rc1-mm3/kernel/sched.c	2006-04-25 12:56:14.000000000 +1000
@@ -2059,7 +2059,10 @@ static int move_tasks(runqueue_t *this_r
 	busiest_best_prio = rq_best_prio(busiest);
 	/*
 	 * Enable handling of the case where there is more than one task
-	 * with the best priority.
+	 * with the best priority.   If the current running task is one
+	 * of those with prio==busiest_best_prio we know it won't be moved
+	 * and therefore it's safe to override the skip (based on load) of
+	 * any task we find with that prio.
 	 */
 	busiest_best_prio_seen = busiest_best_prio == busiest->curr->prio;
 
@@ -2108,9 +2111,10 @@ skip_queue:
 	 */
 	skip_for_load = tmp->load_weight > rem_load_move;
 	if (skip_for_load && idx < this_best_prio)
-		skip_for_load = busiest_best_prio_seen || idx != busiest_best_prio;
+		skip_for_load = !busiest_best_prio_seen && idx == busiest_best_prio;
 	if (skip_for_load ||
 	    !can_migrate_task(tmp, busiest, this_cpu, sd, idle, &pinned)) {
+		busiest_best_prio_seen |= idx == busiest_best_prio;
 		if (curr != head)
 			goto skip_queue;
 		idx++;
@@ -2133,8 +2137,6 @@ skip_queue:
 	if (pulled < max_nr_move && rem_load_move > 0) {
 		if (idx < this_best_prio)
 			this_best_prio = idx;
-		if (idx == busiest_best_prio)
-			busiest_best_prio_seen = 1;
 		if (curr != head)
 			goto skip_queue;
 		idx++;

Re: [PATCH] sched: fix evaluation of skip_for_load in move_tasks()

[Siddha, Suresh B]

On Tue, Apr 25, 2006 at 01:08:00PM +1000, Peter Williams wrote:
> Problem:
> 
> In the patch 
> sched-avoid-unnecessarily-moving-highest-priority-task-move_tasks.patch
> I got a the sense of a boolean expression wrong when assigning a value 
> to skip_for_load.  The expression should have been negated before being 
> assigned.
> 
> Additionally, busiest_best_prio_seen is being set when tasks are moved 
> instead of when they are skipped which will cause problems when the 
> current task does not have prio=busiest_best_prio.
> 
> Solution:
> 
> Negate the expression and apply de Marcos rule to simplify it and move 
> the setting of busiest_best_prio_seen.
> 
> This patch is on top of 
> sched-avoid-unnecessarily-moving-highest-priority-task-move_tasks.patch
> 
> Signed-off-by: Peter Williams <pwil3058@bigpond.com.au>
> 
> -- 
> Peter Williams                                   pwil3058@bigpond.net.au
> 
> "Learning, n. The kind of ignorance distinguishing the studious."
>   -- Ambrose Bierce

> Index: MM-2.6.17-rc1-mm3/kernel/sched.c
> ===================================================================
> --- MM-2.6.17-rc1-mm3.orig/kernel/sched.c	2006-04-25 12:53:39.000000000 +1000
> +++ MM-2.6.17-rc1-mm3/kernel/sched.c	2006-04-25 12:56:14.000000000 +1000
> @@ -2059,7 +2059,10 @@ static int move_tasks(runqueue_t *this_r
>  	busiest_best_prio = rq_best_prio(busiest);
>  	/*
>  	 * Enable handling of the case where there is more than one task
> -	 * with the best priority.
> +	 * with the best priority.   If the current running task is one
> +	 * of those with prio==busiest_best_prio we know it won't be moved
> +	 * and therefore it's safe to override the skip (based on load) of
> +	 * any task we find with that prio.
>  	 */
>  	busiest_best_prio_seen = busiest_best_prio == busiest->curr->prio;
>  
> @@ -2108,9 +2111,10 @@ skip_queue:
>  	 */
>  	skip_for_load = tmp->load_weight > rem_load_move;
>  	if (skip_for_load && idx < this_best_prio)
> -		skip_for_load = busiest_best_prio_seen || idx != busiest_best_prio;
> +		skip_for_load = !busiest_best_prio_seen && idx == busiest_best_prio;

I think we need to change this to
	if (skip_for_load && idx < this_best_prio && idx == busiest_best_prio)
		skip_for_load = !busiest_best_prio_seen;

Otherwise we will reset skip_for_load to '0' even for the tasks whose prio is 
less than this_best_prio but not equal to busiest_best_prio.

>  	if (skip_for_load ||
>  	    !can_migrate_task(tmp, busiest, this_cpu, sd, idle, &pinned)) {
> +		busiest_best_prio_seen |= idx == busiest_best_prio;
>  		if (curr != head)
>  			goto skip_queue;
>  		idx++;
> @@ -2133,8 +2137,6 @@ skip_queue:
>  	if (pulled < max_nr_move && rem_load_move > 0) {
>  		if (idx < this_best_prio)
>  			this_best_prio = idx;
> -		if (idx == busiest_best_prio)
> -			busiest_best_prio_seen = 1;
>  		if (curr != head)
>  			goto skip_queue;
>  		idx++;

Re: [PATCH] sched: fix evaluation of skip_for_load in move_tasks()

[Peter Williams]

Siddha, Suresh B wrote:
> On Tue, Apr 25, 2006 at 01:08:00PM +1000, Peter Williams wrote:
>> Problem:
>>
>> In the patch 
>> sched-avoid-unnecessarily-moving-highest-priority-task-move_tasks.patch
>> I got a the sense of a boolean expression wrong when assigning a value 
>> to skip_for_load.  The expression should have been negated before being 
>> assigned.
>>
>> Additionally, busiest_best_prio_seen is being set when tasks are moved 
>> instead of when they are skipped which will cause problems when the 
>> current task does not have prio=busiest_best_prio.
>>
>> Solution:
>>
>> Negate the expression and apply de Marcos rule to simplify it and move 
>> the setting of busiest_best_prio_seen.
>>
>> This patch is on top of 
>> sched-avoid-unnecessarily-moving-highest-priority-task-move_tasks.patch
>>
>> Signed-off-by: Peter Williams <pwil3058@bigpond.com.au>
>>
>> -- 
>> Peter Williams                                   pwil3058@bigpond.net.au
>>
>> "Learning, n. The kind of ignorance distinguishing the studious."
>>   -- Ambrose Bierce
> 
>> Index: MM-2.6.17-rc1-mm3/kernel/sched.c
>> ===================================================================
>> --- MM-2.6.17-rc1-mm3.orig/kernel/sched.c	2006-04-25 12:53:39.000000000 +1000
>> +++ MM-2.6.17-rc1-mm3/kernel/sched.c	2006-04-25 12:56:14.000000000 +1000
>> @@ -2059,7 +2059,10 @@ static int move_tasks(runqueue_t *this_r
>>  	busiest_best_prio = rq_best_prio(busiest);
>>  	/*
>>  	 * Enable handling of the case where there is more than one task
>> -	 * with the best priority.
>> +	 * with the best priority.   If the current running task is one
>> +	 * of those with prio==busiest_best_prio we know it won't be moved
>> +	 * and therefore it's safe to override the skip (based on load) of
>> +	 * any task we find with that prio.
>>  	 */
>>  	busiest_best_prio_seen = busiest_best_prio == busiest->curr->prio;
>>  
>> @@ -2108,9 +2111,10 @@ skip_queue:
>>  	 */
>>  	skip_for_load = tmp->load_weight > rem_load_move;
>>  	if (skip_for_load && idx < this_best_prio)
>> -		skip_for_load = busiest_best_prio_seen || idx != busiest_best_prio;
>> +		skip_for_load = !busiest_best_prio_seen && idx == busiest_best_prio;
> 
> I think we need to change this to
> 	if (skip_for_load && idx < this_best_prio && idx == busiest_best_prio)
> 		skip_for_load = !busiest_best_prio_seen;
> 
> Otherwise we will reset skip_for_load to '0' even for the tasks whose prio is 
> less than this_best_prio but not equal to busiest_best_prio.

And why is that a problem?  The intention of this code is to make sure 
at least one busiest_best_prio task doesn't get moved as a result of the 
"skip for reasons of load weight" mechanism being overridden by the "idx 
< this_best_prio" exception.  I can't see how this intention is being 
subverted.

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

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

Re: [PATCH] sched: fix evaluation of skip_for_load in move_tasks()

[Siddha, Suresh B]

On Wed, Apr 26, 2006 at 09:23:32AM +1000, Peter Williams wrote:
> Siddha, Suresh B wrote:
> > I think we need to change this to
> > 	if (skip_for_load && idx < this_best_prio && idx == busiest_best_prio)
> > 		skip_for_load = !busiest_best_prio_seen;
> > 
> > Otherwise we will reset skip_for_load to '0' even for the tasks whose prio is 
> > less than this_best_prio but not equal to busiest_best_prio.
> 
> And why is that a problem?  The intention of this code is to make sure 
> at least one busiest_best_prio task doesn't get moved as a result of the 
> "skip for reasons of load weight" mechanism being overridden by the "idx 
> < this_best_prio" exception.  I can't see how this intention is being 
> subverted.

There might be scenarios where we will endup moving other priority tasks(
not those with busiest_best_prio) which will still become highest priority
on new queue. This may or may not be bad. But this was not our intention
with the intended code, right?

thanks,
suresh

Re: [PATCH] sched: fix evaluation of skip_for_load in move_tasks()

[Peter Williams]

Siddha, Suresh B wrote:
> On Wed, Apr 26, 2006 at 09:23:32AM +1000, Peter Williams wrote:
>> Siddha, Suresh B wrote:
>>> I think we need to change this to
>>> 	if (skip_for_load && idx < this_best_prio && idx == busiest_best_prio)
>>> 		skip_for_load = !busiest_best_prio_seen;
>>>
>>> Otherwise we will reset skip_for_load to '0' even for the tasks whose prio is 
>>> less than this_best_prio but not equal to busiest_best_prio.
>> And why is that a problem?  The intention of this code is to make sure 
>> at least one busiest_best_prio task doesn't get moved as a result of the 
>> "skip for reasons of load weight" mechanism being overridden by the "idx 
>> < this_best_prio" exception.  I can't see how this intention is being 
>> subverted.
> 
> There might be scenarios where we will endup moving other priority tasks(
> not those with busiest_best_prio) which will still become highest priority
> on new queue. This may or may not be bad. But this was not our intention
> with the intended code, right?

I considered this (as part of the original code to allow override of the 
skip for cases where moving a task will make it the best priority on the 
CPU -- NB no extra caveats about finding the best priority task which 
meets the criteria) and decided that it wasn't worth worrying about as 
the complexity of the code required to handle it would be considerable.

Also, because we are actually moving a bigger load than is required to 
balance the total weighted loads on the two queues, there's an argument 
that we should move the smallest one that meets the requirement.  With 
these conflicting arguments, it seems best just to move the first one we 
find that meets the criteria and is movable.

Load balancing is probabilistic at best and extra effort trying to be 
perfect will be wasted.  This is even more the case when you take into 
account the fact that as soon as you release the locks on the queues 
everything is highly likely to change anyway.

I figured that this code is consistent with the original load balancing 
code in this regard.

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

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