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

From: Vincent Guittot
Date: Mon Jul 16 2018 - 07:25:04 EST


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);