Re: [PATCH] sched: Avoid unnecessarily moving highest priority task move_tasks()

[Siddha, Suresh B]

On Tue, Apr 25, 2006 at 09:04:41AM +1000, Peter Williams wrote:
> Looks like there's a NOT missing doesn't it.  So there is an error but I 
> don't think that your patch is the right way to fix it.  We just need to 
> negate the above assignment.  E.g.
> 
> skip_for_load = !(busiest_best_prio_seen || idx != busiest_best_prio);
> 
> or
> 
> skip_for_load = !busiest_best_prio_seen && idx == busiest_best_prio;
> 
> whichever is more efficient.

That just will not be enough. busiest_best_prio needs to be set to '1'
whenever we skip the first best priority task(not whenever we move it)

And we also need to initialize busiest_best_prio_seen inside this check.
(like in my patch)
	if (busiest->expired->nr_active) {

And we need to reset busiest_best_prio_seen to '0' whenever we finished
the checking of expired list (and move onto active list) and there are
no best prio tasks on expired list..

> > @@ -2072,6 +2067,13 @@ static int move_tasks(runqueue_t *this_r
> >  	if (busiest->expired->nr_active) {
> >  		array = busiest->expired;
> >  		dst_array = this_rq->expired;
> > +		/*
> > +		 * We already have one or more busiest best prio tasks on
> > +		 * active list.
> 
> This is a pretty bold assertion.  How do we know that this is true.

That comment refers to when 'busiest_best_prio_seen' is initialized to '1'.
Comment needs to be fixed.

thanks,
suresh

> 
> > So if we encounter any busiest best prio task
> > +		 * on expired list, consider it for the move, if it becomes
> > +		 * the best prio on new queue.
> > +		 */
> > +		busiest_best_prio_seen = busiest_best_prio == busiest->curr->prio;
> >  	} else {
> >  		array = busiest->active;
> >  		dst_array = this_rq->active;
> > @@ -2089,6 +2091,7 @@ skip_bitmap:
> >  		if (array == busiest->expired && busiest->active->nr_active) {
> >  			array = busiest->active;
> >  			dst_array = this_rq->active;
> > +			busiest_best_prio_seen = 0;
> >  			goto new_array;
> >  		}
> >  		goto out;
> > @@ -2107,8 +2110,9 @@ skip_queue:
> >  	 * prio value) on its new queue regardless of its load weight
> >  	 */
> >  	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;
> > +	if (skip_for_load && idx < this_best_prio && idx == busiest_best_prio)
> > +		skip_for_load = !busiest_best_prio_seen &&
> > +				head->next == head->prev;
> >  	if (skip_for_load ||
> >  	    !can_migrate_task(tmp, busiest, this_cpu, sd, idle, &pinned)) {
> >  		if (curr != head)
> > @@ -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++;
> 
> 
> Peter
> -- 
> Peter Williams                                   pwil3058@xxxxxxxxxxxxxx
> 
> "Learning, n. The kind of ignorance distinguishing the studious."
>   -- Ambrose Bierce
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/