Re: [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity()

[Vincent Guittot]

On Sat, 15 Sep 2018 at 14:30, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>
> * Vincent Guittot <vincent.guittot@xxxxxxxxxx> wrote:
>
> > Hi Ingo,
> >
> > On Sat, 15 Sep 2018 at 13:47, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
> > >
> > >
> > > * tip-bot for Vincent Guittot <tipbot@xxxxxxxxx> wrote:
> > >
> > > > Commit-ID:  2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a
> > > > Gitweb:     https://git.kernel.org/tip/2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a
> > > > Author:     Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> > > > AuthorDate: Thu, 19 Jul 2018 14:00:06 +0200
> > > > Committer:  Ingo Molnar <mingo@xxxxxxxxxx>
> > > > CommitDate: Wed, 25 Jul 2018 11:41:05 +0200
> > > >
> > > > sched/fair: Remove #ifdefs from scale_rt_capacity()
> > > >
> > > > Reuse cpu_util_irq() that has been defined for schedutil and set irq util
> > > > to 0 when !CONFIG_IRQ_TIME_ACCOUNTING.
> > > >
> > > > But the compiler is not able to optimize the sequence (at least with
> > > > aarch64 GCC 7.2.1):
> > > >
> > > >       free *= (max - irq);
> > > >       free /= max;
> > > >
> > > > when irq is fixed to 0
> > > >
> > > > Add a new inline function scale_irq_capacity() that will scale utilization
> > > > when irq is accounted. Reuse this funciton in schedutil which applies
> > > > similar formula.
> > > >
> > > > Suggested-by: Ingo Molnar <mingo@xxxxxxxxxx>
> > > > Signed-off-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > > > Acked-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > > > Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > > Cc: rjw@xxxxxxxxxxxxx
> > > > Link: http://lkml.kernel.org/r/1532001606-6689-1-git-send-email-vincent.guittot@xxxxxxxxxx
> > > > Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
> > > > ---
> > > >  kernel/sched/core.c              |  2 +-
> > > >  kernel/sched/cpufreq_schedutil.c |  3 +--
> > > >  kernel/sched/fair.c              | 13 +++----------
> > > >  kernel/sched/sched.h             | 20 ++++++++++++++++++--
> > > >  4 files changed, 23 insertions(+), 15 deletions(-)
> > >
> > > This commit introduced a build warning in the SMP=n case, could we please fix that? (Probably
> > > the best to maintain variant would be to mark it as __maybe_unused.)
> >
> > Dou sent a fix for this warning
> > https://lkml.org/lkml/2018/8/10/22
> > This one remove one HAVE_SCHED_AVG_IRQ which is at the opposite of
> > what yyou propose below
>
> Yeah, that's a bad patch because it moves one step back, beyond also having an obvious typo in
> the title and overall atrocious spelling that shows that not much thought must have gone into
> the patch - yet you acked it, which makes me unhappy as a maintainer: please don't ack
> obviously triple-flawed patches!

sorry about that

>
> > and Miguel also made a proposal:
> > https://lkml.org/lkml/2018/9/8/215
> > based on __maybe_unused
>
> That solution is probably better, but:
>
> > >
> > > Also, while at it, there's a number of other places that use this pattern:
> > >
> > > > -#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
> > > > +#ifdef HAVE_SCHED_AVG_IRQ
> > >
> > > Could we convert those to HAVE_SCHED_AVG_IRQ as well?
> >
> > I'm not sure that we can convert all to HAVE_SCHED_AVG_IRQ
> >
> > >
> > > dagon:~/tip> git grep 'defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)' kernel/sched/
> > > kernel/sched/core.c:#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
> >
> > we can't replace one because the variables can be used when
> > HAVE_SCHED_AVG_IRQ is undefined
>
> Well, firstly, why is HAVE_SCHED_AVG_IRQ defined in a header and not in the Kconfig space as I
> originally suggested?
>
> Secondly, HAVE_SCHED_AVG_IRQ is defined poorly AFAICS, it should be 0/1 so it can be used as a
> replacement pattern in #if sequences - not in #ifdef sequences as your patch did.
>
> I.e. this needs to be sorted out and done properly, the concerns you voice are simply a side
> effect of this having been done badly.


Ok. I'm going to prepare a patchset to fix all these points.

Regards,
Vincent

>
> Thanks,
>
>         Ingo