linux-kernel - Re: [tip:sched/core] sched/fair: Remove #ifdefs from 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]

Message-ID: <20180915122956.GA12378@gmail.com>
Date:   Sat, 15 Sep 2018 14:29:56 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     Ingo Molnar <mingo@...hat.com>,
        viresh kumar <viresh.kumar@...aro.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        linux-tip-commits@...r.kernel.org, douly.fnst@...fujitsu.com,
        Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Subject: Re: [tip:sched/core] sched/fair: Remove #ifdefs from
 scale_rt_capacity()


* Vincent Guittot <vincent.guittot@...aro.org> wrote:

> Hi Ingo,
> 
> On Sat, 15 Sep 2018 at 13:47, Ingo Molnar <mingo@...nel.org> wrote:
> >
> >
> > * tip-bot for Vincent Guittot <tipbot@...or.com> wrote:
> >
> > > Commit-ID:  2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a
> > > Gitweb:     https://git.kernel.org/tip/2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a
> > > Author:     Vincent Guittot <vincent.guittot@...aro.org>
> > > AuthorDate: Thu, 19 Jul 2018 14:00:06 +0200
> > > Committer:  Ingo Molnar <mingo@...nel.org>
> > > 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@...hat.com>
> > > Signed-off-by: Vincent Guittot <vincent.guittot@...aro.org>
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> > > Acked-by: Viresh Kumar <viresh.kumar@...aro.org>
> > > Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> > > Cc: Peter Zijlstra <peterz@...radead.org>
> > > Cc: Thomas Gleixner <tglx@...utronix.de>
> > > Cc: rjw@...ysocki.net
> > > Link: http://lkml.kernel.org/r/1532001606-6689-1-git-send-email-vincent.guittot@linaro.org
> > > Signed-off-by: Ingo Molnar <mingo@...nel.org>
> > > ---
> > >  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!

> 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.

Thanks,

	Ingo