lkml.org 
[lkml]   [2015]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RESEND v4 2/3] sched/rt: Fix wrong SMP scheduler behavior for equal prio cases
    On Mon,  9 Mar 2015 15:32:27 +0800
    Xunlei Pang <xlpang@126.com> wrote:

    > From: Xunlei Pang <pang.xunlei@linaro.org>
    >
    > Currently, SMP RT scheduler has some trouble in dealing with
    > equal prio cases.
    >
    > For example, in check_preempt_equal_prio():
    > When RT1(current task) gets preempted by RT2, if there is a
    > migratable RT3 with same prio, RT3 will be pushed away instead
    > of RT1 afterwards, because RT1 will be enqueued to the tail of
    > the pushable list when going through succeeding put_prev_task_rt()
    > triggered by resched. This broke FIFO.
    >
    > Furthermore, this is also problematic for normal preempted cases
    > if there're some rt tasks queued with the same prio as current.
    > Because current will be put behind these tasks in the pushable
    > queue.
    >
    > So, if a task is running and gets preempted by a higher priority
    > task (or even with same priority for migrating), this patch ensures
    > that it is put ahead of any existing task with the same priority in
    > the pushable queue.
    >
    > Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
    > ---
    > kernel/sched/rt.c | 23 ++++++++++++++++-------
    > 1 file changed, 16 insertions(+), 7 deletions(-)
    >
    > diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
    > index f4d4b07..86cd79f 100644
    > --- a/kernel/sched/rt.c
    > +++ b/kernel/sched/rt.c
    > @@ -347,11 +347,15 @@ static inline void set_post_schedule(struct rq *rq)
    > rq->post_schedule = has_pushable_tasks(rq);
    > }
    >
    > -static void enqueue_pushable_task(struct rq *rq, struct task_struct *p)
    > +static void enqueue_pushable_task(struct rq *rq,
    > + struct task_struct *p, bool head)

    Nit.

    static void
    enqueue_pushable_task(struct rq *rq, struct task_struct *p, bool head)

    Is a better breaking of the line.

    > {
    > plist_del(&p->pushable_tasks, &rq->rt.pushable_tasks);
    > plist_node_init(&p->pushable_tasks, p->prio);
    > - plist_add(&p->pushable_tasks, &rq->rt.pushable_tasks);
    > + if (head)
    > + plist_add_head(&p->pushable_tasks, &rq->rt.pushable_tasks);
    > + else
    > + plist_add_tail(&p->pushable_tasks, &rq->rt.pushable_tasks);
    >
    > /* Update the highest prio pushable task */
    > if (p->prio < rq->rt.highest_prio.next)
    > @@ -373,7 +377,8 @@ static void dequeue_pushable_task(struct rq *rq, struct task_struct *p)
    >
    > #else
    >
    > -static inline void enqueue_pushable_task(struct rq *rq, struct task_struct *p)
    > +static inline void enqueue_pushable_task(struct rq *rq,
    > + struct task_struct *p, bool head)

    Same here.

    > {
    > }
    >
    > @@ -1248,7 +1253,7 @@ enqueue_task_rt(struct rq *rq, struct task_struct *p, int flags)
    > enqueue_rt_entity(rt_se, flags & ENQUEUE_HEAD);
    >
    > if (!task_current(rq, p) && p->nr_cpus_allowed > 1)
    > - enqueue_pushable_task(rq, p);
    > + enqueue_pushable_task(rq, p, false);
    > }
    >
    > static void dequeue_task_rt(struct rq *rq, struct task_struct *p, int flags)
    > @@ -1494,8 +1499,12 @@ static void put_prev_task_rt(struct rq *rq, struct task_struct *p)
    > * The previous task needs to be made eligible for pushing
    > * if it is still active
    > */
    > - if (on_rt_rq(&p->rt) && p->nr_cpus_allowed > 1)
    > - enqueue_pushable_task(rq, p);
    > + if (on_rt_rq(&p->rt) && p->nr_cpus_allowed > 1) {
    > + if (task_running(rq, p) && (preempt_count() & PREEMPT_ACTIVE))

    put_prev_task_rt() is called by put_prev_task() which is called by
    several functions: rt_mutex_setprio(), __sched_setscheduler(),
    sched_setnuma(), migrate_tasks(), and sched_move_task(). It's not part
    of being preempted.

    Now it is also called by pick_next_task_rt() which I'm assuming is what
    you want it to affect.

    The above definitely needs a comment about what it is doing. Also, I'm
    not so sure we care about testing task_running(). I'm thinking the
    check for PREEMPT_ACTIVE is good enough, as that would only be set from
    being called within preempt_schedule().

    Also, we could get rid of the if statement and do:

    enqueue_pushable_task(rq, p, !!(preempt_count() & PREEMPT_ACTIVE));


    -- Steve

    > + enqueue_pushable_task(rq, p, true);
    > + else
    > + enqueue_pushable_task(rq, p, false);
    > + }
    > }
    >
    > #ifdef CONFIG_SMP
    > @@ -1914,7 +1923,7 @@ static void set_cpus_allowed_rt(struct task_struct *p,
    > rq->rt.rt_nr_migratory--;
    > } else {
    > if (!task_current(rq, p))
    > - enqueue_pushable_task(rq, p);
    > + enqueue_pushable_task(rq, p, false);
    > rq->rt.rt_nr_migratory++;
    > }
    >



    \
     
     \ /
      Last update: 2015-03-27 16:41    [W:5.862 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site