From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Gregory Haskins" Subject: Re: [PATCH 1/2] sched: push rt tasks only if newly activated tasks have been added Date: Wed, 23 Apr 2008 04:03:14 -0600 Message-ID: <480ED122.BA47.005A.0@novell.com> References: <20080421180747.4560.67794.stgit@novell1.haskins.net> <20080421181015.4560.7658.stgit@novell1.haskins.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 8BIT Cc: , , , , , , To: "Dmitry Adamushko" , "Steven Rostedt" Return-path: Received: from sinclair.provo.novell.com ([137.65.248.137]:46368 "EHLO sinclair.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760906AbYDWKCp convert rfc822-to-8bit (ORCPT ); Wed, 23 Apr 2008 06:02:45 -0400 In-Reply-To: Content-Disposition: inline Sender: linux-rt-users-owner@vger.kernel.org List-ID: >>> On Wed, Apr 23, 2008 at 5:53 AM, in message , "Dmitry Adamushko" wrote: > 2008/4/23 Dmitry Adamushko : >> Hi Steven, >> >> > [ ... ] >> >> > > square#0: >> > > >> > > cpu1: T0 is running >> > > >> > > T1 is of the same prio as T0 (shouldn't really matter but to get the >> > > same result it would require altering the flow of events slightly) >> > > >> > > T1's affinity allows it to be run only on cpu1. >> > > T0 can run on both. >> > > >> > > try_to_wake_up() is called for T1. >> > > | >> > > --> select_task_rq_rt() => gives cpu1 >> > > | >> > > --> task_wake_up_rt() >> > > | >> > > ---> push_rt_tasks() -> rq->rt.pushed = 1 >> > > >> > > now, neither T1 (due to its affinity), nor T0 (it's running) can be >> > > pushed away to cpu0. >> > >> > Ah, this may be what you are talking about. T0 was running, but because >> > T1 has its affinity set to cpu1 it wont cause a push. When T0 schedules >> > away to give T1 its cpu time, T0 wont push away because of the pushed >> > flag. >> > >> > Hmm, interesting. Of course my response is "Don't use SCHED_RR! It's >> > evil!" ;-) >> >> It's not just SCHED_RR ;-) They both can be of SCHED_FIFO. >> >> T1 _preempts_ T0 and again >> >> >> --> task_wake_up_rt() >> | >> ---> push_rt_tasks() -> rq->rt.pushed = 1 >> >> and T0 won't be pushed away to cpu0 by post_schedule_rt(). >> >> As Gregory has pointed out, at the very least it's a test in >> task_wake_up_rt() which is wrong. >> >> push_rt_tasks() should not be called when 'p' (a newly woken up task) >> is the next one to run. >> >> IOW, it should be (p->prio < rq->curr->prio) instead of (p->prio >= >> rq->rt.highest_prio). > > No, this argument is wrong indeed. > > Something like this: > (white-spaces are broken) > > --- sched_rt-prev.c 2008-04-23 11:26:39.000000000 +0200 > +++ sched_rt.c 2008-04-23 11:36:20.000000000 +0200 > @@ -1121,9 +1121,13 @@ static void post_schedule_rt(struct rq * > > static void task_wake_up_rt(struct rq *rq, struct task_struct *p) > { > - if (!task_running(rq, p) && > - (p->prio >= rq->rt.highest_prio) && > - rq->rt.overloaded) > + /* > + * Consider pushing 'p' off to other CPUS only > + * if it's not the next task to run on this CPU. > + */ > + if (rq->rt.overloaded && > + p->prio > rq->rt.highest_prio && > + pick_rt_task(rq, p, -1)) > push_rt_tasks(rq); > } > > > or even this (although, it's a bit heavier) > > --- sched_rt-prev.c 2008-04-23 11:26:39.000000000 +0200 > +++ sched_rt.c 2008-04-23 11:49:03.000000000 +0200 > @@ -1118,12 +1118,22 @@ static void post_schedule_rt(struct rq * > } > } > > static void task_wake_up_rt(struct rq *rq, struct task_struct *p) > { > - if (!task_running(rq, p) && > - (p->prio >= rq->rt.highest_prio) && > - rq->rt.overloaded) > + if (!rq->rt.overloaded) > + return; > + > + /* > + * Consider pushing 'p' off to other CPUS only > + * if it's not the next task to run on this CPU. > + * i.e. it's not a single task with the highest prio > + * on the queue. > + */ > + if (p->prio == rq->rt.highest_prio && > + p->rt.run_list.prev == p->rt.run_list.next) > + return; > + > + if (pick_rt_task(rq, p, -1)) > push_rt_tasks(rq); > } > I think we can simplify this further. We really only need to push here if we are not going to reschedule anytime soon (probably white-space damaged): --- a/kernel/sched_rt.c +++ b/kernel/sched_rt.c @@ -1058,11 +1058,14 @@ static void post_schedule_rt(struct rq *rq) } }