[PATCH v2 1/6] sched/fair: add SD_CLUSTER in comments

[PATCH v2 1/6] sched/fair: add SD_CLUSTER in comments

[alexs]

From: Alex Shi <alexs@kernel.org>

The SD_CLUSTER omitted in following TOPOLOGY_SD_FLAGS explaination, add
it to fill the absent.

Signed-off-by: Alex Shi <alexs@kernel.org>
To: linux-kernel@vger.kernel.org
To: Daniel Bristot de Oliveira <bristot@redhat.com>
To: Ben Segall <bsegall@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
To: Dietmar Eggemann <dietmar.eggemann@arm.com>
To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Valentin Schneider <vschneid@redhat.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
To: Juri Lelli <juri.lelli@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
To: Ingo Molnar <mingo@redhat.com>
---
 kernel/sched/topology.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 10d1391e7416..c342c52b1f34 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
  * function:
  *
  *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
+ *   SD_CLUSTER		    - describes Cluster topologies
  *   SD_SHARE_PKG_RESOURCES - describes shared caches
  *   SD_NUMA                - describes NUMA topologies
  *
-- 
2.43.0

[PATCH v2 2/6] sched/fair: remove unused parameters

[alexs]

From: Alex Shi <alexs@kernel.org>

sds isn't used in function sched_asym(), so remove it to cleanup code.

Signed-off-by: Alex Shi <alexs@kernel.org>
Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Valentin Schneider <vschneid@redhat.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
To: Juri Lelli <juri.lelli@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
To: Ingo Molnar <mingo@redhat.com>
---
 kernel/sched/fair.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 46ba8329b10a..8d70417f5125 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9750,7 +9750,6 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
 /**
  * sched_asym - Check if the destination CPU can do asym_packing load balance
  * @env:	The load balancing environment
- * @sds:	Load-balancing data with statistics of the local group
  * @sgs:	Load-balancing statistics of the candidate busiest group
  * @group:	The candidate busiest group
  *
@@ -9769,8 +9768,7 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
  * otherwise.
  */
 static inline bool
-sched_asym(struct lb_env *env, struct sd_lb_stats *sds,  struct sg_lb_stats *sgs,
-	   struct sched_group *group)
+sched_asym(struct lb_env *env, struct sg_lb_stats *sgs, struct sched_group *group)
 {
 	/* Ensure that the whole local core is idle, if applicable. */
 	if (!sched_use_asym_prio(env->sd, env->dst_cpu))
@@ -9941,7 +9939,7 @@ static inline void update_sg_lb_stats(struct lb_env *env,
 	/* Check if dst CPU is idle and preferred to this group */
 	if (!local_group && env->sd->flags & SD_ASYM_PACKING &&
 	    env->idle != CPU_NOT_IDLE && sgs->sum_h_nr_running &&
-	    sched_asym(env, sds, sgs, group)) {
+	    sched_asym(env, sgs, group)) {
 		sgs->group_asym_packing = 1;
 	}
 
-- 
2.43.0

[PATCH v2 3/6] sched/fair: cleanup sched_use_asym_prio

[alexs]

From: Alex Shi <alexs@kernel.org>

And simplify the one line code. No function change.

Signed-off-by: Alex Shi <alexs@kernel.org>
To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Valentin Schneider <vschneid@redhat.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
To: Peter Zijlstra <peterz@infradead.org>
To: Ingo Molnar <mingo@redhat.com>
---
 kernel/sched/fair.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 8d70417f5125..ebd659af2d78 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9741,10 +9741,8 @@ group_type group_classify(unsigned int imbalance_pct,
  */
 static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
 {
-	if (!sched_smt_active())
-		return true;
-
-	return sd->flags & SD_SHARE_CPUCAPACITY || is_core_idle(cpu);
+	return (!sched_smt_active()) ||
+		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
 }
 
 /**
-- 
2.43.0

[PATCH v2 4/6] sched/fair: packing func sched_use_asym_prio/sched_asym_prefer

[alexs]

From: Alex Shi <alexs@kernel.org>

Packing the func sched_use_asym_prio/sched_asym_prefer into one, Using
new func to simply code. No function change.

Thanks Ricardo Neri for func rename and comments suggestion!

Signed-off-by: Alex Shi <alexs@kernel.org>
To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Valentin Schneider <vschneid@redhat.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
To: Peter Zijlstra <peterz@infradead.org>
To: Ingo Molnar <mingo@redhat.com>
---
 kernel/sched/fair.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index ebd659af2d78..835dbe77b260 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9745,8 +9745,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
 		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
 }
 
+static inline bool sched_asym(struct sched_domain *sd, int dst_cpu, int src_cpu)
+{
+	/* Check if asym balance applicable, then check priorities.*/
+	return sched_use_asym_prio(sd, dst_cpu) &&
+		sched_asym_prefer(dst_cpu, src_cpu);
+}
+
 /**
- * sched_asym - Check if the destination CPU can do asym_packing load balance
+ * sched_group_asym - Check if the destination CPU can do asym_packing balance
  * @env:	The load balancing environment
  * @sgs:	Load-balancing statistics of the candidate busiest group
  * @group:	The candidate busiest group
@@ -9766,22 +9773,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
  * otherwise.
  */
 static inline bool
-sched_asym(struct lb_env *env, struct sg_lb_stats *sgs, struct sched_group *group)
+sched_group_asym(struct lb_env *env, struct sg_lb_stats *sgs, struct sched_group *group)
 {
-	/* Ensure that the whole local core is idle, if applicable. */
-	if (!sched_use_asym_prio(env->sd, env->dst_cpu))
-		return false;
-
 	/*
-	 * CPU priorities does not make sense for SMT cores with more than one
+	 * CPU priorities do not make sense for SMT cores with more than one
 	 * busy sibling.
 	 */
-	if (group->flags & SD_SHARE_CPUCAPACITY) {
-		if (sgs->group_weight - sgs->idle_cpus != 1)
-			return false;
-	}
-
-	return sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu);
+	return !(group->flags & SD_SHARE_CPUCAPACITY &&
+			sgs->group_weight - sgs->idle_cpus != 1) &&
+		sched_asym(env->sd, env->dst_cpu, group->asym_prefer_cpu);
 }
 
 /* One group has more than one SMT CPU while the other group does not */
@@ -9937,7 +9937,7 @@ static inline void update_sg_lb_stats(struct lb_env *env,
 	/* Check if dst CPU is idle and preferred to this group */
 	if (!local_group && env->sd->flags & SD_ASYM_PACKING &&
 	    env->idle != CPU_NOT_IDLE && sgs->sum_h_nr_running &&
-	    sched_asym(env, sgs, group)) {
+	    sched_group_asym(env, sgs, group)) {
 		sgs->group_asym_packing = 1;
 	}
 
@@ -11036,8 +11036,7 @@ static struct rq *find_busiest_queue(struct lb_env *env,
 		 * SMT cores with more than one busy sibling.
 		 */
 		if ((env->sd->flags & SD_ASYM_PACKING) &&
-		    sched_use_asym_prio(env->sd, i) &&
-		    sched_asym_prefer(i, env->dst_cpu) &&
+		    sched_asym(env->sd, i, env->dst_cpu) &&
 		    nr_running == 1)
 			continue;
 
@@ -11907,8 +11906,7 @@ static void nohz_balancer_kick(struct rq *rq)
 		 * preferred CPU must be idle.
 		 */
 		for_each_cpu_and(i, sched_domain_span(sd), nohz.idle_cpus_mask) {
-			if (sched_use_asym_prio(sd, i) &&
-			    sched_asym_prefer(i, cpu)) {
+			if (sched_asym(sd, i, cpu)) {
 				flags = NOHZ_STATS_KICK | NOHZ_BALANCE_KICK;
 				goto unlock;
 			}
-- 
2.43.0

[PATCH v2 5/6] sched/fair: pack SD_ASYM_PACKING into sched_use_asym_prio

[alexs]

From: Alex Shi <alexs@kernel.org>

Then the flags check passed into sched_asym and sched_group_asym.
It's a code cleanup, no func changes.

Signed-off-by: Alex Shi <alexs@kernel.org>
To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: Ben Segall <bsegall@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
To: Dietmar Eggemann <dietmar.eggemann@arm.com>
To: Valentin Schneider <vschneid@redhat.com>
To: Daniel Bristot de Oliveira <bristot@redhat.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
To: Juri Lelli <juri.lelli@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
To: Ingo Molnar <mingo@redhat.com>
---
 kernel/sched/fair.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 835dbe77b260..6680cb39c787 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9741,6 +9741,9 @@ group_type group_classify(unsigned int imbalance_pct,
  */
 static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
 {
+	if (!(sd->flags & SD_ASYM_PACKING))
+		return false;
+
 	return (!sched_smt_active()) ||
 		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
 }
@@ -9935,11 +9938,9 @@ static inline void update_sg_lb_stats(struct lb_env *env,
 	sgs->group_weight = group->group_weight;
 
 	/* Check if dst CPU is idle and preferred to this group */
-	if (!local_group && env->sd->flags & SD_ASYM_PACKING &&
-	    env->idle != CPU_NOT_IDLE && sgs->sum_h_nr_running &&
-	    sched_group_asym(env, sgs, group)) {
+	if (!local_group && env->idle != CPU_NOT_IDLE && sgs->sum_h_nr_running &&
+			sched_group_asym(env, sgs, group))
 		sgs->group_asym_packing = 1;
-	}
 
 	/* Check for loaded SMT group to be balanced to dst CPU */
 	if (!local_group && smt_balance(env, sgs, group))
@@ -11035,9 +11036,7 @@ static struct rq *find_busiest_queue(struct lb_env *env,
 		 * If balancing between cores, let lower priority CPUs help
 		 * SMT cores with more than one busy sibling.
 		 */
-		if ((env->sd->flags & SD_ASYM_PACKING) &&
-		    sched_asym(env->sd, i, env->dst_cpu) &&
-		    nr_running == 1)
+		if (sched_asym(env->sd, i, env->dst_cpu) && nr_running == 1)
 			continue;
 
 		switch (env->migration_type) {
@@ -11133,7 +11132,7 @@ asym_active_balance(struct lb_env *env)
 	 * the lower priority @env::dst_cpu help it. Do not follow
 	 * CPU priority.
 	 */
-	return env->idle != CPU_NOT_IDLE && (env->sd->flags & SD_ASYM_PACKING) &&
+	return env->idle != CPU_NOT_IDLE &&
 	       sched_use_asym_prio(env->sd, env->dst_cpu) &&
 	       (sched_asym_prefer(env->dst_cpu, env->src_cpu) ||
 		!sched_use_asym_prio(env->sd, env->src_cpu));
-- 
2.43.0

[PATCH v2 6/6 RFT] sched/fair: change sched asym checking condition

[alexs]

From: Alex Shi <alexs@kernel.org>

Asym only used on SMT sd, or core sd with ITMT and core idled.
!sched_smt_active isn't necessary.

Signed-off-by: Alex Shi <alexs@kernel.org>
To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
To: linux-kernel@vger.kernel.org
To: Valentin Schneider <vschneid@redhat.com>
To: Daniel Bristot de Oliveira <bristot@redhat.com>
To: Ben Segall <bsegall@google.com>
To: Steven Rostedt <rostedt@goodmis.org>
To: Dietmar Eggemann <dietmar.eggemann@arm.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
To: Juri Lelli <juri.lelli@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
To: Ingo Molnar <mingo@redhat.com>
---
 kernel/sched/fair.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 6680cb39c787..0b7530b93429 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9744,8 +9744,8 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
 	if (!(sd->flags & SD_ASYM_PACKING))
 		return false;
 
-	return (!sched_smt_active()) ||
-		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
+	return (sd->flags & SD_SHARE_CPUCAPACITY) ||
+		(is_core_idle(cpu) && test_bit(cpu_core_flags(), (void *)&sd->flags));
 }
 
 static inline bool sched_asym(struct sched_domain *sd, int dst_cpu, int src_cpu)
-- 
2.43.0

Re: [PATCH v2 1/6] sched/fair: add SD_CLUSTER in comments

[Ricardo Neri]

On Tue, Jan 30, 2024 at 09:17:03PM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> The SD_CLUSTER omitted in following TOPOLOGY_SD_FLAGS explaination, add
> it to fill the absent.

Perhaps saying: "The description of SD_CLUSTER is missing. Add it." is
sufficient.

> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> To: linux-kernel@vger.kernel.org
> To: Daniel Bristot de Oliveira <bristot@redhat.com>
> To: Ben Segall <bsegall@google.com>
> To: Steven Rostedt <rostedt@goodmis.org>
> To: Dietmar Eggemann <dietmar.eggemann@arm.com>
> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> To: Valentin Schneider <vschneid@redhat.com>
> To: Vincent Guittot <vincent.guittot@linaro.org>
> To: Juri Lelli <juri.lelli@redhat.com>
> To: Peter Zijlstra <peterz@infradead.org>
> To: Ingo Molnar <mingo@redhat.com>
> ---
>  kernel/sched/topology.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 10d1391e7416..c342c52b1f34 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
>   * function:
>   *
>   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
> + *   SD_CLUSTER		    - describes Cluster topologies

This description does not really add much. Perhaps syaing "describes
CPU cluster topologies". Then users can go to the definiton of
SD_CLUSTER.

Re: [PATCH v2 4/6] sched/fair: packing func sched_use_asym_prio/sched_asym_prefer

[Ricardo Neri]

On Tue, Jan 30, 2024 at 09:17:06PM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> Packing the func sched_use_asym_prio/sched_asym_prefer into one, Using
> new func to simply code. No function change.

You should use imperative mood when writing changelogs. Also, you should
avoid abbreviations. When referring to functions,
please use (). In this specific instance, you could probably say:

     "Consolidate the functions sched_use_asym_prio() and 
      sched_asym_prefer() into one. This makes the code easier to
      read. No functional changes."
> 
> Thanks Ricardo Neri for func rename and comments suggestion!

Thanks for the mention! But no need. :)

Perhaps you could explain the reasons for renaming the functions as you
did. Explaining why you make changes is an essential part of the
changelog.

> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> To: Valentin Schneider <vschneid@redhat.com>
> To: Vincent Guittot <vincent.guittot@linaro.org>
> To: Peter Zijlstra <peterz@infradead.org>
> To: Ingo Molnar <mingo@redhat.com>
> ---
>  kernel/sched/fair.c | 34 ++++++++++++++++------------------
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index ebd659af2d78..835dbe77b260 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9745,8 +9745,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>  		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
>  }
>  
> +static inline bool sched_asym(struct sched_domain *sd, int dst_cpu, int src_cpu)
> +{
> +	/* Check if asym balance applicable, then check priorities.*/
> +	return sched_use_asym_prio(sd, dst_cpu) &&
> +		sched_asym_prefer(dst_cpu, src_cpu);
> +}
> +
>  /**
> - * sched_asym - Check if the destination CPU can do asym_packing load balance
> + * sched_group_asym - Check if the destination CPU can do asym_packing balance
>   * @env:	The load balancing environment
>   * @sgs:	Load-balancing statistics of the candidate busiest group
>   * @group:	The candidate busiest group
> @@ -9766,22 +9773,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>   * otherwise.
>   */
>  static inline bool
> -sched_asym(struct lb_env *env, struct sg_lb_stats *sgs, struct sched_group *group)
> +sched_group_asym(struct lb_env *env, struct sg_lb_stats *sgs, struct sched_group *group)
>  {
> -	/* Ensure that the whole local core is idle, if applicable. */
> -	if (!sched_use_asym_prio(env->sd, env->dst_cpu))
> -		return false;
> -
>  	/*
> -	 * CPU priorities does not make sense for SMT cores with more than one
> +	 * CPU priorities do not make sense for SMT cores with more than one
>  	 * busy sibling.
>  	 */
> -	if (group->flags & SD_SHARE_CPUCAPACITY) {
> -		if (sgs->group_weight - sgs->idle_cpus != 1)
> -			return false;
> -	}
> -
> -	return sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu);
> +	return !(group->flags & SD_SHARE_CPUCAPACITY &&
> +			sgs->group_weight - sgs->idle_cpus != 1) &&
> +		sched_asym(env->sd, env->dst_cpu, group->asym_prefer_cpu);


I am having second thoughts about compressing these conditions into a
single line. For instance, the comment above only applies to the first
condition and it is clear how the condition is met.

Checking the conditions separately makes the funcion easier to read, IMO.

Re: [PATCH v2 5/6] sched/fair: pack SD_ASYM_PACKING into sched_use_asym_prio

[Ricardo Neri]

On Tue, Jan 30, 2024 at 09:17:07PM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
pack SD_ASYM_PACKING into sched_use_asym_prio

Instead of saying `pack` you could say "Check the SD_ASYM_PACKING flag
in sched_use_asym_prio()"

> 
> Then the flags check passed into sched_asym and sched_group_asym.
> It's a code cleanup, no func changes.

I'd the changelog as follows:

"sched_use_asym_prio() checks whether CPU priorities should be used. It
makes sense to check for the SD_ASYM_PACKING() inside the function.
Since both sched_asym() and sched_group_asym() use sched_use_asym_prio(),
remove the now superfluous checks for the flag in various places"

Re: [PATCH v2 6/6 RFT] sched/fair: change sched asym checking condition

[Ricardo Neri]

On Tue, Jan 30, 2024 at 09:17:08PM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> Asym only used on SMT sd, or core sd with ITMT and core idled.
> !sched_smt_active isn't necessary.

sched_smt_active() is implemented as a static key. Thus, if SMT is not
enabled, we can quickly return without having to check the rest of the
conditions, as we should.

> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> To: linux-kernel@vger.kernel.org
> To: Valentin Schneider <vschneid@redhat.com>
> To: Daniel Bristot de Oliveira <bristot@redhat.com>
> To: Ben Segall <bsegall@google.com>
> To: Steven Rostedt <rostedt@goodmis.org>
> To: Dietmar Eggemann <dietmar.eggemann@arm.com>
> To: Vincent Guittot <vincent.guittot@linaro.org>
> To: Juri Lelli <juri.lelli@redhat.com>
> To: Peter Zijlstra <peterz@infradead.org>
> To: Ingo Molnar <mingo@redhat.com>
> ---
>  kernel/sched/fair.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 6680cb39c787..0b7530b93429 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9744,8 +9744,8 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>  	if (!(sd->flags & SD_ASYM_PACKING))
>  		return false;
>  
> -	return (!sched_smt_active()) ||
> -		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
> +	return (sd->flags & SD_SHARE_CPUCAPACITY) ||
> +		(is_core_idle(cpu) && test_bit(cpu_core_flags(), (void *)&sd->flags));

cpu_core_flags() can contain more than one flag, AFAICS. Which bit should
it check? Moreover, it is implemented differently for each architecture.
Also, as stated, in x86 asym_packing is also used in domains other than MC.

Re: [PATCH v2 3/6] sched/fair: cleanup sched_use_asym_prio

[Ricardo Neri]

On Tue, Jan 30, 2024 at 09:17:05PM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> And simplify the one line code. No function change.
> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> To: Valentin Schneider <vschneid@redhat.com>
> To: Vincent Guittot <vincent.guittot@linaro.org>
> To: Peter Zijlstra <peterz@infradead.org>
> To: Ingo Molnar <mingo@redhat.com>
> ---
>  kernel/sched/fair.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8d70417f5125..ebd659af2d78 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9741,10 +9741,8 @@ group_type group_classify(unsigned int imbalance_pct,
>   */
>  static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>  {
> -	if (!sched_smt_active())
> -		return true;
> -
> -	return sd->flags & SD_SHARE_CPUCAPACITY || is_core_idle(cpu);
> +	return (!sched_smt_active()) ||
> +		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);

I think that compressing the two conditions into one hurts readability.
As implemented, it is clear that no further checks are required if there
is no SMT.

Also, please see my comment in patch 6/6.
 

Re: [PATCH v2 3/6] sched/fair: cleanup sched_use_asym_prio

[kuiliang Shi]

On 2/1/24 9:13 AM, Ricardo Neri wrote:
> On Tue, Jan 30, 2024 at 09:17:05PM +0800, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> And simplify the one line code. No function change.
>>
>> Signed-off-by: Alex Shi <alexs@kernel.org>
>> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>> To: Valentin Schneider <vschneid@redhat.com>
>> To: Vincent Guittot <vincent.guittot@linaro.org>
>> To: Peter Zijlstra <peterz@infradead.org>
>> To: Ingo Molnar <mingo@redhat.com>
>> ---
>>  kernel/sched/fair.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 8d70417f5125..ebd659af2d78 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -9741,10 +9741,8 @@ group_type group_classify(unsigned int imbalance_pct,
>>   */
>>  static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>>  {
>> -	if (!sched_smt_active())
>> -		return true;
>> -
>> -	return sd->flags & SD_SHARE_CPUCAPACITY || is_core_idle(cpu);
>> +	return (!sched_smt_active()) ||
>> +		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
> 
> I think that compressing the two conditions into one hurts readability.

Sure, will remove this change.

Thanks
Alex

> As implemented, it is clear that no further checks are required if there
> is no SMT.
> 
> Also, please see my comment in patch 6/6.
>  

Re: [PATCH v2 5/6] sched/fair: pack SD_ASYM_PACKING into sched_use_asym_prio

[kuiliang Shi]

On 2/1/24 8:55 AM, Ricardo Neri wrote:
> On Tue, Jan 30, 2024 at 09:17:07PM +0800, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
> pack SD_ASYM_PACKING into sched_use_asym_prio
> 
> Instead of saying `pack` you could say "Check the SD_ASYM_PACKING flag
> in sched_use_asym_prio()"
> 
>>
>> Then the flags check passed into sched_asym and sched_group_asym.
>> It's a code cleanup, no func changes.
> 
> I'd the changelog as follows:
> 
> "sched_use_asym_prio() checks whether CPU priorities should be used. It
> makes sense to check for the SD_ASYM_PACKING() inside the function.
> Since both sched_asym() and sched_group_asym() use sched_use_asym_prio(),
> remove the now superfluous checks for the flag in various places"

Thanks a lot for all comments for this series patches, will take your versions in all commits' logs.

Thanks!
Alex

Re: [PATCH v2 1/6] sched/fair: add SD_CLUSTER in comments

[Shrikanth Hegde]

On 1/30/24 6:47 PM, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> The SD_CLUSTER omitted in following TOPOLOGY_SD_FLAGS explaination, add
> it to fill the absent.
> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> To: linux-kernel@vger.kernel.org
> To: Daniel Bristot de Oliveira <bristot@redhat.com>
> To: Ben Segall <bsegall@google.com>
> To: Steven Rostedt <rostedt@goodmis.org>
> To: Dietmar Eggemann <dietmar.eggemann@arm.com>
> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> To: Valentin Schneider <vschneid@redhat.com>
> To: Vincent Guittot <vincent.guittot@linaro.org>
> To: Juri Lelli <juri.lelli@redhat.com>
> To: Peter Zijlstra <peterz@infradead.org>
> To: Ingo Molnar <mingo@redhat.com>
> ---
>  kernel/sched/topology.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 10d1391e7416..c342c52b1f34 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1554,6 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
>   * function:
>   *
>   *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
> + *   SD_CLUSTER		    - describes Cluster topologies

nit: would be better if - aligns. Some additional text in between. 

>   *   SD_SHARE_PKG_RESOURCES - describes shared caches
>   *   SD_NUMA                - describes NUMA topologies
>   *


On Patch 3/6 and 6/6 agree with Ricardo. It is more difficult to understand 
when it is compressed. Some future change in this function would make it more 
difficult. 

Re: [PATCH v2 6/6 RFT] sched/fair: change sched asym checking condition

[kuiliang Shi]

On 2/1/24 9:10 AM, Ricardo Neri wrote:
> On Tue, Jan 30, 2024 at 09:17:08PM +0800, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> Asym only used on SMT sd, or core sd with ITMT and core idled.
>> !sched_smt_active isn't necessary.
> 
> sched_smt_active() is implemented as a static key. Thus, if SMT is not
> enabled, we can quickly return without having to check the rest of the
> conditions, as we should.

Hi Ricardo,

Thanks a lot for comments! I will drop this patch in this series.

But forgive my stupidity, asym feature is possible when SMT enabled instead of SMT disable. Why no SMT is a condition for asm feature? For this asym feature, I only see the SMT and MC domain use this, correct me if I'm wrong. 

> 
>>
>> Signed-off-by: Alex Shi <alexs@kernel.org>
>> To: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>> To: linux-kernel@vger.kernel.org
>> To: Valentin Schneider <vschneid@redhat.com>
>> To: Daniel Bristot de Oliveira <bristot@redhat.com>
>> To: Ben Segall <bsegall@google.com>
>> To: Steven Rostedt <rostedt@goodmis.org>
>> To: Dietmar Eggemann <dietmar.eggemann@arm.com>
>> To: Vincent Guittot <vincent.guittot@linaro.org>
>> To: Juri Lelli <juri.lelli@redhat.com>
>> To: Peter Zijlstra <peterz@infradead.org>
>> To: Ingo Molnar <mingo@redhat.com>
>> ---
>>  kernel/sched/fair.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 6680cb39c787..0b7530b93429 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -9744,8 +9744,8 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>>  	if (!(sd->flags & SD_ASYM_PACKING))
>>  		return false;
>>  
>> -	return (!sched_smt_active()) ||
>> -		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
>> +	return (sd->flags & SD_SHARE_CPUCAPACITY) ||
>> +		(is_core_idle(cpu) && test_bit(cpu_core_flags(), (void *)&sd->flags));
> 
> cpu_core_flags() can contain more than one flag, AFAICS. Which bit should
> it check? Moreover, it is implemented differently for each architecture.

It seems only x86 using the function. But there is still a error which SMT/CLUSTER domain also has this flags bit.
$ git  grep 'cpu_core_flags('
arch/x86/kernel/smpboot.c:      return cpu_core_flags() | x86_sched_itmt_flags();
include/linux/sched/topology.h:static inline int cpu_core_flags(void)

> Also, as stated, in x86 asym_packing is also used in domains other than MC.

For the feature SD_ASYM_PACKING and SD_ASYM_CPUCAPACITY, for guts of 2 features, is it possible to combine them into one, if we give a little bit more capacity to priority cpus, like 5%?

Thanks
Alex

Re: [PATCH v2 6/6 RFT] sched/fair: change sched asym checking condition

[Shrikanth Hegde]

On 2/1/24 5:10 PM, kuiliang Shi wrote:
> 
> 
> On 2/1/24 9:10 AM, Ricardo Neri wrote:
>> On Tue, Jan 30, 2024 at 09:17:08PM +0800, alexs@kernel.org wrote:
>>> From: Alex Shi <alexs@kernel.org>
>>>
>>> Asym only used on SMT sd, or core sd with ITMT and core idled.
>>> !sched_smt_active isn't necessary.
>>
>> sched_smt_active() is implemented as a static key. Thus, if SMT is not
>> enabled, we can quickly return without having to check the rest of the
>> conditions, as we should.
> 
> Hi Ricardo,
> 
> Thanks a lot for comments! I will drop this patch in this series.
> 
> But forgive my stupidity, asym feature is possible when SMT enabled instead of SMT disable. Why no SMT is a condition for asm feature? For this asym feature, I only see the SMT and MC domain use this, correct me if I'm wrong. 
> 

on power7 ASYM_PACKING is used to pack at SMT level. 

On x86, ITMT topology uses ASYM_PACKING to do load balancing instead of using different cpu capacities.

Its possible to have it in PKG(earlier referred as DIE) as well. 
In powerpc recently we did that for shared processor LPAR's. So asym feature is in PKG as well. 

>>
>>>

Re: [PATCH v2 6/6 RFT] sched/fair: change sched asym checking condition

[Ricardo Neri]

On Thu, Feb 01, 2024 at 08:49:05PM +0530, Shrikanth Hegde wrote:
> 
> 
> On 2/1/24 5:10 PM, kuiliang Shi wrote:
> > 
> > 
> > On 2/1/24 9:10 AM, Ricardo Neri wrote:
> >> On Tue, Jan 30, 2024 at 09:17:08PM +0800, alexs@kernel.org wrote:
> >>> From: Alex Shi <alexs@kernel.org>
> >>>
> >>> Asym only used on SMT sd, or core sd with ITMT and core idled.
> >>> !sched_smt_active isn't necessary.
> >>
> >> sched_smt_active() is implemented as a static key. Thus, if SMT is not
> >> enabled, we can quickly return without having to check the rest of the
> >> conditions, as we should.
> > 
> > Hi Ricardo,
> > 
> > Thanks a lot for comments! I will drop this patch in this series.
> > 
> > But forgive my stupidity, asym feature is possible when SMT enabled instead of SMT disable. Why no SMT is a condition for asm feature? For this asym feature, I only see the SMT and MC domain use this, correct me if I'm wrong. 
> > 
> 
> on power7 ASYM_PACKING is used to pack at SMT level. 

Indeed, this is why the function returns true if it the sched domain has
the SD_SHARE_CPUCAPACITY flag.

When SMT is disabled there is no point in doing any check because we will
always want to use asym_packing.

> 
> On x86, ITMT topology uses ASYM_PACKING to do load balancing instead of using different cpu capacities.

You can look at x86_cluster_flags() and x86_die_flags().

> 
> Its possible to have it in PKG(earlier referred as DIE) as well. 
> In powerpc recently we did that for shared processor LPAR's. So asym feature is in PKG as well. 
> 
> >>
> >>>