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

From: Mathieu Desnoyers
Date: Tue Sep 20 2016 - 18:10:55 EST


----- 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