linux-kernel - Re: [PATCH v3 4/6] sched/rt: fast switch to maximum frequency when RT tasks are scheduled

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171206113821.GO31247@e110439-lin>
Date:   Wed, 6 Dec 2017 11:38:21 +0000
From:   Patrick Bellasi <patrick.bellasi@....com>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Morten Rasmussen <morten.rasmussen@....com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Todd Kjos <tkjos@...roid.com>,
        Joel Fernandes <joelaf@...gle.com>
Subject: Re: [PATCH v3 4/6] sched/rt: fast switch to maximum frequency when
 RT tasks are scheduled

Hi Vincent,

On 06-Dec 10:39, Vincent Guittot wrote:
> Hi Patrick,
> 
> On 30 November 2017 at 12:47, Patrick Bellasi <patrick.bellasi@....com> wrote:

[...]

> >  static void dequeue_task_rt(struct rq *rq, struct task_struct *p, int flags)
> > @@ -1564,6 +1564,9 @@ pick_next_task_rt(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> >
> >         p = _pick_next_task_rt(rq);
> >
> > +       /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> 
> p is null when there is no rt task to pick.
> You should test this condition before calling cpufreq_update_util

Mmm... for what I see, from the above function's implementation,
_pick_next_task_rt() is always returning a valid pointer to an RT
task.

The above call does a:

   p->se.exec_start = rq_clock_task(rq);

right before returning, and there is also a BUG_ON(!rt_se) in the
previous RT tasks scanning loop.

Am I missing something?

> > +       cpufreq_update_util(rq, SCHED_CPUFREQ_RT);
> > +
> >         /* The running task is never eligible for pushing */
> >         dequeue_pushable_task(rq, p);

[...]

> > @@ -2317,6 +2323,9 @@ static void set_curr_task_rt(struct rq *rq)
> >
> >         p->se.exec_start = rq_clock_task(rq);
> >
> > +       /* Kick cpufreq (see the comment in kernel/sched/sched.h). */
> > +       cpufreq_update_util(rq, SCHED_CPUFREQ_RT);
> 
> Is this change linked to the "- when a task is set to be RT" in the
> commit message ?
> 
> I can't see a situation where this is call without the previous one.
> AFAICT, enqueue_task_rt will be called before each call to this
> function

Yeah, you right, in core.c the pattern seems to always be:

    if (queued)
            enqueue_task()
    if (running)
            set_curr_task()

I'll remove this chunk from the next version.

> 
> > +
> >         /* The running task is never eligible for pushing */
> >         dequeue_pushable_task(rq, p);
> >  }

Thanks for the review!

-- 
#include <best/regards.h>

Patrick Bellasi