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

Steven Rostedt
2012-04-10T15:52:12Z

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 */