LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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>
Subject: Re: [PATCH RESEND 1/2] sched/rt: Check to push the task when changing its affinity
Date: Wed, 4 Feb 2015 20:59:49 +0800	[thread overview]
Message-ID: <CADcy93U3t8acuA33QxBdwBhby=Ya1uKNA6+BMeSCBeShLywAtw@mail.gmail.com> (raw)
In-Reply-To: <20150203221419.4428225e@grimm.local.home>

Hi Steve,

On 4 February 2015 at 11:14, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed,  4 Feb 2015 09:12:20 +0800
> Xunlei Pang <xlpang@126.com> wrote:
>
>> From: Xunlei Pang <pang.xunlei@linaro.org>
>>
>> +              */
>> +             cpumask_copy(&p->cpus_allowed, new_mask);
>> +             p->nr_cpus_allowed = new_weight;
>> +
>> +             if (task_running(rq, p) &&
>> +                 cpumask_test_cpu(task_cpu(p), new_mask) &&
>> +                 cpupri_find(&rq->rd->cpupri, p, NULL)) {
>
> Hmm, You called cpupri_find() which should also return a mask of the
> CPUs with the lowest priorities. I wonder if we could have utilize this
> information instead of doing it twice? Of course things could change by
> the time the task migrates.

We do this if the target task is running, so we can migrate it only
by resched_cur() or stop_one_cpu(), that's what I can think of now :)
I think resched_cur() would be better.

>
>> +                     /*
>> +                      * 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);
>> +                             /* peek only */
>> +                             next = _pick_next_task_rt(rq, 1);
>> +                             if (next != p && next->prio == p->prio)
>> +                                     preempt_push = 1;
>> +                     }
>
> I'm thinking it would be better just to send an IPI to the CPU that
> figures this out and pushes a task off of itself.

My thought is that we try the best not to disturb the running task,
actually using direct push_rt_tasks() here instead of IPI is sort of
similar to that logic in task_woken_rt().

>
>> +             } else if (!task_running(rq, p))
>> +                     direct_push = 1;
>> +     }
>>
>>       /*
>>        * 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 +1961,13 @@ static void set_cpus_allowed_rt(struct task_struct *p,
>>       }
>>
>>       update_rt_migration(&rq->rt);
>> +
>> +out:
>> +     if (direct_push)
>> +             push_rt_tasks(rq);
>> +
>> +     if (preempt_push)
>> +             resched_curr(rq);
>
> I don't know. This just doesn't seem clean.
>

Thanks for your time, any of your suggestions would be helpful.

Regards,
Xunlei

      reply	other threads:[~2015-02-04 12:59 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-04  1:12 Xunlei Pang
2015-02-04  1:12 ` [PATCH RESEND 2/2] sched/rt: Add check_preempt_equal_prio() logic in pick_next_task_rt() Xunlei Pang
2015-02-04  3:17   ` Steven Rostedt
2015-02-04 13:05     ` Xunlei Pang
2015-02-04  3:14 ` [PATCH RESEND 1/2] sched/rt: Check to push the task when changing its affinity Steven Rostedt
2015-02-04 12:59   ` Xunlei Pang [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CADcy93U3t8acuA33QxBdwBhby=Ya1uKNA6+BMeSCBeShLywAtw@mail.gmail.com' \
    --to=pang.xunlei@linaro.org \
    --cc=juri.lelli@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=xlpang@126.com \
    --subject='Re: [PATCH RESEND 1/2] sched/rt: Check to push the task when changing its affinity' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).