[PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[alexs]

From: Alex Shi <alexs@kernel.org>

The description of SD_CLUSTER is missing. Add it.

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: 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..8b45f16a1890 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 CPU Cluster topologies
  *   SD_SHARE_PKG_RESOURCES - describes shared caches
  *   SD_NUMA                - describes NUMA topologies
  *
-- 
2.43.0

[PATCH v3 2/4] 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 v3 3/4] sched/fair: packing func sched_use_asym_prio()/sched_asym_prefer()

[alexs]

From: Alex Shi <alexs@kernel.org>

Consolidate the functions sched_use_asym_prio() and sched_asym_prefer()
into one. and rename sched_asym() as sched_group_asym().
This makes the code easier to read. No functional changes.

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 | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 8d70417f5125..44fd5e2ca642 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9747,8 +9747,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
 	return 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
@@ -9768,22 +9775,18 @@ 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);
+	if ((group->flags & SD_SHARE_CPUCAPACITY) &&
+	    (sgs->group_weight - sgs->idle_cpus != 1))
+		return false;
+
+	return 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 */
@@ -9939,7 +9942,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;
 	}
 
@@ -11038,8 +11041,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;
 
@@ -11909,8 +11911,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 v3 4/4] sched/fair: Check the SD_ASYM_PACKING flag in sched_use_asym_prio()

[alexs]

From: Alex Shi <alexs@kernel.org>

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"

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 44fd5e2ca642..bd32efbea688 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;
+
 	if (!sched_smt_active())
 		return true;
 
@@ -9940,11 +9943,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))
@@ -11040,9 +11041,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) {
@@ -11138,7 +11137,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

Re: [PATCH v3 2/4] sched/fair: remove unused parameters

[Valentin Schneider]

On 01/02/24 19:54, alexs@kernel.org wrote:
> 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>

AFAICT this is unsused as of:
  c9ca07886aaa ("sched/fair: Do not even the number of busy CPUs via asym_packing")

Reviewed-by: Valentin Schneider <vschneid@redhat.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

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[Valentin Schneider]

Subject nit: the prefix should be sched/topology

On 01/02/24 19:54, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
>
> The description of SD_CLUSTER is missing. Add it.
>
> 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: 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..8b45f16a1890 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 CPU Cluster topologies

So I know this is the naming we've gone for the "Cluster" naming, but this
comment isn't really explaining anything.

include/linux/sched/sd_flags.h has a bit more info already:
 * Domain members share CPU cluster (LLC tags or L2 cache)

I had to go through a bit of git history to remember what the CLUSTER thing
was about, how about this:

* SD_CLUSTER             - describes shared shared caches, cache tags or busses
* SD_SHARE_PKG_RESOURCES - describes shared LLC cache

And looking at this it would make sense to:
  rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
  rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
but that's another topic...

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

Re: [PATCH v3 3/4] sched/fair: packing func sched_use_asym_prio()/sched_asym_prefer()

[kuiliang Shi]

Hi Ricardo,

Since your good suggestions took in this and the next patch, do you mind to give Reviewed-by for both of them?

Thanks
Alex

On 2/1/24 7:54 PM, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> Consolidate the functions sched_use_asym_prio() and sched_asym_prefer()
> into one. and rename sched_asym() as sched_group_asym().
> This makes the code easier to read. No functional changes.
> 
> 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 | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8d70417f5125..44fd5e2ca642 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9747,8 +9747,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>  	return 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
> @@ -9768,22 +9775,18 @@ 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);
> +	if ((group->flags & SD_SHARE_CPUCAPACITY) &&
> +	    (sgs->group_weight - sgs->idle_cpus != 1))
> +		return false;
> +
> +	return 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 */
> @@ -9939,7 +9942,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;
>  	}
>  
> @@ -11038,8 +11041,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;
>  
> @@ -11909,8 +11911,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;
>  			}

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[kuiliang Shi]

On 2/2/24 10:27 PM, Valentin Schneider wrote:
> 
> Subject nit: the prefix should be sched/topology
> 
> On 01/02/24 19:54, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> The description of SD_CLUSTER is missing. Add it.
>>
>> 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: 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..8b45f16a1890 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 CPU Cluster topologies
> 
> So I know this is the naming we've gone for the "Cluster" naming, but this
> comment isn't really explaining anything.
> 
> include/linux/sched/sd_flags.h has a bit more info already:
>  * Domain members share CPU cluster (LLC tags or L2 cache)
> 
> I had to go through a bit of git history to remember what the CLUSTER thing
> was about, how about this:
> 
> * SD_CLUSTER             - describes shared shared caches, cache tags or busses

Double "shared", so could we use:
* SD_CLUSTER             - describes shared caches, cache tags or busses?


> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
> 
> And looking at this it would make sense to:
>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
> but that's another topic...

Uh, naming is a hard things. :)

Thanks
Alex
> 
>>   *   SD_SHARE_PKG_RESOURCES - describes shared caches
>>   *   SD_NUMA                - describes NUMA topologies
>>   *
>> --
>> 2.43.0
> 

Re: [PATCH v3 2/4] sched/fair: remove unused parameters

[Ricardo Neri]

On Fri, Feb 02, 2024 at 03:27:24PM +0100, Valentin Schneider wrote:
> On 01/02/24 19:54, alexs@kernel.org wrote:
> > 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>
> 
> AFAICT this is unsused as of:
>   c9ca07886aaa ("sched/fair: Do not even the number of busy CPUs via asym_packing")

Maybe a Fixes: c9ca07886aaa ("sched/fair: Do not even the number of busy CPUs via asym_packing")
is in order?

Re: [PATCH v3 3/4] sched/fair: packing func sched_use_asym_prio()/sched_asym_prefer()

[Ricardo Neri]

On Thu, Feb 01, 2024 at 07:54:46PM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>

subject:

sched/fair: packing func sched_use_asym_prio()/sched_asym_prefer()

Do not use gerund mood in the subject. Better to say:
sched/fair: Rework sched_use_asym_prio() and sched_asym_prefer()
> 
> Consolidate the functions sched_use_asym_prio() and sched_asym_prefer()
> into one. and rename sched_asym() as sched_group_asym().
> This makes the code easier to read. No functional changes.

Maybe giving more reasons?

sched_use_asym_prio() sched_asym_prefer() are used together in various
places. Consolidate them into a single function sched_asym().

The existing sched_group_asym() is only used when collecting statistics
of a scheduling group. Rename it as sched_group_asym().

> 
> 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 | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8d70417f5125..44fd5e2ca642 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9747,8 +9747,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>  	return 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.*/

Perhaps the comment can be made more descriptive:
	/*
	 * First check if @dst_cpu can do asym_packing load balance. Only do it
	 * if it has higher priority than @src_cpu.
	 */
> +	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
> @@ -9768,22 +9775,18 @@ 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);

After applying this patch there is a blank line between the comment and the
return statement. Can you remove it?

> +	if ((group->flags & SD_SHARE_CPUCAPACITY) &&
> +	    (sgs->group_weight - sgs->idle_cpus != 1))
> +		return false;
> +
> +	return 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 */
> @@ -9939,7 +9942,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;
>  	}
>  
> @@ -11038,8 +11041,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;
>  
> @@ -11909,8 +11911,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
> 

Re: [PATCH v3 4/4] sched/fair: Check the SD_ASYM_PACKING flag in sched_use_asym_prio()

[Ricardo Neri]

On Thu, Feb 01, 2024 at 07:54:47PM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> 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"

s/places"/places./

> 
> 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 44fd5e2ca642..bd32efbea688 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;
> +
>  	if (!sched_smt_active())
>  		return true;
>  
> @@ -9940,11 +9943,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))

You should align sched_group_asym() to !local_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))
> @@ -11040,9 +11041,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) {
> @@ -11138,7 +11137,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) ||

Perhaps you can rearrange the spaghetti of conditions to make better use of
the full 80-column line.

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[Ricardo Neri]

On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
> 
> Subject nit: the prefix should be sched/topology
> 
> On 01/02/24 19:54, alexs@kernel.org wrote:
> > From: Alex Shi <alexs@kernel.org>
> >
> > The description of SD_CLUSTER is missing. Add it.
> >
> > 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: 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..8b45f16a1890 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 CPU Cluster topologies
> 
> So I know this is the naming we've gone for the "Cluster" naming, but this
> comment isn't really explaining anything.
> 
> include/linux/sched/sd_flags.h has a bit more info already:
>  * Domain members share CPU cluster (LLC tags or L2 cache)

I also thought of this, but I didn't want to suggest to repeat in topolog.c
what is described in sd_flags.h.

Maybe it is better to remove the descriptions of all flags here and instead
direct the reader to sd_flags.h?

> 
> I had to go through a bit of git history to remember what the CLUSTER thing
> was about, how about this:
> 
> * SD_CLUSTER             - describes shared shared caches, cache tags or busses

AFAIK, this describes a subset of CPUs in the package that share a
resource, likely L2 cache.

> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
> 
> And looking at this it would make sense to:
>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES

but not all CPUs in the package share the resource

>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
> but that's another topic...
> 

Re: [PATCH v3 4/4] sched/fair: Check the SD_ASYM_PACKING flag in sched_use_asym_prio()

[kuiliang Shi]

On 2/6/24 6:38 AM, Ricardo Neri wrote:
> On Thu, Feb 01, 2024 at 07:54:47PM +0800, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> 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"
> 
> s/places"/places./
> 
>>
>> 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 44fd5e2ca642..bd32efbea688 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;
>> +
>>  	if (!sched_smt_active())
>>  		return true;
>>  
>> @@ -9940,11 +9943,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))
> 
> You should align sched_group_asym() to !local_group.

Thanks for all suggestion. I will take them in next version.

> 
>>  		sgs->group_asym_packing = 1;
>> -	}
>>  
>>  	/* Check for loaded SMT group to be balanced to dst CPU */
>>  	if (!local_group && smt_balance(env, sgs, group))
>> @@ -11040,9 +11041,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) {
>> @@ -11138,7 +11137,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) ||
> 
> Perhaps you can rearrange the spaghetti of conditions to make better use of
> the full 80-column line.

80-column doesn't work here, will try 100 column.

Thanks
Alex

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[Yicong Yang]

On 2024/2/2 22:27, Valentin Schneider wrote:
> 
> Subject nit: the prefix should be sched/topology
> 
> On 01/02/24 19:54, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> The description of SD_CLUSTER is missing. Add it.
>>
>> 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: 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..8b45f16a1890 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 CPU Cluster topologies
> 
> So I know this is the naming we've gone for the "Cluster" naming, but this
> comment isn't really explaining anything.
> 
> include/linux/sched/sd_flags.h has a bit more info already:
>  * Domain members share CPU cluster (LLC tags or L2 cache)
> 

Cluster topology in scheduler should mean CPUs beyond the SMT which are sharing
some cache resources (currently L2 on some Intel platforms or L3 Tag on our platforms)
but not the LLC.

A drawing in c5e22feffdd7 ("topology: Represent clusters of CPUs within a die") has
a good illustration and comment of cpus_share_resources() also illustrate this a bit:

/*
 * Whether CPUs are share cache resources, which means LLC on non-cluster
 * machines and LLC tag or L2 on machines with clusters.
 */
bool cpus_share_resources(int this_cpu, int that_cpu)

> I had to go through a bit of git history to remember what the CLUSTER thing
> was about, how about this:
> 
> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
> 
> And looking at this it would make sense to:
>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
> but that's another topic...
> 
>>   *   SD_SHARE_PKG_RESOURCES - describes shared caches
>>   *   SD_NUMA                - describes NUMA topologies
>>   *
>> --
>> 2.43.0
> 
> 
> .
> 

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[kuiliang Shi]

On 2/6/24 10:46 AM, Ricardo Neri wrote:
> On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
>>
>> Subject nit: the prefix should be sched/topology
>>
>> On 01/02/24 19:54, alexs@kernel.org wrote:
>>> From: Alex Shi <alexs@kernel.org>
>>>
>>> The description of SD_CLUSTER is missing. Add it.
>>>
>>> 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: 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..8b45f16a1890 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 CPU Cluster topologies
>>
>> So I know this is the naming we've gone for the "Cluster" naming, but this
>> comment isn't really explaining anything.
>>
>> include/linux/sched/sd_flags.h has a bit more info already:
>>  * Domain members share CPU cluster (LLC tags or L2 cache)
> 
> I also thought of this, but I didn't want to suggest to repeat in topolog.c
> what is described in sd_flags.h.
> 
> Maybe it is better to remove the descriptions of all flags here and instead
> direct the reader to sd_flags.h?

yes, good idea for getting their meaning directly from the header file. 
So is it fine for next sending?

    sched/fair: remove duplicate comments for SD_ flags
    
    Originally, it missed SD_CLUSTER flag description, but comparing to copy
    the flags meaning from sd_flags.h, reader is better to get info from
    there.
    
    Keep SD_ASYM_PACKING unchange for a point from another way.
    
    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: Juri Lelli <juri.lelli@redhat.com>
    To: Peter Zijlstra <peterz@infradead.org>
    To: Ingo Molnar <mingo@redhat.com>

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 10d1391e7416..96a24bf954ad 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1551,11 +1551,7 @@ static struct cpumask            ***sched_domains_numa_masks;
  *
  * These flags are purely descriptive of the topology and do not prescribe
  * behaviour. Behaviour is artificial and mapped in the below sd_init()
- * function:
- *
- *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
- *   SD_SHARE_PKG_RESOURCES - describes shared caches
- *   SD_NUMA                - describes NUMA topologies
+ * function, for details in include/linux/sched/sd_flags.h:
  *
  * Odd one out, which beside describing the topology has a quirk also
  * prescribes the desired behaviour that goes along with it:
> 
>>
>> I had to go through a bit of git history to remember what the CLUSTER thing
>> was about, how about this:
>>
>> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
> 
> AFAIK, this describes a subset of CPUs in the package that share a
> resource, likely L2 cache.
> 
>> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
>>
>> And looking at this it would make sense to:
>>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
> 
> but not all CPUs in the package share the resource
> 
>>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
>> but that's another topic...
>>

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[Valentin Schneider]

On 05/02/24 18:46, Ricardo Neri wrote:
> On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
>>
>> Subject nit: the prefix should be sched/topology
>>
>> On 01/02/24 19:54, alexs@kernel.org wrote:
>> > From: Alex Shi <alexs@kernel.org>
>> >
>> > The description of SD_CLUSTER is missing. Add it.
>> >
>> > 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: 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..8b45f16a1890 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 CPU Cluster topologies
>>
>> So I know this is the naming we've gone for the "Cluster" naming, but this
>> comment isn't really explaining anything.
>>
>> include/linux/sched/sd_flags.h has a bit more info already:
>>  * Domain members share CPU cluster (LLC tags or L2 cache)
>
> I also thought of this, but I didn't want to suggest to repeat in topolog.c
> what is described in sd_flags.h.
>
> Maybe it is better to remove the descriptions of all flags here and instead
> direct the reader to sd_flags.h?
>

Yeah I agree on less duplication.

>>
>> I had to go through a bit of git history to remember what the CLUSTER thing
>> was about, how about this:
>>
>> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
>
> AFAIK, this describes a subset of CPUs in the package that share a
> resource, likely L2 cache.
>
>> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
>>
>> And looking at this it would make sense to:
>>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
>
> but not all CPUs in the package share the resource

But SD_CLUSTER never expands beyond the package, right?

Regardless, my main point is that having both SD_CLUSTER and
SD_SHARE_PKG_RESOURCES is a source of confusion (at the very least for
myself), and given SD_SHARE_PKG_RESOURCES is really used to mean "shares
LLC" (see update_top_cache_domain()), we could make that flag more explicit
and lift some ambiguity with SD_CLUSTER.

>
>>   rename SD_SHARE_PKG_RESOURCES into SD_SHARE_LLC
>> but that's another topic...
>>

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[Ricardo Neri]

On Tue, Feb 06, 2024 at 04:56:02PM +0800, kuiliang Shi wrote:
> 
> 
> On 2/6/24 10:46 AM, Ricardo Neri wrote:
> > On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
> >>
> >> Subject nit: the prefix should be sched/topology
> >>
> >> On 01/02/24 19:54, alexs@kernel.org wrote:
> >>> From: Alex Shi <alexs@kernel.org>
> >>>
> >>> The description of SD_CLUSTER is missing. Add it.
> >>>
> >>> 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: 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..8b45f16a1890 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 CPU Cluster topologies
> >>
> >> So I know this is the naming we've gone for the "Cluster" naming, but this
> >> comment isn't really explaining anything.
> >>
> >> include/linux/sched/sd_flags.h has a bit more info already:
> >>  * Domain members share CPU cluster (LLC tags or L2 cache)
> > 
> > I also thought of this, but I didn't want to suggest to repeat in topolog.c
> > what is described in sd_flags.h.
> > 
> > Maybe it is better to remove the descriptions of all flags here and instead
> > direct the reader to sd_flags.h?
> 
> yes, good idea for getting their meaning directly from the header file. 
> So is it fine for next sending?

Some tweaks below.

> 
>     sched/fair: remove duplicate comments for SD_ flags

      sched/topology: Remove duplicate descriptions from TOPOLOGY_SD_FLAGS
>     
>     Originally, it missed SD_CLUSTER flag description, but comparing to copy
>     the flags meaning from sd_flags.h, reader is better to get info from
>     there.

      These flags are already documented in include/linux/sched/sd_flags.h.

      Keep the comment on SD_ASYM_PACKING as it is a special case.
>     
>     Keep SD_ASYM_PACKING unchange for a point from another way.
>     
>     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: Juri Lelli <juri.lelli@redhat.com>
>     To: Peter Zijlstra <peterz@infradead.org>
>     To: Ingo Molnar <mingo@redhat.com>
> 
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 10d1391e7416..96a24bf954ad 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1551,11 +1551,7 @@ static struct cpumask            ***sched_domains_numa_masks;
>   *
>   * These flags are purely descriptive of the topology and do not prescribe
>   * behaviour. Behaviour is artificial and mapped in the below sd_init()
> - * function:
> - *
> - *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
> - *   SD_SHARE_PKG_RESOURCES - describes shared caches
> - *   SD_NUMA                - describes NUMA topologies

Maybe only remove the descriptions but keep the enumeration?

> + * function, for details in include/linux/sched/sd_flags.h:

      function. For details, see include/linux/sched/sd_flags.h.

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[Ricardo Neri]

On Tue, Feb 06, 2024 at 02:16:06PM +0100, Valentin Schneider wrote:
> On 05/02/24 18:46, Ricardo Neri wrote:
> > On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
> >>
> >> Subject nit: the prefix should be sched/topology
> >>
> >> On 01/02/24 19:54, alexs@kernel.org wrote:
> >> > From: Alex Shi <alexs@kernel.org>
> >> >
> >> > The description of SD_CLUSTER is missing. Add it.
> >> >
> >> > 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: 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..8b45f16a1890 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 CPU Cluster topologies
> >>
> >> So I know this is the naming we've gone for the "Cluster" naming, but this
> >> comment isn't really explaining anything.
> >>
> >> include/linux/sched/sd_flags.h has a bit more info already:
> >>  * Domain members share CPU cluster (LLC tags or L2 cache)
> >
> > I also thought of this, but I didn't want to suggest to repeat in topolog.c
> > what is described in sd_flags.h.
> >
> > Maybe it is better to remove the descriptions of all flags here and instead
> > direct the reader to sd_flags.h?
> >
> 
> Yeah I agree on less duplication.
> 
> >>
> >> I had to go through a bit of git history to remember what the CLUSTER thing
> >> was about, how about this:
> >>
> >> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
> >
> > AFAIK, this describes a subset of CPUs in the package that share a
> > resource, likely L2 cache.
> >
> >> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
> >>
> >> And looking at this it would make sense to:
> >>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
> >
> > but not all CPUs in the package share the resource
> 
> But SD_CLUSTER never expands beyond the package, right?

Correct.

> 
> Regardless, my main point is that having both SD_CLUSTER and
> SD_SHARE_PKG_RESOURCES is a source of confusion (at the very least for
> myself),

Agreed!

> and given SD_SHARE_PKG_RESOURCES is really used to mean "shares
> LLC" (see update_top_cache_domain()), we could make that flag more explicit
> and lift some ambiguity with SD_CLUSTER.

As Yicong stated, cluster topology should mean CPUs beyond SMT that share
some resource but not LLC.

It makes sense to me to keep SD_CLUSTER name as it is today and rename
SD_SHARE_PKG_RESOURCES as SD_SHARE_LLC.

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[kuiliang Shi]

On 2/7/24 5:57 AM, Ricardo Neri wrote:
> On Tue, Feb 06, 2024 at 02:16:06PM +0100, Valentin Schneider wrote:
>> On 05/02/24 18:46, Ricardo Neri wrote:
>>> On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
>>>>
>>>> Subject nit: the prefix should be sched/topology
>>>>
>>>> On 01/02/24 19:54, alexs@kernel.org wrote:
>>>>> From: Alex Shi <alexs@kernel.org>
>>>>>
>>>>> The description of SD_CLUSTER is missing. Add it.
>>>>>
>>>>> 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: 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..8b45f16a1890 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 CPU Cluster topologies
>>>>
>>>> So I know this is the naming we've gone for the "Cluster" naming, but this
>>>> comment isn't really explaining anything.
>>>>
>>>> include/linux/sched/sd_flags.h has a bit more info already:
>>>>  * Domain members share CPU cluster (LLC tags or L2 cache)
>>>
>>> I also thought of this, but I didn't want to suggest to repeat in topolog.c
>>> what is described in sd_flags.h.
>>>
>>> Maybe it is better to remove the descriptions of all flags here and instead
>>> direct the reader to sd_flags.h?
>>>
>>
>> Yeah I agree on less duplication.
>>
>>>>
>>>> I had to go through a bit of git history to remember what the CLUSTER thing
>>>> was about, how about this:
>>>>
>>>> * SD_CLUSTER             - describes shared shared caches, cache tags or busses
>>>
>>> AFAIK, this describes a subset of CPUs in the package that share a
>>> resource, likely L2 cache.
>>>
>>>> * SD_SHARE_PKG_RESOURCES - describes shared LLC cache
>>>>
>>>> And looking at this it would make sense to:
>>>>   rename SD_CLUSTER into SD_SHARE_PKG_RESOURCES
>>>
>>> but not all CPUs in the package share the resource
>>
>> But SD_CLUSTER never expands beyond the package, right?
> 
> Correct.
> 
>>
>> Regardless, my main point is that having both SD_CLUSTER and
>> SD_SHARE_PKG_RESOURCES is a source of confusion (at the very least for
>> myself),
> 
> Agreed!
> 
>> and given SD_SHARE_PKG_RESOURCES is really used to mean "shares
>> LLC" (see update_top_cache_domain()), we could make that flag more explicit
>> and lift some ambiguity with SD_CLUSTER.
> 
> As Yicong stated, cluster topology should mean CPUs beyond SMT that share
> some resource but not LLC.
> 
> It makes sense to me to keep SD_CLUSTER name as it is today and rename
> SD_SHARE_PKG_RESOURCES as SD_SHARE_LLC.

yes, agree with his.

Re: [PATCH v3 1/4] sched/fair: add SD_CLUSTER in comments

[kuiliang Shi]

On 2/7/24 5:24 AM, Ricardo Neri wrote:
> On Tue, Feb 06, 2024 at 04:56:02PM +0800, kuiliang Shi wrote:
>>
>>
>> On 2/6/24 10:46 AM, Ricardo Neri wrote:
>>> On Fri, Feb 02, 2024 at 03:27:32PM +0100, Valentin Schneider wrote:
>>>>
>>>> Subject nit: the prefix should be sched/topology
>>>>
>>>> On 01/02/24 19:54, alexs@kernel.org wrote:
>>>>> From: Alex Shi <alexs@kernel.org>
>>>>>
>>>>> The description of SD_CLUSTER is missing. Add it.
>>>>>
>>>>> 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: 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..8b45f16a1890 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 CPU Cluster topologies
>>>>
>>>> So I know this is the naming we've gone for the "Cluster" naming, but this
>>>> comment isn't really explaining anything.
>>>>
>>>> include/linux/sched/sd_flags.h has a bit more info already:
>>>>  * Domain members share CPU cluster (LLC tags or L2 cache)
>>>
>>> I also thought of this, but I didn't want to suggest to repeat in topolog.c
>>> what is described in sd_flags.h.
>>>
>>> Maybe it is better to remove the descriptions of all flags here and instead
>>> direct the reader to sd_flags.h?
>>
>> yes, good idea for getting their meaning directly from the header file. 
>> So is it fine for next sending?
> 
> Some tweaks below.
> 
>>
>>     sched/fair: remove duplicate comments for SD_ flags
> 
>       sched/topology: Remove duplicate descriptions from TOPOLOGY_SD_FLAGS
>>     
>>     Originally, it missed SD_CLUSTER flag description, but comparing to copy
>>     the flags meaning from sd_flags.h, reader is better to get info from
>>     there.
> 
>       These flags are already documented in include/linux/sched/sd_flags.h.
> 
>       Keep the comment on SD_ASYM_PACKING as it is a special case.
>>     
>>     Keep SD_ASYM_PACKING unchange for a point from another way.
>>     
>>     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: Juri Lelli <juri.lelli@redhat.com>
>>     To: Peter Zijlstra <peterz@infradead.org>
>>     To: Ingo Molnar <mingo@redhat.com>
>>
>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>> index 10d1391e7416..96a24bf954ad 100644
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -1551,11 +1551,7 @@ static struct cpumask            ***sched_domains_numa_masks;
>>   *
>>   * These flags are purely descriptive of the topology and do not prescribe
>>   * behaviour. Behaviour is artificial and mapped in the below sd_init()
>> - * function:
>> - *
>> - *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
>> - *   SD_SHARE_PKG_RESOURCES - describes shared caches
>> - *   SD_NUMA                - describes NUMA topologies
> 
> Maybe only remove the descriptions but keep the enumeration?
> 
>> + * function, for details in include/linux/sched/sd_flags.h:
> 
>       function. For details, see include/linux/sched/sd_flags.h.
> 

Thanks for comments. will update in next version.
>