MIME-Version: 1.0
In-Reply-To: <20150206160934.0663505e@gandalf.local.home>
References: <1423151974-22557-1-git-send-email-xlpang@126.com>
	<20150206160934.0663505e@gandalf.local.home>
Date: Sun, 8 Feb 2015 22:55:09 +0800
Message-ID: <CADcy93WObaU75S1CbsHQQ9=rAXZjRUSrFjWOGaG31ULnBXn-Ng@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] sched/rt: Check to push the task when changing its affinity
From: Xunlei Pang <pang.xunlei@linaro.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Xunlei Pang <xlpang@126.com>, lkml <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@gmail.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4023
Lines: 122

Hi Steve,

On 7 February 2015 at 05:09, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu,  5 Feb 2015 23:59:33 +0800
>> +
>> +             if (task_running(rq, p) &&
>> +                 cpumask_test_cpu(task_cpu(p), new_mask) &&
>
> Why the check for task_cpu being in new_mask?

If the current cpu of this task is not in the new_mask,
it will get migrated by set_cpus_allowed_ptr(), so we
don't need to resched.

>
>> +                 cpupri_find(&rq->rd->cpupri, p, NULL)) {
>> +                     /*
>> +                      * At this point, current task gets migratable most
>> +                      * likely due to the change of its affinity, let's
>> +                      * figure out if we can migrate it.
>> +                      *
>> +                      * Is there any task with the same priority as that
>> +                      * of current task? If found one, we should resched.
>> +                      * NOTE: The target may be unpushable.
>> +                      */
>> +                     if (p->prio == rq->rt.highest_prio.next) {
>> +                             /* One target just in pushable_tasks list. */
>> +                             requeue_task_rt(rq, p, 0);
>> +                             preempt_push = 1;
>> +                     } else if (rq->rt.rt_nr_total > 1) {
>> +                             struct task_struct *next;
>> +
>> +                             requeue_task_rt(rq, p, 0);
>> +                             next = peek_next_task_rt(rq);
>> +                             if (next != p && next->prio == p->prio)
>> +                                     preempt_push = 1;
>> +                     }
>> +             } else if (!task_running(rq, p))
>> +                     direct_push = 1;
>
> We could avoid the second check (!task_running()) by splitting up the
> first if:

ok, I'll adjust it.

>
>         if (task_running(rq, p)) {
>                 if (cpumask_test_cpu() && cpupri_find()) {
>                 }
>         } else {
>                 direct push = 1
>
> Also, is the copy of cpus_allowed only done so that cpupri_find is
> called? If so maybe move it in there too:
>
>         if (task_running(rq, p)) {
>                 if (!cpumask_test_cpu())
>                         goto update;
>
>                 cpumask_copy(&p->cpus_allowed, new_mask);
>                 p->nr_cpus_allowed = new_weight;
>
>                 if (!cpupri_find())
>                         goto update;
>
>                 [...]
>
> This way we avoid the double copy of cpumask unless we truly need to do
> it.

The new_mask can also be used by direct_push case, so I think it's ok.

>
>> +     }
>>
>>       /*
>>        * Only update if the process changes its state from whether it
>>        * can migrate or not.
>>        */
>> -     if ((p->nr_cpus_allowed > 1) == (weight > 1))
>> -             return;
>> -
>> -     rq = task_rq(p);
>> +     if ((old_weight > 1) == (new_weight > 1))
>> +             goto out;
>>
>>       /*
>>        * The process used to be able to migrate OR it can now migrate
>>        */
>> -     if (weight <= 1) {
>> +     if (new_weight <= 1) {
>>               if (!task_current(rq, p))
>>                       dequeue_pushable_task(rq, p);
>>               BUG_ON(!rq->rt.rt_nr_migratory);
>> @@ -1919,6 +1970,15 @@ static void set_cpus_allowed_rt(struct task_struct *p,
>>       }
>>
>>       update_rt_migration(&rq->rt);
>> +
>> +out:
>> +     BUG_ON(direct_push == 1 && preempt_push == 1);
>
> Do we really need this bug on?
>
>> +
>> +     if (direct_push)
>> +             push_rt_tasks(rq);
>> +
>> +     if (preempt_push)
>
> We could make that an "else if" if they really are mutually exclusive.
>

I'll fix those things, and resend another version.

Thanks,
Xunlei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/