Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1530200714-4504-1-git-send-email-vincent.guittot@linaro.org>
 <1530200714-4504-10-git-send-email-vincent.guittot@linaro.org> <20180715221538.GA23280@gmail.com>
In-Reply-To: <20180715221538.GA23280@gmail.com>
From:   Vincent Guittot <vincent.guittot@linaro.org>
Date:   Mon, 16 Jul 2018 13:24:48 +0200
Message-ID: <CAKfTPtCNtsu3v95+2vtVAn_mxAO_gQgb0X1L2G97T2dwdvee1g@mail.gmail.com>
Subject: Re: [PATCH 09/11] sched: use pelt for scale_rt_capacity()
To:     Ingo Molnar <mingo@kernel.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Juri Lelli <juri.lelli@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Morten Rasmussen <Morten.Rasmussen@arm.com>,
        viresh kumar <viresh.kumar@linaro.org>,
        Valentin Schneider <valentin.schneider@arm.com>,
        Patrick Bellasi <patrick.bellasi@arm.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Quentin Perret <quentin.perret@arm.com>,
        Luca Abeni <luca.abeni@santannapisa.it>,
        Claudio Scordino <claudio@evidence.eu.com>,
        Ingo Molnar <mingo@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Ingo,

On Mon, 16 Jul 2018 at 00:15, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Vincent Guittot <vincent.guittot@linaro.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 =3D 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 =3D READ_ONCE(rq->age_stamp);
> > -     avg =3D READ_ONCE(rq->rt_avg);
> > -     delta =3D __rq_clock_broken(rq) - age_stamp;
> > +     unsigned long max =3D arch_scale_cpu_capacity(NULL, cpu);
> > +     unsigned long used, irq, free;
> >
> > -     if (unlikely(delta < 0))
> > -             delta =3D 0;
> > +#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIM=
E_ACCOUNTING)
> > +     irq =3D READ_ONCE(rq->avg_irq.util_avg);
> >
> > -     total =3D sched_avg_period() + delta;
> > +     if (unlikely(irq >=3D max))
> > +             return 1;
> > +#endif
>
> Note that 'irq' is unused outside that macro block, resulting in a new wa=
rning on
> defconfig builds:
>
>  CC      kernel/sched/fair.o
>  kernel/sched/fair.c: In function =E2=80=98scale_rt_capacity=E2=80=99:
>  kernel/sched/fair.c:7553:22: warning: unused variable =E2=80=98irq=E2=80=
=99 [-Wunused-variable]
>    unsigned long used, irq, free;
>                        ^~~
>
> I have applied the delta fix below for simplicity, but what we really wan=
t 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 th=
at if the
> configs are off the compiler does the right thing and eliminates this ide=
ntity
> transformation for the irq=3D=3D0 case:
>
>         free *=3D (max - irq);
>         free /=3D max;
>
> If the compiler refuses to optimize this away (due to the zero and overfl=
ow
> 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 =3D cpu_rq(cpu);
>         unsigned long max =3D 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 =3D READ_ONCE(rq->avg_irq.util_avg);