lkml.org 
[lkml]   [2012]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH]sched_rt.c: Avoid unnecessary dequeue and enqueue of pushable tasks in set_cpus_allowed_rt()
    From
    Date
    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 */




    \
     
     \ /
      Last update: 2012-04-10 17:55    [W:2.525 / U:0.268 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site