Re: [PATCH 09/11] sched: use pelt for scale_rt_capacity()

[Vincent Guittot]

Hi Ingo,

On Mon, 16 Jul 2018 at 00:15, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>
> * Vincent Guittot <vincent.guittot@xxxxxxxxxx> 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);