Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1367793 > unrolled thread

[PATCH RESEND v2 0/6] sched/fair: Clean up sched metric definitions

Back to article view | Back to linux.kernel

Contents

  [PATCH RESEND v2 0/6] sched/fair: Clean up sched metric definitions Yuyang Du <yuyang.du@intel.com> - 2016-03-31 06:10 +0200
    [PATCH RESEND v2 6/6] sched/fair: Remove unconditionally inactive code Yuyang Du <yuyang.du@intel.com> - 2016-03-31 06:10 +0200
      Re: [PATCH RESEND v2 6/6] sched/fair: Remove unconditionally inactive code bsegall@google.com - 2016-03-31 20:00 +0200
        Re: [PATCH RESEND v2 6/6] sched/fair: Remove unconditionally  inactive code Yuyang Du <yuyang.du@intel.com> - 2016-04-01 09:10 +0200
    [PATCH RESEND v2 2/6] sched/fair: Remove SCHED_LOAD_SHIFT and SCHED_LOAD_SCALE Yuyang Du <yuyang.du@intel.com> - 2016-03-31 06:10 +0200

#1367793 — [PATCH RESEND v2 0/6] sched/fair: Clean up sched metric definitions

Hi Peter,

This patch searies is left in last year, and thus I resend it. Would you
please give it a look?

The previous version is at http://thread.gmane.org/gmane.linux.kernel/2068513

This series cleans up the sched metrics, changes include:
(1) Define SCHED_FIXEDPOINT_SHIFT for all fixed point arithmetic scaling.
(2) Get rid of confusing scaling factors: SCHED_LOAD_SHIFT and SCHED_LOAD_SCALE,
    and thus only levae NICE_0_LOAD (for load) and SCHED_CAPACITY_SCALE (for util).
(3) Consistently use SCHED_CAPACITY_SCALE for util related.
(4) Add more detailed introduction to the sched metrics.
(5) Get rid of unnecessary extra scaling up and down for load.
(6) Rename the mappings between priority (user) and load (kernel).
(7) Remove/replace inactive code.

So, except for (5), we did not change any logic. Per request by Ingo, I checked
the disassembly of kernel/sched/built-in.o before vs. after the patches. But
since the very first patch to the end, there are a bunch of "offset" changes,
all like the pattern:

     60e3:      eb 21                   jmp    6106 <rq_clock+0x7c>
-    60e5:      be db 02 00 00          mov    $0x2db,%esi
+    60e5:      be e0 02 00 00          mov    $0x2e0,%esi

I have no idea what is changed, but venture a guess, code layout changed a bit?

Anyway, thanks a lot to Ben, Morten, Dietmar, Vincent, and others who provided
valuable comments.

v2 changes:
- Rename SCHED_RESOLUTION_SHIFT to SCHED_FIXEDPOINT_SHIFT, thanks to Peter
- Fix bugs in calculate_imbalance(), thanks to Vincent
- Fix "#if 0" for increased kernel load, suggested by Ingo

Thanks,
Yuyang

Yuyang Du (6):
  sched/fair: Generalize the load/util averages resolution definition
  sched/fair: Remove SCHED_LOAD_SHIFT and SCHED_LOAD_SCALE
  sched/fair: Add introduction to the sched load avg metrics
  sched/fair: Remove scale_load_down() for load_avg
  sched/fair: Rename scale_load() and scale_load_down()
  sched/fair: Remove unconditionally inactive code

 include/linux/sched.h | 81 +++++++++++++++++++++++++++++++++++++++++++--------
 init/Kconfig          | 16 ++++++++++
 kernel/sched/core.c   |  8 ++---
 kernel/sched/fair.c   | 33 ++++++++++-----------
 kernel/sched/sched.h  | 52 +++++++++++++++------------------
 5 files changed, 127 insertions(+), 63 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1367795 — [PATCH RESEND v2 6/6] sched/fair: Remove unconditionally inactive code

The increased load resolution (fixed point arithmetic range) is
unconditionally deactivated with #if 0, so it is effectively broken.

But the increased load range is still used somewhere (e.g., in Google),
so we keep this feature. The reconciliation is we define
CONFIG_CFS_INCREASE_LOAD_RANGE and it depends on FAIR_GROUP_SCHED and
64BIT and BROKEN.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 init/Kconfig         | 16 +++++++++++++++
 kernel/sched/sched.h | 55 +++++++++++++++++++++-------------------------------
 2 files changed, 38 insertions(+), 33 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 2232080..d072c09 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1025,6 +1025,22 @@ config CFS_BANDWIDTH
 	  restriction.
 	  See tip/Documentation/scheduler/sched-bwc.txt for more information.
 
+config CFS_INCREASE_LOAD_RANGE
+	bool "Increase kernel load range"
+	depends on 64BIT && BROKEN
+	default n
+	help
+	  Increase resolution of nice-level calculations for 64-bit architectures.
+	  The extra resolution improves shares distribution and load balancing of
+	  low-weight task groups (eg. nice +19 on an autogroup), deeper taskgroup
+	  hierarchies, especially on larger systems. This is not a user-visible change
+	  and does not change the user-interface for setting shares/weights.
+	  We increase resolution only if we have enough bits to allow this increased
+	  resolution (i.e. BITS_PER_LONG > 32). The costs for increasing resolution
+	  when BITS_PER_LONG <= 32 are pretty high and the returns do not justify the
+	  increased costs.
+	  Currently broken: it increases power usage under light load.
+
 config RT_GROUP_SCHED
 	bool "Group scheduling for SCHED_RR/FIFO"
 	depends on CGROUP_SCHED
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index ebe16e3..1bb0d69 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -42,39 +42,6 @@ static inline void update_cpu_load_active(struct rq *this_rq) { }
 #define NS_TO_JIFFIES(TIME)	((unsigned long)(TIME) / (NSEC_PER_SEC / HZ))
 
 /*
- * Increase resolution of nice-level calculations for 64-bit architectures.
- * The extra resolution improves shares distribution and load balancing of
- * low-weight task groups (eg. nice +19 on an autogroup), deeper taskgroup
- * hierarchies, especially on larger systems. This is not a user-visible change
- * and does not change the user-interface for setting shares/weights.
- *
- * We increase resolution only if we have enough bits to allow this increased
- * resolution (i.e. BITS_PER_LONG > 32). The costs for increasing resolution
- * when BITS_PER_LONG <= 32 are pretty high and the returns do not justify the
- * increased costs.
- */
-#if 0 /* BITS_PER_LONG > 32 -- currently broken: it increases power usage under light load  */
-# define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT + SCHED_FIXEDPOINT_SHIFT)
-# define user_to_kernel_load(w)	((w) << SCHED_FIXEDPOINT_SHIFT)
-# define kernel_to_user_load(w)	((w) >> SCHED_FIXEDPOINT_SHIFT)
-#else
-# define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT)
-# define user_to_kernel_load(w)	(w)
-# define kernel_to_user_load(w)	(w)
-#endif
-
-/*
- * Task weight (visible to user) and its load (invisible to user) have
- * independent resolution, but they should be well calibrated. We use
- * user_to_kernel_load() and kernel_to_user_load(w) to convert between
- * them. The following must be true:
- *
- * user_to_kernel_load(sched_prio_to_weight[USER_PRIO(NICE_TO_PRIO(0))]) == NICE_0_LOAD
- * kernel_to_user_load(NICE_0_LOAD) == sched_prio_to_weight[USER_PRIO(NICE_TO_PRIO(0))]
- */
-#define NICE_0_LOAD		(1L << NICE_0_LOAD_SHIFT)
-
-/*
  * Single value that decides SCHED_DEADLINE internal math precision.
  * 10 -> just above 1us
  * 9  -> just above 0.5us
@@ -1150,6 +1117,28 @@ extern const int sched_prio_to_weight[40];
 extern const u32 sched_prio_to_wmult[40];
 
 /*
+ * Task weight (visible to user) and its load (invisible to user) have
+ * independent ranges, but they should be well calibrated. We use
+ * user_to_kernel_load() and kernel_to_user_load(w) to convert between
+ * them.
+ *
+ * The following must also be true:
+ * user_to_kernel_load(sched_prio_to_weight[USER_PRIO(NICE_TO_PRIO(0))]) == NICE_0_LOAD
+ * kernel_to_user_load(NICE_0_LOAD) == sched_prio_to_weight[USER_PRIO(NICE_TO_PRIO(0))]
+ */
+#ifdef CONFIG_CFS_INCREASE_LOAD_RANGE
+#define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT + SCHED_FIXEDPOINT_SHIFT)
+#define user_to_kernel_load(w)	(w << SCHED_FIXEDPOINT_SHIFT)
+#define kernel_to_user_load(w)	(w >> SCHED_FIXEDPOINT_SHIFT)
+#else
+#define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT)
+#define user_to_kernel_load(w)	(w)
+#define kernel_to_user_load(w)	(w)
+#endif
+
+#define NICE_0_LOAD		(1UL << NICE_0_LOAD_SHIFT)
+
+/*
  * {de,en}queue flags:
  *
  * DEQUEUE_SLEEP  - task is no longer runnable
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1368509 — Re: [PATCH RESEND v2 6/6] sched/fair: Remove unconditionally inactive code

Yuyang Du <yuyang.du@intel.com> writes:

> The increased load resolution (fixed point arithmetic range) is
> unconditionally deactivated with #if 0, so it is effectively broken.
>
> But the increased load range is still used somewhere (e.g., in Google),
> so we keep this feature. The reconciliation is we define
> CONFIG_CFS_INCREASE_LOAD_RANGE and it depends on FAIR_GROUP_SCHED and
> 64BIT and BROKEN.
>
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Yuyang Du <yuyang.du@intel.com>

The title of this patch "Remove unconditionally inactive code" is
misleading since it's more like giving it a CONFIG.

Also as a side note, does anyone remember/have a test for whatever got
it turned off to begin with, given all the changes in load tracking and
the load balancer and everything else?

> ---
>  init/Kconfig         | 16 +++++++++++++++
>  kernel/sched/sched.h | 55 +++++++++++++++++++++-------------------------------
>  2 files changed, 38 insertions(+), 33 deletions(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 2232080..d072c09 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1025,6 +1025,22 @@ config CFS_BANDWIDTH
>  	  restriction.
>  	  See tip/Documentation/scheduler/sched-bwc.txt for more information.
>  
> +config CFS_INCREASE_LOAD_RANGE
> +	bool "Increase kernel load range"
> +	depends on 64BIT && BROKEN
> +	default n
> +	help
> +	  Increase resolution of nice-level calculations for 64-bit architectures.
> +	  The extra resolution improves shares distribution and load balancing of
> +	  low-weight task groups (eg. nice +19 on an autogroup), deeper taskgroup
> +	  hierarchies, especially on larger systems. This is not a user-visible change
> +	  and does not change the user-interface for setting shares/weights.
> +	  We increase resolution only if we have enough bits to allow this increased
> +	  resolution (i.e. BITS_PER_LONG > 32). The costs for increasing resolution
> +	  when BITS_PER_LONG <= 32 are pretty high and the returns do not justify the
> +	  increased costs.
> +	  Currently broken: it increases power usage under light load.
> +
>  config RT_GROUP_SCHED
>  	bool "Group scheduling for SCHED_RR/FIFO"
>  	depends on CGROUP_SCHED
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index ebe16e3..1bb0d69 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -42,39 +42,6 @@ static inline void update_cpu_load_active(struct rq *this_rq) { }
>  #define NS_TO_JIFFIES(TIME)	((unsigned long)(TIME) / (NSEC_PER_SEC / HZ))
>  
>  /*
> - * Increase resolution of nice-level calculations for 64-bit architectures.
> - * The extra resolution improves shares distribution and load balancing of
> - * low-weight task groups (eg. nice +19 on an autogroup), deeper taskgroup
> - * hierarchies, especially on larger systems. This is not a user-visible change
> - * and does not change the user-interface for setting shares/weights.
> - *
> - * We increase resolution only if we have enough bits to allow this increased
> - * resolution (i.e. BITS_PER_LONG > 32). The costs for increasing resolution
> - * when BITS_PER_LONG <= 32 are pretty high and the returns do not justify the
> - * increased costs.
> - */
> -#if 0 /* BITS_PER_LONG > 32 -- currently broken: it increases power usage under light load  */
> -# define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT + SCHED_FIXEDPOINT_SHIFT)
> -# define user_to_kernel_load(w)	((w) << SCHED_FIXEDPOINT_SHIFT)
> -# define kernel_to_user_load(w)	((w) >> SCHED_FIXEDPOINT_SHIFT)
> -#else
> -# define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT)
> -# define user_to_kernel_load(w)	(w)
> -# define kernel_to_user_load(w)	(w)
> -#endif
> -
> -/*
> - * Task weight (visible to user) and its load (invisible to user) have
> - * independent resolution, but they should be well calibrated. We use
> - * user_to_kernel_load() and kernel_to_user_load(w) to convert between
> - * them. The following must be true:
> - *
> - * user_to_kernel_load(sched_prio_to_weight[USER_PRIO(NICE_TO_PRIO(0))]) == NICE_0_LOAD
> - * kernel_to_user_load(NICE_0_LOAD) == sched_prio_to_weight[USER_PRIO(NICE_TO_PRIO(0))]
> - */
> -#define NICE_0_LOAD		(1L << NICE_0_LOAD_SHIFT)
> -
> -/*
>   * Single value that decides SCHED_DEADLINE internal math precision.
>   * 10 -> just above 1us
>   * 9  -> just above 0.5us
> @@ -1150,6 +1117,28 @@ extern const int sched_prio_to_weight[40];
>  extern const u32 sched_prio_to_wmult[40];
>  
>  /*
> + * Task weight (visible to user) and its load (invisible to user) have
> + * independent ranges, but they should be well calibrated. We use
> + * user_to_kernel_load() and kernel_to_user_load(w) to convert between
> + * them.
> + *
> + * The following must also be true:
> + * user_to_kernel_load(sched_prio_to_weight[USER_PRIO(NICE_TO_PRIO(0))]) == NICE_0_LOAD
> + * kernel_to_user_load(NICE_0_LOAD) == sched_prio_to_weight[USER_PRIO(NICE_TO_PRIO(0))]
> + */
> +#ifdef CONFIG_CFS_INCREASE_LOAD_RANGE
> +#define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT + SCHED_FIXEDPOINT_SHIFT)
> +#define user_to_kernel_load(w)	(w << SCHED_FIXEDPOINT_SHIFT)
> +#define kernel_to_user_load(w)	(w >> SCHED_FIXEDPOINT_SHIFT)
> +#else
> +#define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT)
> +#define user_to_kernel_load(w)	(w)
> +#define kernel_to_user_load(w)	(w)
> +#endif
> +
> +#define NICE_0_LOAD		(1UL << NICE_0_LOAD_SHIFT)
> +
> +/*
>   * {de,en}queue flags:
>   *
>   * DEQUEUE_SLEEP  - task is no longer runnable

[toc] | [prev] | [next] | [standalone]

#1369040 — Re: [PATCH RESEND v2 6/6] sched/fair: Remove unconditionally inactive code

On Thu, Mar 31, 2016 at 10:53:02AM -0700, bsegall@google.com wrote:
> Yuyang Du <yuyang.du@intel.com> writes:
> 
> > The increased load resolution (fixed point arithmetic range) is
> > unconditionally deactivated with #if 0, so it is effectively broken.
> >
> > But the increased load range is still used somewhere (e.g., in Google),
> > so we keep this feature. The reconciliation is we define
> > CONFIG_CFS_INCREASE_LOAD_RANGE and it depends on FAIR_GROUP_SCHED and
> > 64BIT and BROKEN.
> >
> > Suggested-by: Ingo Molnar <mingo@kernel.org>
> > Signed-off-by: Yuyang Du <yuyang.du@intel.com>
> 
> The title of this patch "Remove unconditionally inactive code" is
> misleading since it's more like giving it a CONFIG.

Reasonable argument.

> 
> Also as a side note, does anyone remember/have a test for whatever got
> it turned off to begin with, given all the changes in load tracking and
> the load balancer and everything else?

It is this commit that turned it off:

Commit e4c2fb0d5776: "sched: Disable (revert) SCHED_LOAD_SCALE increase"

[toc] | [prev] | [next] | [standalone]

#1367796 — [PATCH RESEND v2 2/6] sched/fair: Remove SCHED_LOAD_SHIFT and SCHED_LOAD_SCALE

After cleaning up the sched metrics, these two definitions that cause
ambiguity are not needed any more. Use NICE_0_LOAD_SHIFT and NICE_0_LOAD
instead (the names suggest clearly who they are).

Suggested-by: Ben Segall <bsegall@google.com>
Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 kernel/sched/fair.c  |  4 ++--
 kernel/sched/sched.h | 22 +++++++++++-----------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 1d3fc01..bf835b5 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -682,7 +682,7 @@ void init_entity_runnable_average(struct sched_entity *se)
 	sa->period_contrib = 1023;
 	sa->load_avg = scale_load_down(se->load.weight);
 	sa->load_sum = sa->load_avg * LOAD_AVG_MAX;
-	sa->util_avg = scale_load_down(SCHED_LOAD_SCALE);
+	sa->util_avg = SCHED_CAPACITY_SCALE;
 	sa->util_sum = sa->util_avg * LOAD_AVG_MAX;
 	/* when this task enqueue'ed, it will contribute to its cfs_rq's load_avg */
 }
@@ -6877,7 +6877,7 @@ static inline void calculate_imbalance(struct lb_env *env, struct sd_lb_stats *s
 	if (busiest->group_type == group_overloaded &&
 	    local->group_type   == group_overloaded) {
 		load_above_capacity = busiest->sum_nr_running *
-					SCHED_LOAD_SCALE;
+				      scale_load_down(NICE_0_LOAD);
 		if (load_above_capacity > busiest->group_capacity)
 			load_above_capacity -= busiest->group_capacity;
 		else
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 15a89ee..94ba652 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -54,25 +54,25 @@ static inline void update_cpu_load_active(struct rq *this_rq) { }
  * increased costs.
  */
 #if 0 /* BITS_PER_LONG > 32 -- currently broken: it increases power usage under light load  */
-# define SCHED_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT + SCHED_FIXEDPOINT_SHIFT)
+# define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT + SCHED_FIXEDPOINT_SHIFT)
 # define scale_load(w)		((w) << SCHED_FIXEDPOINT_SHIFT)
 # define scale_load_down(w)	((w) >> SCHED_FIXEDPOINT_SHIFT)
 #else
-# define SCHED_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT)
+# define NICE_0_LOAD_SHIFT	(SCHED_FIXEDPOINT_SHIFT)
 # define scale_load(w)		(w)
 # define scale_load_down(w)	(w)
 #endif
 
-#define SCHED_LOAD_SCALE	(1L << SCHED_LOAD_SHIFT)
-
 /*
- * NICE_0's weight (visible to user) and its load (invisible to user) have
- * independent ranges, but they should be well calibrated. We use scale_load()
- * and scale_load_down(w) to convert between them, the following must be true:
- * scale_load(sched_prio_to_weight[20]) == NICE_0_LOAD
+ * Task weight (visible to user) and its load (invisible to user) have
+ * independent resolution, but they should be well calibrated. We use
+ * scale_load() and scale_load_down(w) to convert between them. The
+ * following must be true:
+ *
+ *  scale_load(sched_prio_to_weight[USER_PRIO(NICE_TO_PRIO(0))]) == NICE_0_LOAD
+ *
  */
-#define NICE_0_LOAD		SCHED_LOAD_SCALE
-#define NICE_0_SHIFT		SCHED_LOAD_SHIFT
+#define NICE_0_LOAD		(1L << NICE_0_LOAD_SHIFT)
 
 /*
  * Single value that decides SCHED_DEADLINE internal math precision.
@@ -859,7 +859,7 @@ DECLARE_PER_CPU(struct sched_domain *, sd_asym);
 struct sched_group_capacity {
 	atomic_t ref;
 	/*
-	 * CPU capacity of this group, SCHED_LOAD_SCALE being max capacity
+	 * CPU capacity of this group, SCHED_CAPACITY_SCALE being max capacity
 	 * for a single CPU.
 	 */
 	unsigned int capacity;
-- 
2.1.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web