Re: [RFC PATCH 1/6] sched/fair: check_preempt_wakeup: Fix assumption on the default policy

[Mathieu Desnoyers]

----- On Sep 20, 2016, at 4:49 PM, Thomas Gleixner tglx@xxxxxxxxxxxxx wrote:

> On Fri, 16 Sep 2016, Julien Desfossez wrote:
> 
>> Tasks with RT or deadline scheduling class may inherit from a task with
>> a "fair" scheduling class.
> 
> This makes no sense. A RT/DL task can never inherit anything from a sched
> fair task. That would be inverted priority inheritance.
> 
>> This priority inheritance changes the scheduling class, but not the task
>> "policy" field.
>>
>> Therefore, the fair scheduler should not assume that policy !=
>> SCHED_NORMAL is the same as (policy == SCHED_BATCH || policy ==
>> SCHED_IDLE), because the policy could also be SCHED_RR, SCHED_FIFO, or
>> SCHED_DEADLINE.
>> 
>> The incorrect comparison in check_preempt_wakeup makes RR, FIFO and
>> DEADLINE tasks which inherit from a fair task behave as if they were
>> IDLE or BATCH tasks, thus awaiting the following tick before preempting
>> the current task.
> 
> This is just wrong.
> 
> Priority/deadline inheritance elevates a fair task to RR/FIFO/DL, i.e. to
> the scheduling class of the task which is blocked on a resource held by the
> fair task.
> 
> The check_preempt_curr() callback of a scheduling class is only invoked
> when the freshly woken task is in the same scheduling class as the task
> which is currently on the cpu.
> 
> So which problem are you actually solving?

So what is then puzzling us is this:

rt_mutex_setprio()

        if (dl_prio(prio)) {
                struct task_struct *pi_task = rt_mutex_get_top_task(p);
                if (!dl_prio(p->normal_prio) ||
                    (pi_task && dl_entity_preempt(&pi_task->dl, &p->dl))) {
                        p->dl.dl_boosted = 1;
                        queue_flag |= ENQUEUE_REPLENISH;
                } else
                        p->dl.dl_boosted = 0;
                p->sched_class = &dl_sched_class;
        } else if (rt_prio(prio)) {
                if (dl_prio(oldprio))
                        p->dl.dl_boosted = 0;
                if (oldprio < prio)
                        queue_flag |= ENQUEUE_HEAD;
                p->sched_class = &rt_sched_class;
        } else {
                if (dl_prio(oldprio))
                        p->dl.dl_boosted = 0;
                if (rt_prio(oldprio))
                        p->rt.timeout = 0;
                p->sched_class = &fair_sched_class;
        }

So in the 3rd block, this is the case where we inherit a
new prio which is neither LD nor RT, so it's "fair".

If we try to assign a fair prio to a task of DL or RT
prio, the dl_boosted is set to 0, or the rt timeout is
set to 0. However, we do change the sched_class of the
target task to &fair_sched_class.

This code path seems to imply that a RT or DL task can
change sched_class to "fair". Indeed, it makes no sense,
so I have the feeling we're missing something important
here.


> 
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Steven Rostedt (Red Hat) <rostedt@xxxxxxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
>> Signed-off-by: Julien Desfossez <jdesfossez@xxxxxxxxxxxx>
> 
> Who wrote the patch?

Julien is the author.

Thanks,

Mathieu

> 
> Thanks,
> 
> 	tglx

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com