diff mbox series

sched/fair: remove #ifdefs from scale_rt_capacity

Message ID 1532001606-6689-1-git-send-email-vincent.guittot@linaro.org
State New
Headers show
Series sched/fair: remove #ifdefs from scale_rt_capacity | expand

Commit Message

Vincent Guittot July 19, 2018, noon UTC 
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@redhat.com>
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

---
 kernel/sched/cpufreq_schedutil.c |  3 +--
 kernel/sched/fair.c              | 13 +++----------
 kernel/sched/sched.h             | 20 ++++++++++++++++++--
 3 files changed, 22 insertions(+), 14 deletions(-)

-- 
2.7.4

Comments

Viresh Kumar July 20, 2018, 4:55 a.m. UTC | #1 
On 19-07-18, 14:00, Vincent Guittot wrote:
> 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@redhat.com>

> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

> ---

>  kernel/sched/cpufreq_schedutil.c |  3 +--

>  kernel/sched/fair.c              | 13 +++----------

>  kernel/sched/sched.h             | 20 ++++++++++++++++++--

>  3 files changed, 22 insertions(+), 14 deletions(-)

> 

> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c

> index 97dcd44..3fffad3 100644

> --- a/kernel/sched/cpufreq_schedutil.c

> +++ b/kernel/sched/cpufreq_schedutil.c

> @@ -247,8 +247,7 @@ static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)

>  	 *   U' = irq + ------- * U

>  	 *                max

>  	 */

> -	util *= (max - irq);

> -	util /= max;

> +	util = scale_irq_capacity(util, irq, max);

>  	util += irq;

>  

>  	/*

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c

> index d5f7d52..14c3fdd 100644

> --- a/kernel/sched/fair.c

> +++ b/kernel/sched/fair.c

> @@ -7551,16 +7551,12 @@ 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, 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);

> +	irq = cpu_util_irq(rq);

>  

>  	if (unlikely(irq >= max))

>  		return 1;

> -#endif

>  

>  	used = READ_ONCE(rq->avg_rt.util_avg);

>  	used += READ_ONCE(rq->avg_dl.util_avg);

> @@ -7569,11 +7565,8 @@ static unsigned long scale_rt_capacity(int cpu)

>  		return 1;

>  

>  	free = max - used;

> -#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)

> -	free *= (max - irq);

> -	free /= max;

> -#endif

> -	return free;

> +

> +	return scale_irq_capacity(free, irq, max);

>  }

>  

>  static void update_cpu_capacity(struct sched_domain *sd, int cpu)

> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h

> index ebb4b3c..b80c3fd 100644

> --- a/kernel/sched/sched.h

> +++ b/kernel/sched/sched.h

> @@ -2210,17 +2210,33 @@ static inline unsigned long cpu_util_rt(struct rq *rq)

>  {

>  	return READ_ONCE(rq->avg_rt.util_avg);

>  }

> +#endif

>  

> -#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)

> +#if defined(SMP) \

> + && (defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING))

>  static inline unsigned long cpu_util_irq(struct rq *rq)

>  {

>  	return rq->avg_irq.util_avg;

>  }

> +

> +static inline

> +unsigned long scale_irq_capacity(unsigned long util, unsigned long irq, unsigned long max)

> +{

> +	util *= (max - irq);


() can be dropped here. 

Other than that: 

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>


-- 
viresh
Peter Zijlstra July 20, 2018, 12:31 p.m. UTC | #2 
On Thu, Jul 19, 2018 at 02:00:06PM +0200, Vincent Guittot wrote:
> 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


So much for compilers.... I though those things were supposed to be
'clever'.

> +#if defined(SMP) \

> + && (defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING))


That's atrocious :-)

Fixed it with the below.


--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -135,7 +135,7 @@ static void update_rq_clock_task(struct
  * In theory, the compile should just see 0 here, and optimize out the call
  * to sched_rt_avg_update. But I don't trust it...
  */
-#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
+#ifdef HAVE_SCHED_AVG_IRQ
 	s64 steal = 0, irq_delta = 0;
 #endif
 #ifdef CONFIG_IRQ_TIME_ACCOUNTING
@@ -177,7 +177,7 @@ static void update_rq_clock_task(struct
 
 	rq->clock_task += delta;
 
-#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
+#ifdef HAVE_SCHED_AVG_IRQ
 	if ((irq_delta + steal) && sched_feat(NONTASK_CAPACITY))
 		update_irq_load_avg(rq, irq_delta + steal);
 #endif
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -856,6 +856,7 @@ struct rq {
 	struct sched_avg	avg_rt;
 	struct sched_avg	avg_dl;
 #if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
+#define HAVE_SCHED_AVG_IRQ
 	struct sched_avg	avg_irq;
 #endif
 	u64			idle_stamp;
@@ -2212,8 +2213,7 @@ static inline unsigned long cpu_util_rt(
 }
 #endif
 
-#if defined(SMP) \
- && (defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING))
+#ifdef HAVE_SCHED_AVG_IRQ
 static inline unsigned long cpu_util_irq(struct rq *rq)
 {
 	return rq->avg_irq.util_avg;
Vincent Guittot July 30, 2018, 4:04 p.m. UTC | #3 
Hi Peter,

On Fri, 20 Jul 2018 at 14:31, Peter Zijlstra <peterz@infradead.org> wrote:
>

> On Thu, Jul 19, 2018 at 02:00:06PM +0200, Vincent Guittot wrote:

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

>

> So much for compilers.... I though those things were supposed to be

> 'clever'.

>

> > +#if defined(SMP) \

> > + && (defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING))

>

> That's atrocious :-)

>

> Fixed it with the below.


Thanks for the fix


>

>

> --- a/kernel/sched/core.c

> +++ b/kernel/sched/core.c

> @@ -135,7 +135,7 @@ static void update_rq_clock_task(struct

>   * In theory, the compile should just see 0 here, and optimize out the call

>   * to sched_rt_avg_update. But I don't trust it...

>   */

> -#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)

> +#ifdef HAVE_SCHED_AVG_IRQ

>         s64 steal = 0, irq_delta = 0;

>  #endif

>  #ifdef CONFIG_IRQ_TIME_ACCOUNTING

> @@ -177,7 +177,7 @@ static void update_rq_clock_task(struct

>

>         rq->clock_task += delta;

>

> -#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)

> +#ifdef HAVE_SCHED_AVG_IRQ

>         if ((irq_delta + steal) && sched_feat(NONTASK_CAPACITY))

>                 update_irq_load_avg(rq, irq_delta + steal);

>  #endif

> --- a/kernel/sched/sched.h

> +++ b/kernel/sched/sched.h

> @@ -856,6 +856,7 @@ struct rq {

>         struct sched_avg        avg_rt;

>         struct sched_avg        avg_dl;

>  #if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)

> +#define HAVE_SCHED_AVG_IRQ

>         struct sched_avg        avg_irq;

>  #endif

>         u64                     idle_stamp;

> @@ -2212,8 +2213,7 @@ static inline unsigned long cpu_util_rt(

>  }

>  #endif

>

> -#if defined(SMP) \

> - && (defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING))

> +#ifdef HAVE_SCHED_AVG_IRQ

>  static inline unsigned long cpu_util_irq(struct rq *rq)

>  {

>         return rq->avg_irq.util_avg;
Ingo Molnar Sept. 15, 2018, 11:46 a.m. UTC | #4 
* tip-bot for Vincent Guittot <tipbot@zytor.com> wrote:

> Commit-ID:  2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a

> Gitweb:     https://git.kernel.org/tip/2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a

> Author:     Vincent Guittot <vincent.guittot@linaro.org>

> AuthorDate: Thu, 19 Jul 2018 14:00:06 +0200

> Committer:  Ingo Molnar <mingo@kernel.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@redhat.com>

> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

> Cc: Linus Torvalds <torvalds@linux-foundation.org>

> Cc: Peter Zijlstra <peterz@infradead.org>

> Cc: Thomas Gleixner <tglx@linutronix.de>

> Cc: rjw@rjwysocki.net

> Link: http://lkml.kernel.org/r/1532001606-6689-1-git-send-email-vincent.guittot@linaro.org

> Signed-off-by: Ingo Molnar <mingo@kernel.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.)

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?

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)
kernel/sched/fair.c:#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
kernel/sched/pelt.c:#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
kernel/sched/pelt.h:#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
kernel/sched/sched.h:#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)

Thanks,

	Ingo
Vincent Guittot Sept. 15, 2018, 12:17 p.m. UTC | #5 
Hi Ingo,

On Sat, 15 Sep 2018 at 13:47, Ingo Molnar <mingo@kernel.org> wrote:
>

>

> * tip-bot for Vincent Guittot <tipbot@zytor.com> wrote:

>

> > Commit-ID:  2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a

> > Gitweb:     https://git.kernel.org/tip/2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a

> > Author:     Vincent Guittot <vincent.guittot@linaro.org>

> > AuthorDate: Thu, 19 Jul 2018 14:00:06 +0200

> > Committer:  Ingo Molnar <mingo@kernel.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@redhat.com>

> > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

> > Cc: Linus Torvalds <torvalds@linux-foundation.org>

> > Cc: Peter Zijlstra <peterz@infradead.org>

> > Cc: Thomas Gleixner <tglx@linutronix.de>

> > Cc: rjw@rjwysocki.net

> > Link: http://lkml.kernel.org/r/1532001606-6689-1-git-send-email-vincent.guittot@linaro.org

> > Signed-off-by: Ingo Molnar <mingo@kernel.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

and Miguel also made a proposal:
https://lkml.org/lkml/2018/9/8/215
based on __maybe_unused

>

> 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

> kernel/sched/fair.c:#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)


This one can be replaced indeed

> kernel/sched/pelt.c:#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)


This one can be replaced as well

> kernel/sched/pelt.h:#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)


This one too

> kernel/sched/sched.h:#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)

This one is used to declare HAVE_SCHED_AVG_IRQ

>

> Thanks,

>

>         Ingo
Ingo Molnar Sept. 15, 2018, 12:29 p.m. UTC | #6 
* Vincent Guittot <vincent.guittot@linaro.org> wrote:

> Hi Ingo,

> 

> On Sat, 15 Sep 2018 at 13:47, Ingo Molnar <mingo@kernel.org> wrote:

> >

> >

> > * tip-bot for Vincent Guittot <tipbot@zytor.com> wrote:

> >

> > > Commit-ID:  2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a

> > > Gitweb:     https://git.kernel.org/tip/2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a

> > > Author:     Vincent Guittot <vincent.guittot@linaro.org>

> > > AuthorDate: Thu, 19 Jul 2018 14:00:06 +0200

> > > Committer:  Ingo Molnar <mingo@kernel.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@redhat.com>

> > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

> > > Cc: Linus Torvalds <torvalds@linux-foundation.org>

> > > Cc: Peter Zijlstra <peterz@infradead.org>

> > > Cc: Thomas Gleixner <tglx@linutronix.de>

> > > Cc: rjw@rjwysocki.net

> > > Link: http://lkml.kernel.org/r/1532001606-6689-1-git-send-email-vincent.guittot@linaro.org

> > > Signed-off-by: Ingo Molnar <mingo@kernel.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
Vincent Guittot Sept. 15, 2018, 2:35 p.m. UTC | #7 
On Sat, 15 Sep 2018 at 14:30, Ingo Molnar <mingo@kernel.org> wrote:
>

>

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

>

> > Hi Ingo,

> >

> > On Sat, 15 Sep 2018 at 13:47, Ingo Molnar <mingo@kernel.org> wrote:

> > >

> > >

> > > * tip-bot for Vincent Guittot <tipbot@zytor.com> wrote:

> > >

> > > > Commit-ID:  2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a

> > > > Gitweb:     https://git.kernel.org/tip/2e62c4743adc4c7bfcbc1f45118fc7bec58cf30a

> > > > Author:     Vincent Guittot <vincent.guittot@linaro.org>

> > > > AuthorDate: Thu, 19 Jul 2018 14:00:06 +0200

> > > > Committer:  Ingo Molnar <mingo@kernel.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@redhat.com>

> > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>

> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

> > > > Cc: Linus Torvalds <torvalds@linux-foundation.org>

> > > > Cc: Peter Zijlstra <peterz@infradead.org>

> > > > Cc: Thomas Gleixner <tglx@linutronix.de>

> > > > Cc: rjw@rjwysocki.net

> > > > Link: http://lkml.kernel.org/r/1532001606-6689-1-git-send-email-vincent.guittot@linaro.org

> > > > Signed-off-by: Ingo Molnar <mingo@kernel.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!


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
Ingo Molnar Sept. 18, 2018, 6:38 a.m. UTC | #8 
* Vincent Guittot <vincent.guittot@linaro.org> wrote:

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


Thanks!

	Ingo
diff mbox series

Patch

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 97dcd44..3fffad3 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -247,8 +247,7 @@  static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
 	 *   U' = irq + ------- * U
 	 *                max
 	 */
-	util *= (max - irq);
-	util /= max;
+	util = scale_irq_capacity(util, irq, max);
 	util += irq;
 
 	/*
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d5f7d52..14c3fdd 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7551,16 +7551,12 @@  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, 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);
+	irq = cpu_util_irq(rq);
 
 	if (unlikely(irq >= max))
 		return 1;
-#endif
 
 	used = READ_ONCE(rq->avg_rt.util_avg);
 	used += READ_ONCE(rq->avg_dl.util_avg);
@@ -7569,11 +7565,8 @@  static unsigned long scale_rt_capacity(int cpu)
 		return 1;
 
 	free = max - used;
-#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
-	free *= (max - irq);
-	free /= max;
-#endif
-	return free;
+
+	return scale_irq_capacity(free, irq, max);
 }
 
 static void update_cpu_capacity(struct sched_domain *sd, int cpu)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index ebb4b3c..b80c3fd 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2210,17 +2210,33 @@  static inline unsigned long cpu_util_rt(struct rq *rq)
 {
 	return READ_ONCE(rq->avg_rt.util_avg);
 }
+#endif
 
-#if defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING)
+#if defined(SMP) \
+ && (defined(CONFIG_IRQ_TIME_ACCOUNTING) || defined(CONFIG_PARAVIRT_TIME_ACCOUNTING))
 static inline unsigned long cpu_util_irq(struct rq *rq)
 {
 	return rq->avg_irq.util_avg;
 }
+
+static inline
+unsigned long scale_irq_capacity(unsigned long util, unsigned long irq, unsigned long max)
+{
+	util *= (max - irq);
+	util /= max;
+
+	return util;
+
+}
 #else
 static inline unsigned long cpu_util_irq(struct rq *rq)
 {
 	return 0;
 }
 
-#endif
+static inline
+unsigned long scale_irq_capacity(unsigned long util, unsigned long irq, unsigned long max)
+{
+	return util;
+}
 #endif