linux-kernel - Re: [PATCH 09/11] sched: use pelt for scale_rt

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:   Mon, 16 Jul 2018 13:24:48 +0200
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Juri Lelli <juri.lelli@...hat.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Morten Rasmussen <Morten.Rasmussen@....com>,
        viresh kumar <viresh.kumar@...aro.org>,
        Valentin Schneider <valentin.schneider@....com>,
        Patrick Bellasi <patrick.bellasi@....com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Quentin Perret <quentin.perret@....com>,
        Luca Abeni <luca.abeni@...tannapisa.it>,
        Claudio Scordino <claudio@...dence.eu.com>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH 09/11] sched: use pelt for scale_rt_capacity()

Hi Ingo,

On Mon, 16 Jul 2018 at 00:15, Ingo Molnar <mingo@...nel.org> wrote:
>
>
> * Vincent Guittot <vincent.guittot@...aro.org> wrote:
>

> > @@ -7550,39 +7550,36 @@ static inline int get_sd_load_idx(struct sched_domain *sd,
> >  static unsigned long scale_rt_capacity(int cpu)
> >  {
> >       struct rq *rq = cpu_rq(cpu);
> > -     u64 total, used, age_stamp, avg;
> > -     s64 delta;
> > -
> > -     /*
> > -      * Since we're reading these variables without serialization make sure
> > -      * we read them once before doing sanity checks on them.
> > -      */
> > -     age_stamp = READ_ONCE(rq->age_stamp);
> > -     avg = READ_ONCE(rq->rt_avg);
> > -     delta = __rq_clock_broken(rq) - age_stamp;
> > +     unsigned long max = arch_scale_cpu_capacity(NULL, cpu);
> > +     unsigned long used, irq, free;
> >
> > -     if (unlikely(delta < 0))
> > -             delta = 0;
> > +#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
> > +     irq = READ_ONCE(rq->avg_irq.util_avg);
> >
> > -     total = sched_avg_period() + delta;
> > +     if (unlikely(irq >= max))
> > +             return 1;
> > +#endif
>
> Note that 'irq' is unused outside that macro block, resulting in a new warning on
> defconfig builds:
>
>  CC      kernel/sched/fair.o
>  kernel/sched/fair.c: In function ‘scale_rt_capacity’:
>  kernel/sched/fair.c:7553:22: warning: unused variable ‘irq’ [-Wunused-variable]
>    unsigned long used, irq, free;
>                        ^~~
>
> I have applied the delta fix below for simplicity, but what we really want is a
> cleanup of that function to eliminate the #ifdefs. One solution would be to factor
> out the 'irq' utilization value into a helper inline, and double check that if the
> configs are off the compiler does the right thing and eliminates this identity
> transformation for the irq==0 case:
>
>         free *= (max - irq);
>         free /= max;
>
> If the compiler refuses to optimize this away (due to the zero and overflow
> cases), try to find something more clever?

Thanks for the fix.
I'm off for now and will look at your proposal above once back

Regards,
Vincent

>
> Thanks,
>
>         Ingo
>
>  kernel/sched/fair.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index e3221db0511a..d5f7d521e448 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7550,7 +7550,10 @@ static unsigned long scale_rt_capacity(int cpu)
>  {
>         struct rq *rq = cpu_rq(cpu);
>         unsigned long max = arch_scale_cpu_capacity(NULL, cpu);
> -       unsigned long used, irq, free;
> +       unsigned long used, free;
> +#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
> +       unsigned long irq;
> +#endif
>
>  #if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
>         irq = READ_ONCE(rq->avg_irq.util_avg);