Re: [PATCH]sched_rt.c: Avoid unnecessary dequeue and enqueue of pushable tasks in set_cpus_allowed_rt()

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 2012-02-19 at 18:17 +0400, Kirill Tkhai wrote:
> ---
> 
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 3640ebb..bf48343 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1779,43 +1779,36 @@ static void set_cpus_allowed_rt(struct task_struct *p,
>  				const struct cpumask *new_mask)
>  {
>  	int weight = cpumask_weight(new_mask);

Lets move the assignment of weight down. Gcc may optimize, but I don't
want to rely on it.

> +	struct rq *rq;
>  
>  	BUG_ON(!rt_task(p));
>  
>  	/*
> -	 * Update the migration status of the RQ if we have an RT task
> -	 * which is running AND changing its weight value.
> +	 * Just exit if it's not necessary to change migration status

Let's comment this better. Something like:

	Only update if the process changes its state from whether it
	can migrate or not.


>  	 */
> -	if (p->on_rq && (weight != p->rt.nr_cpus_allowed)) {
> -		struct rq *rq = task_rq(p);
> -
> -		if (!task_current(rq, p)) {
> -			/*
> -			 * Make sure we dequeue this task from the pushable list
> -			 * before going further.  It will either remain off of
> -			 * the list because we are no longer pushable, or it
> -			 * will be requeued.
> -			 */
> -			if (p->rt.nr_cpus_allowed > 1)
> -				dequeue_pushable_task(rq, p);
> -
> -			/*
> -			 * Requeue if our weight is changing and still > 1
> -			 */
> -			if (weight > 1)
> -				enqueue_pushable_task(rq, p);
> +	if ((p->rt.nr_cpus_allowed > 1) == (weight > 1))
> +		return;
>  
> -		}
> +	if (!p->on_rq)
> +		return;

Make the on_rq check first, and move the weight calculation below it.
Why calculate the weight if we don't plan on doing anything with it?

>  
> -		if ((p->rt.nr_cpus_allowed <= 1) && (weight > 1)) {
> -			rq->rt.rt_nr_migratory++;
> -		} else if ((p->rt.nr_cpus_allowed > 1) && (weight <= 1)) {
> -			BUG_ON(!rq->rt.rt_nr_migratory);
> -			rq->rt.rt_nr_migratory--;
> -		}
> +	rq = task_rq(p);
>  
> -		update_rt_migration(&rq->rt);
> +	/*
> +	 * Several cpus were allowed but now it's not so OR vice versa

I rather say:

	The process use to be able to migrate OR it can now migrate

Otherwise, the patch looks good.

Thanks,

-- Steve

P.S. I don't have any more trips in the near future, so I should be much
quicker in my responses ;-)



> +	 */
> +	if (weight <= 1) {
> +		if (!task_current(rq, p))
> +			dequeue_pushable_task(rq, p);
> +		BUG_ON(!rq->rt.rt_nr_migratory);
> +		rq->rt.rt_nr_migratory--;
> +	} else {
> +		if (!task_current(rq, p))
> +			enqueue_pushable_task(rq, p);
> +		rq->rt.rt_nr_migratory++;
>  	}
> +
> +	update_rt_migration(&rq->rt);
>  }
>  
>  /* Assumes rq->lock is held */