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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 20 Sep 2016 20:09:42 -0400
From:   Julien Desfossez <jdesfossez@...icios.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Peter Zijlstra <peterz@...radead.org>,
        rostedt <rostedt@...dmis.org>, Ingo Molnar <mingo@...hat.com>,
        daolivei <daolivei@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/6] sched/fair: check_preempt_wakeup: Fix assumption
 on the default policy

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@...radead.org>
> > >> Cc: Steven Rostedt (Red Hat) <rostedt@...dmis.org>
> > >> Cc: Thomas Gleixner <tglx@...utronix.de>
> > >> Cc: Ingo Molnar <mingo@...hat.com>
> > >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> > >> Signed-off-by: Julien Desfossez <jdesfossez@...icios.com>
> > > 
> > > 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