From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752272AbbBDDNq (ORCPT ); Tue, 3 Feb 2015 22:13:46 -0500 Received: from cdptpa-outbound-snat.email.rr.com ([107.14.166.226]:35561 "EHLO cdptpa-oedge-vip.email.rr.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750944AbbBDDNo (ORCPT ); Tue, 3 Feb 2015 22:13:44 -0500 Date: Tue, 3 Feb 2015 22:14:19 -0500 From: Steven Rostedt To: Xunlei Pang Cc: linux-kernel@vger.kernel.org, Peter Zijlstra , Juri Lelli , Xunlei Pang Subject: Re: [PATCH RESEND 1/2] sched/rt: Check to push the task when changing its affinity Message-ID: <20150203221419.4428225e@grimm.local.home> In-Reply-To: <1423012341-30265-1-git-send-email-xlpang@126.com> References: <1423012341-30265-1-git-send-email-xlpang@126.com> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-RR-Connecting-IP: 107.14.168.130:25 X-Cloudmark-Score: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 4 Feb 2015 09:12:20 +0800 Xunlei Pang wrote: > From: Xunlei Pang > > We may suffer from extra rt overload rq due to the affinity, > so when the affinity of any runnable rt task is changed, we > should check to trigger balancing, otherwise it will cause > some unnecessary delayed real-time response. Unfortunately, > current RT global scheduler doesn't trigger anything. > > For example: a 2-cpu system with two runnable FIFO tasks(same > rt_priority) bound on CPU0, let's name them rt1(running) and > rt2(runnable) respectively; CPU1 has no RTs. Then, someone sets > the affinity of rt2 to 0x3(i.e. CPU0 and CPU1), but after this, > rt2 still can't be scheduled until rt1 enters schedule(), this > definitely causes some/big response latency for rt2. > I understand the issue you point out, but I have to be honest and say that I really do not like this approach. > So, when doing set_cpus_allowed_rt(), if detecting such cases, > check to trigger a push behaviour. > > Signed-off-by: Xunlei Pang > --- > kernel/sched/rt.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 59 insertions(+), 10 deletions(-) > > diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c > index f4d4b07..4dacb6e 100644 > --- a/kernel/sched/rt.c > +++ b/kernel/sched/rt.c > @@ -1428,7 +1428,7 @@ static struct sched_rt_entity *pick_next_rt_entity(struct rq *rq, > return next; > } > > -static struct task_struct *_pick_next_task_rt(struct rq *rq) > +static struct task_struct *_pick_next_task_rt(struct rq *rq, int peek_only) > { peek_only should be bool, but don't worry about it, I think this isn't needed. > struct sched_rt_entity *rt_se; > struct task_struct *p; > @@ -1441,7 +1441,8 @@ static struct task_struct *_pick_next_task_rt(struct rq *rq) > } while (rt_rq); > > p = rt_task_of(rt_se); > - p->se.exec_start = rq_clock_task(rq); > + if (!peek_only) > + p->se.exec_start = rq_clock_task(rq); > > return p; > } > @@ -1476,7 +1477,7 @@ pick_next_task_rt(struct rq *rq, struct task_struct *prev) > > put_prev_task(rq, prev); > > - p = _pick_next_task_rt(rq); > + p = _pick_next_task_rt(rq, 0); > > /* The running task is never eligible for pushing */ > dequeue_pushable_task(rq, p); > @@ -1886,28 +1887,69 @@ static void set_cpus_allowed_rt(struct task_struct *p, > const struct cpumask *new_mask) > { > struct rq *rq; > - int weight; > + int old_weight, new_weight; > + int preempt_push = 0, direct_push = 0; > > BUG_ON(!rt_task(p)); > > if (!task_on_rq_queued(p)) > return; > > - weight = cpumask_weight(new_mask); > + old_weight = p->nr_cpus_allowed; > + new_weight = cpumask_weight(new_mask); > + > + rq = task_rq(p); > + > + if (new_weight > 1 && > + rt_task(rq->curr) && > + !test_tsk_need_resched(rq->curr)) { > + /* > + * Set new mask information to prepare pushing. > + * It's safe to do this here. Please explain why it is safe. > + */ > + 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. > + /* > + * 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. > + } 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. -- Steve > } > > /* Assumes rq->lock is held */