linux-kernel - Re: [Patch v8 4/7] sched/fair: Enable periodic update of average thermal pressure

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]

Date:   Fri, 17 Jan 2020 14:22:51 +0100
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Thara Gopinath <thara.gopinath@...aro.org>,
        Ingo Molnar <mingo@...hat.com>,
        Ionela Voinescu <ionela.voinescu@....com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Zhang Rui <rui.zhang@...el.com>,
        Quentin Perret <qperret@...gle.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        viresh kumar <viresh.kumar@...aro.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Amit Kachhap <amit.kachhap@...il.com>,
        Javi Merino <javi.merino@...nel.org>,
        Amit Kucheria <amit.kucheria@...durent.com>
Subject: Re: [Patch v8 4/7] sched/fair: Enable periodic update of average
 thermal pressure

On Thu, 16 Jan 2020 at 16:15, Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Tue, Jan 14, 2020 at 02:57:36PM -0500, Thara Gopinath wrote:
> > Introduce support in CFS periodic tick and other bookkeeping apis
> > to trigger the process of computing average thermal pressure for a
> > cpu. Also consider avg_thermal.load_avg in others_have_blocked
> > which allows for decay of pelt signals.
> >
> > Signed-off-by: Thara Gopinath <thara.gopinath@...aro.org>
> > ---
> >  kernel/sched/fair.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 8da0222..311bb0b 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -7470,6 +7470,9 @@ static inline bool others_have_blocked(struct rq *rq)
> >       if (READ_ONCE(rq->avg_dl.util_avg))
> >               return true;
> >
> > +     if (READ_ONCE(rq->avg_thermal.load_avg))
> > +             return true;
> > +
>
> Given that struct sched_avg is 1 cacheline, the above is a pointless
> guaranteed cacheline miss if the arch doesn't
> CONFIG_HAVE_SCHED_THERMAL_PRESSURE.
>
> >  #ifdef CONFIG_HAVE_SCHED_AVG_IRQ
> >       if (READ_ONCE(rq->avg_irq.util_avg))
> >               return true;
> > @@ -7495,6 +7498,7 @@ static bool __update_blocked_others(struct rq *rq, bool *done)
> >  {
> >       const struct sched_class *curr_class;
> >       u64 now = rq_clock_pelt(rq);
> > +     unsigned long thermal_pressure = arch_cpu_thermal_pressure(cpu_of(rq));
> >       bool decayed;
> >
> >       /*
> > @@ -7505,6 +7509,8 @@ static bool __update_blocked_others(struct rq *rq, bool *done)
> >
> >       decayed = update_rt_rq_load_avg(now, rq, curr_class == &rt_sched_class) |
> >                 update_dl_rq_load_avg(now, rq, curr_class == &dl_sched_class) |
> > +               update_thermal_load_avg(rq_clock_task(rq), rq,
> > +                                       thermal_pressure)                     |
> >                 update_irq_load_avg(rq, 0);
> >
> >       if (others_have_blocked(rq))
>
> That there indentation trainwreck is a reason to rename the function.
>
>         decayed = update_rt_rq_load_avg(now, rq, curr_class == &rt_sched_class) |
>                   update_dl_rq_load_avg(now, rq, curr_class == &dl_sched_class) |
>                   update_thermal_load_avg(rq_clock_task(rq), rq, thermal_pressure) |
>                   update_irq_load_avg(rq, 0);
>
> Is much better.
>
> But now that you made me look at that, I noticed it's using a different
> clock -- it is _NOT_ using now/rq_clock_pelt(), which means it'll not be
> in sync with the other averages.
>
> Is there a good reason for that?

We don't need to apply frequency and cpu capacity invariance on the
thermal capping signal which is  what rq_clock_pelt does

>
> > @@ -10275,6 +10281,7 @@ static void task_tick_fair(struct rq *rq, struct task_struct *curr, int queued)
> >  {
> >       struct cfs_rq *cfs_rq;
> >       struct sched_entity *se = &curr->se;
> > +     unsigned long thermal_pressure = arch_cpu_thermal_pressure(cpu_of(rq));
> >
> >       for_each_sched_entity(se) {
> >               cfs_rq = cfs_rq_of(se);
> > @@ -10286,6 +10293,7 @@ static void task_tick_fair(struct rq *rq, struct task_struct *curr, int queued)
> >
> >       update_misfit_status(curr, rq);
> >       update_overutilized_status(task_rq(curr));
> > +     update_thermal_load_avg(rq_clock_task(rq), rq, thermal_pressure);
> >  }
>
> I'm thinking this is the wrong place; should this not be in
> scheduler_tick(), right before calling sched_class::task_tick() ? Surely
> any execution will affect thermals, not only fair class execution.