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

[Julien Desfossez]

On 21-Sep-2016 12:56:32 AM, Thomas Gleixner wrote:
> On Tue, 20 Sep 2016, Mathieu Desnoyers wrote:
> > 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.
> 
> Yes. Look at the call site of this. This is called in two cases:
> 
>  - The task blocked on a lock pushes the task owning the lock and in that
>    case it is guaranteed that the blocked task is in the same or in a
>    higher scheduling class. We have no support for inverse PI (yet).
> 
>  - The task which got boosted deboosts itself after releasing the lock. In
>    that case it falls back to its original class/priority/bandwidth.
> 
> Hope that helps.

Thanks for clarifying that, so indeed there is no risk of ambiguity here
between the scheduling class and the policy for fair tasks so this patch
is useless.

This was the only fix of the patchset, the other patches aim to extract
accurate scheduling informations in the trace.

> > >> 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.
> 
> So the SOB chain is wrong because you neither authored the patch nor you
> conveyed it.

My bad, sorry about that.

Thanks,

Julien