Re: [PATCH 1/2] sched: push rt tasks only if newly activated tasks have been added

["Dmitry Adamushko" <dmitry.adamushko@gmail.com>]

2008/4/23 Dmitry Adamushko <dmitry.adamushko@gmail.com>:
> 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);
 }


-- 
Best regards,
Dmitry Adamushko