linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] sched/fair: remove #ifdefs from scale_rt_capacity
@ 2018-07-19 12:00 Vincent Guittot
  2018-07-20  4:55 ` Viresh Kumar
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Vincent Guittot @ 2018-07-19 12:00 UTC (permalink / raw)
  To: peterz, mingo, linux-kernel; +Cc: rjw, viresh.kumar, Vincent Guittot

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched/fair: remove #ifdefs from scale_rt_capacity
  2018-07-19 12:00 [PATCH] sched/fair: remove #ifdefs from scale_rt_capacity Vincent Guittot
@ 2018-07-20  4:55 ` Viresh Kumar
  2018-07-20 12:31 ` Peter Zijlstra
  2018-07-25 14:22 ` [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity() tip-bot for Vincent Guittot
  2 siblings, 0 replies; 10+ messages in thread
From: Viresh Kumar @ 2018-07-20  4:55 UTC (permalink / raw)
  To: Vincent Guittot; +Cc: peterz, mingo, linux-kernel, rjw

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched/fair: remove #ifdefs from scale_rt_capacity
  2018-07-19 12:00 [PATCH] sched/fair: remove #ifdefs from scale_rt_capacity Vincent Guittot
  2018-07-20  4:55 ` Viresh Kumar
@ 2018-07-20 12:31 ` Peter Zijlstra
  2018-07-30 16:04   ` Vincent Guittot
  2018-07-25 14:22 ` [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity() tip-bot for Vincent Guittot
  2 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2018-07-20 12:31 UTC (permalink / raw)
  To: Vincent Guittot; +Cc: mingo, linux-kernel, rjw, viresh.kumar

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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity()
  2018-07-19 12:00 [PATCH] sched/fair: remove #ifdefs from scale_rt_capacity Vincent Guittot
  2018-07-20  4:55 ` Viresh Kumar
  2018-07-20 12:31 ` Peter Zijlstra
@ 2018-07-25 14:22 ` tip-bot for Vincent Guittot
  2018-09-15 11:46   ` Ingo Molnar
  2 siblings, 1 reply; 10+ messages in thread
From: tip-bot for Vincent Guittot @ 2018-07-25 14:22 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, viresh.kumar, mingo, mingo, hpa, vincent.guittot,
	peterz, tglx, torvalds

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

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c3cf7d992159..fc177c06e490 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -177,7 +177,7 @@ static void update_rq_clock_task(struct rq *rq, s64 delta)
 
 	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
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 97dcd4472a0e..3fffad3bc8a8 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 d5f7d521e448..14c3fddf822a 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 ebb4b3c3ece7..614170d9b1aa 100644
--- 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;
@@ -2210,17 +2211,32 @@ 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)
+#ifdef HAVE_SCHED_AVG_IRQ
 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched/fair: remove #ifdefs from scale_rt_capacity
  2018-07-20 12:31 ` Peter Zijlstra
@ 2018-07-30 16:04   ` Vincent Guittot
  0 siblings, 0 replies; 10+ messages in thread
From: Vincent Guittot @ 2018-07-30 16:04 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Ingo Molnar, linux-kernel, Rafael J. Wysocki, viresh kumar

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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity()
  2018-07-25 14:22 ` [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity() tip-bot for Vincent Guittot
@ 2018-09-15 11:46   ` Ingo Molnar
  2018-09-15 12:17     ` Vincent Guittot
  0 siblings, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2018-09-15 11:46 UTC (permalink / raw)
  To: mingo, viresh.kumar, linux-kernel, peterz, torvalds, tglx,
	vincent.guittot, hpa
  Cc: linux-tip-commits


* 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity()
  2018-09-15 11:46   ` Ingo Molnar
@ 2018-09-15 12:17     ` Vincent Guittot
  2018-09-15 12:29       ` Ingo Molnar
  0 siblings, 1 reply; 10+ messages in thread
From: Vincent Guittot @ 2018-09-15 12:17 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Ingo Molnar, viresh kumar, linux-kernel, Peter Zijlstra,
	Linus Torvalds, Thomas Gleixner, H. Peter Anvin,
	linux-tip-commits, douly.fnst, Miguel Ojeda

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity()
  2018-09-15 12:17     ` Vincent Guittot
@ 2018-09-15 12:29       ` Ingo Molnar
  2018-09-15 14:35         ` Vincent Guittot
  0 siblings, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2018-09-15 12:29 UTC (permalink / raw)
  To: Vincent Guittot
  Cc: Ingo Molnar, viresh kumar, linux-kernel, Peter Zijlstra,
	Linus Torvalds, Thomas Gleixner, H. Peter Anvin,
	linux-tip-commits, douly.fnst, Miguel Ojeda


* 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity()
  2018-09-15 12:29       ` Ingo Molnar
@ 2018-09-15 14:35         ` Vincent Guittot
  2018-09-18  6:38           ` Ingo Molnar
  0 siblings, 1 reply; 10+ messages in thread
From: Vincent Guittot @ 2018-09-15 14:35 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Ingo Molnar, viresh kumar, linux-kernel, Peter Zijlstra,
	Linus Torvalds, Thomas Gleixner, H. Peter Anvin,
	linux-tip-commits, douly.fnst, Miguel Ojeda

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity()
  2018-09-15 14:35         ` Vincent Guittot
@ 2018-09-18  6:38           ` Ingo Molnar
  0 siblings, 0 replies; 10+ messages in thread
From: Ingo Molnar @ 2018-09-18  6:38 UTC (permalink / raw)
  To: Vincent Guittot
  Cc: Ingo Molnar, viresh kumar, linux-kernel, Peter Zijlstra,
	Linus Torvalds, Thomas Gleixner, H. Peter Anvin,
	linux-tip-commits, douly.fnst, Miguel Ojeda


* 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-09-18  6:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-19 12:00 [PATCH] sched/fair: remove #ifdefs from scale_rt_capacity Vincent Guittot
2018-07-20  4:55 ` Viresh Kumar
2018-07-20 12:31 ` Peter Zijlstra
2018-07-30 16:04   ` Vincent Guittot
2018-07-25 14:22 ` [tip:sched/core] sched/fair: Remove #ifdefs from scale_rt_capacity() tip-bot for Vincent Guittot
2018-09-15 11:46   ` Ingo Molnar
2018-09-15 12:17     ` Vincent Guittot
2018-09-15 12:29       ` Ingo Molnar
2018-09-15 14:35         ` Vincent Guittot
2018-09-18  6:38           ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).