All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] sched/fair: add SD_CLUSTER in comments
@ 2024-01-17  8:57 alexs
  2024-01-17  8:57 ` [PATCH 2/5] sched/fair: remove unused parameters alexs
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: alexs @ 2024-01-17  8:57 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
  Cc: Alex Shi

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


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

* [PATCH 2/5] sched/fair: remove unused parameters
  2024-01-17  8:57 [PATCH 1/5] sched/fair: add SD_CLUSTER in comments alexs
@ 2024-01-17  8:57 ` alexs
  2024-01-26  0:18   ` Ricardo Neri
  2024-01-17  8:57 ` [PATCH 3/5] sched/fair: cleanup sched_use_asym_prio alexs
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: alexs @ 2024-01-17  8:57 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
  Cc: Alex Shi

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


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

* [PATCH 3/5] sched/fair: cleanup sched_use_asym_prio
  2024-01-17  8:57 [PATCH 1/5] sched/fair: add SD_CLUSTER in comments alexs
  2024-01-17  8:57 ` [PATCH 2/5] sched/fair: remove unused parameters alexs
@ 2024-01-17  8:57 ` alexs
  2024-01-17  8:57 ` [PATCH 4/5] sched/fair: add a func _sched_asym alexs
  2024-01-17  8:57 ` [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario alexs
  3 siblings, 0 replies; 13+ messages in thread
From: alexs @ 2024-01-17  8:57 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
  Cc: Alex Shi

From: Alex Shi <alexs@kernel.org>

And simplify the one line code. No function change.

Signed-off-by: Alex Shi <alexs@kernel.org>
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


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

* [PATCH 4/5] sched/fair: add a func _sched_asym
  2024-01-17  8:57 [PATCH 1/5] sched/fair: add SD_CLUSTER in comments alexs
  2024-01-17  8:57 ` [PATCH 2/5] sched/fair: remove unused parameters alexs
  2024-01-17  8:57 ` [PATCH 3/5] sched/fair: cleanup sched_use_asym_prio alexs
@ 2024-01-17  8:57 ` alexs
  2024-01-25 21:56   ` Ricardo Neri
  2024-01-17  8:57 ` [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario alexs
  3 siblings, 1 reply; 13+ messages in thread
From: alexs @ 2024-01-17  8:57 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
  Cc: Alex Shi

From: Alex Shi <alexs@kernel.org>

Use this func in sched_asym and other path to simply code.
No function change.

Signed-off-by: Alex Shi <alexs@kernel.org>
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 | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index ebd659af2d78..96163ab69ae0 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9745,6 +9745,14 @@ 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 repl_cpu)
+{
+	/* Ensure that the whole local core is idle, if applicable. */
+	return sched_use_asym_prio(sd, dst_cpu) &&
+			sched_asym_prefer(dst_cpu, repl_cpu);
+}
+
 /**
  * sched_asym - Check if the destination CPU can do asym_packing load balance
  * @env:	The load balancing environment
@@ -9768,20 +9776,13 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
 static inline bool
 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))
-		return false;
-
 	/*
 	 * CPU priorities does 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 */
@@ -11036,8 +11037,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 +11907,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


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

* [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario
  2024-01-17  8:57 [PATCH 1/5] sched/fair: add SD_CLUSTER in comments alexs
                   ` (2 preceding siblings ...)
  2024-01-17  8:57 ` [PATCH 4/5] sched/fair: add a func _sched_asym alexs
@ 2024-01-17  8:57 ` alexs
  2024-01-23  8:47   ` Shrikanth Hegde
  3 siblings, 1 reply; 13+ messages in thread
From: alexs @ 2024-01-17  8:57 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
  Cc: Alex Shi

From: Alex Shi <alexs@kernel.org>

Current function doesn't match it's comments, in fact, core_idle
checking is only meaningful with non-SMT.
So make the function right.

Signed-off-by: Alex Shi <alexs@kernel.org>
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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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


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

* Re: [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario
  2024-01-17  8:57 ` [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario alexs
@ 2024-01-23  8:47   ` Shrikanth Hegde
  2024-01-25  9:35     ` kuiliang Shi
  2024-01-26  0:04     ` Ricardo Neri
  0 siblings, 2 replies; 13+ messages in thread
From: Shrikanth Hegde @ 2024-01-23  8:47 UTC (permalink / raw)
  To: alexs
  Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel



On 1/17/24 2:27 PM, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> Current function doesn't match it's comments, in fact, core_idle
> checking is only meaningful with non-SMT.
> So make the function right.
> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 96163ab69ae0..0a321f639c79 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9741,8 +9741,8 @@ group_type group_classify(unsigned int imbalance_pct,
>   */
>  static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>  {
> -	return (!sched_smt_active()) ||
> -		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
> +	return	(sd->flags & SD_SHARE_CPUCAPACITY) ||
> +		(!sched_smt_active() && is_core_idle(cpu));
>  }

This seems wrong. This would always return false for higher than SMT domains 
if smt is active. 

Was this meant to be sched_smt_active() && is_core_idle(cpu)? 

>  
>  static inline bool _sched_asym(struct sched_domain *sd,

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

* Re: [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario
  2024-01-23  8:47   ` Shrikanth Hegde
@ 2024-01-25  9:35     ` kuiliang Shi
  2024-01-26  0:06       ` Ricardo Neri
  2024-01-26  0:04     ` Ricardo Neri
  1 sibling, 1 reply; 13+ messages in thread
From: kuiliang Shi @ 2024-01-25  9:35 UTC (permalink / raw)
  To: Shrikanth Hegde, alexs
  Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel



On 1/23/24 4:47 PM, Shrikanth Hegde wrote:
> 
> 
> On 1/17/24 2:27 PM, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> Current function doesn't match it's comments, in fact, core_idle
>> checking is only meaningful with non-SMT.
>> So make the function right.
>>
>> Signed-off-by: Alex Shi <alexs@kernel.org>
>> 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 | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 96163ab69ae0..0a321f639c79 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -9741,8 +9741,8 @@ group_type group_classify(unsigned int imbalance_pct,
>>   */
>>  static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>>  {
>> -	return (!sched_smt_active()) ||
>> -		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
>> +	return	(sd->flags & SD_SHARE_CPUCAPACITY) ||
>> +		(!sched_smt_active() && is_core_idle(cpu));
>>  }
> 
> This seems wrong. This would always return false for higher than SMT domains 
> if smt is active. 
> 

yes, thanks for point out.

> Was this meant to be sched_smt_active() && is_core_idle(cpu)? 

In theory, yes, it should like this. But I have no ASYM device to test. :(

Thanks!
Alex

> 
>>  
>>  static inline bool _sched_asym(struct sched_domain *sd,

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

* Re: [PATCH 4/5] sched/fair: add a func _sched_asym
  2024-01-17  8:57 ` [PATCH 4/5] sched/fair: add a func _sched_asym alexs
@ 2024-01-25 21:56   ` Ricardo Neri
  2024-01-26  1:57     ` kuiliang Shi
  0 siblings, 1 reply; 13+ messages in thread
From: Ricardo Neri @ 2024-01-25 21:56 UTC (permalink / raw)
  To: alexs
  Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel

On Wed, Jan 17, 2024 at 04:57:14PM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> Use this func in sched_asym and other path to simply code.
> No function change.
> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> 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 | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index ebd659af2d78..96163ab69ae0 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9745,6 +9745,14 @@ 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 repl_cpu)

What does repl_cpu mean? Maybe renaming to src_cpu?

> +{
> +	/* Ensure that the whole local core is idle, if applicable. */
> +	return sched_use_asym_prio(sd, dst_cpu) &&
> +			sched_asym_prefer(dst_cpu, repl_cpu);

The comment no longer applies to the whole expression. Perhaps rewording is
in order. First we check for the whole idle core if applicable (i.e., when
not balancing among SMT siblings). Then we check priorities.

Also, indentation should be aligned with `return`, IMO.

> +}
> +
>  /**
>   * sched_asym - Check if the destination CPU can do asym_packing load balance
>   * @env:	The load balancing environment
> @@ -9768,20 +9776,13 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>  static inline bool
>  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))
> -		return false;
> -
>  	/*
>  	 * CPU priorities does not make sense for SMT cores with more than one
>  	 * busy sibling.

While here, it might be good to fix a syntax error above: s/does/do/.

>  	 */
> -	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);

Perhaps we can come up with a better name than _sched_asym(). After this
patch the difference between sched_asym() and _sched_asym() is that the
former considers the stats of the source group. Maybe sched_asym() can be
renamed as sched_group_asym(); it is only used in update_sg_lb_stats().
Your _sched_asym() can become sched_asym().

>  }
>  
>  /* One group has more than one SMT CPU while the other group does not */
> @@ -11036,8 +11037,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 +11907,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
> 

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

* Re: [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario
  2024-01-23  8:47   ` Shrikanth Hegde
  2024-01-25  9:35     ` kuiliang Shi
@ 2024-01-26  0:04     ` Ricardo Neri
  1 sibling, 0 replies; 13+ messages in thread
From: Ricardo Neri @ 2024-01-26  0:04 UTC (permalink / raw)
  To: Shrikanth Hegde
  Cc: alexs, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel

On Tue, Jan 23, 2024 at 02:17:00PM +0530, Shrikanth Hegde wrote:
> 
> 
> On 1/17/24 2:27 PM, alexs@kernel.org wrote:
> > From: Alex Shi <alexs@kernel.org>
> > 
> > Current function doesn't match it's comments, in fact, core_idle
> > checking is only meaningful with non-SMT.

For SMT cores, we _do_ need to check for a whole core to be idle when
deciding to use asym_packing priorities when balancing between cores, but
not in SMT domains. This is what the function's documentation states.

> > So make the function right.
> > 
> > Signed-off-by: Alex Shi <alexs@kernel.org>
> > 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 | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 96163ab69ae0..0a321f639c79 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -9741,8 +9741,8 @@ group_type group_classify(unsigned int imbalance_pct,
> >   */
> >  static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
> >  {
> > -	return (!sched_smt_active()) ||
> > -		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
> > +	return	(sd->flags & SD_SHARE_CPUCAPACITY) ||
> > +		(!sched_smt_active() && is_core_idle(cpu));
> >  }
> 
> This seems wrong. This would always return false for higher than SMT domains 
> if smt is active. 

Agreed.

> 
> Was this meant to be sched_smt_active() && is_core_idle(cpu)? 

But this would not work if SMT is inactive, in such case checking
for a whole idle core is pointless.


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

* Re: [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario
  2024-01-25  9:35     ` kuiliang Shi
@ 2024-01-26  0:06       ` Ricardo Neri
  2024-01-30 13:18         ` kuiliang Shi
  0 siblings, 1 reply; 13+ messages in thread
From: Ricardo Neri @ 2024-01-26  0:06 UTC (permalink / raw)
  To: kuiliang Shi
  Cc: Shrikanth Hegde, alexs, Ingo Molnar, Peter Zijlstra, Juri Lelli,
	Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
	Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider,
	linux-kernel

On Thu, Jan 25, 2024 at 05:35:32PM +0800, kuiliang Shi wrote:
> 
> 
> On 1/23/24 4:47 PM, Shrikanth Hegde wrote:
> > 
> > 
> > On 1/17/24 2:27 PM, alexs@kernel.org wrote:
> >> From: Alex Shi <alexs@kernel.org>
> >>
> >> Current function doesn't match it's comments, in fact, core_idle
> >> checking is only meaningful with non-SMT.
> >> So make the function right.
> >>
> >> Signed-off-by: Alex Shi <alexs@kernel.org>
> >> 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 | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 96163ab69ae0..0a321f639c79 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -9741,8 +9741,8 @@ group_type group_classify(unsigned int imbalance_pct,
> >>   */
> >>  static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
> >>  {
> >> -	return (!sched_smt_active()) ||
> >> -		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
> >> +	return	(sd->flags & SD_SHARE_CPUCAPACITY) ||
> >> +		(!sched_smt_active() && is_core_idle(cpu));
> >>  }
> > 
> > This seems wrong. This would always return false for higher than SMT domains 
> > if smt is active. 
> > 
> 
> yes, thanks for point out.
> 
> > Was this meant to be sched_smt_active() && is_core_idle(cpu)? 
> 
> In theory, yes, it should like this. But I have no ASYM device to test. :(

This would not work with !SMT and asym_packing.

I can test your patches on asym_packing + SMT systems if you post a new
version.


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

* Re: [PATCH 2/5] sched/fair: remove unused parameters
  2024-01-17  8:57 ` [PATCH 2/5] sched/fair: remove unused parameters alexs
@ 2024-01-26  0:18   ` Ricardo Neri
  0 siblings, 0 replies; 13+ messages in thread
From: Ricardo Neri @ 2024-01-26  0:18 UTC (permalink / raw)
  To: alexs
  Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel

On Wed, Jan 17, 2024 at 04:57:12PM +0800, 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>
> 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>

FWIW, Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
 

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

* Re: [PATCH 4/5] sched/fair: add a func _sched_asym
  2024-01-25 21:56   ` Ricardo Neri
@ 2024-01-26  1:57     ` kuiliang Shi
  0 siblings, 0 replies; 13+ messages in thread
From: kuiliang Shi @ 2024-01-26  1:57 UTC (permalink / raw)
  To: Ricardo Neri, alexs
  Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel



On 1/26/24 5:56 AM, Ricardo Neri wrote:
> On Wed, Jan 17, 2024 at 04:57:14PM +0800, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> Use this func in sched_asym and other path to simply code.
>> No function change.
>>
>> Signed-off-by: Alex Shi <alexs@kernel.org>
>> 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 | 27 +++++++++++++--------------
>>  1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index ebd659af2d78..96163ab69ae0 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -9745,6 +9745,14 @@ 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 repl_cpu)
> 
> What does repl_cpu mean? Maybe renaming to src_cpu?

Uh, src_cpu is better than a 'replacing' cpu. Thanks!

> 
>> +{
>> +	/* Ensure that the whole local core is idle, if applicable. */
>> +	return sched_use_asym_prio(sd, dst_cpu) &&
>> +			sched_asym_prefer(dst_cpu, repl_cpu);
> 
> The comment no longer applies to the whole expression. Perhaps rewording is
> in order. First we check for the whole idle core if applicable (i.e., when
> not balancing among SMT siblings). Then we check priorities.

Right will fix this by v2.
> 
> Also, indentation should be aligned with `return`, IMO.
> 
>> +}
>> +
>>  /**
>>   * sched_asym - Check if the destination CPU can do asym_packing load balance
>>   * @env:	The load balancing environment
>> @@ -9768,20 +9776,13 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>>  static inline bool
>>  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))
>> -		return false;
>> -
>>  	/*
>>  	 * CPU priorities does not make sense for SMT cores with more than one
>>  	 * busy sibling.
> 
> While here, it might be good to fix a syntax error above: s/does/do/.
> 

Yes, thanks

>>  	 */
>> -	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);
> 
> Perhaps we can come up with a better name than _sched_asym(). After this
> patch the difference between sched_asym() and _sched_asym() is that the
> former considers the stats of the source group. Maybe sched_asym() can be
> renamed as sched_group_asym(); it is only used in update_sg_lb_stats().
> Your _sched_asym() can become sched_asym().

Thanks for good suggestion! will send v2 according your suggestion. 

Alex
> 
>>  }
>>  
>>  /* One group has more than one SMT CPU while the other group does not */
>> @@ -11036,8 +11037,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 +11907,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
>>

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

* Re: [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario
  2024-01-26  0:06       ` Ricardo Neri
@ 2024-01-30 13:18         ` kuiliang Shi
  0 siblings, 0 replies; 13+ messages in thread
From: kuiliang Shi @ 2024-01-30 13:18 UTC (permalink / raw)
  To: Ricardo Neri
  Cc: Shrikanth Hegde, alexs, Ingo Molnar, Peter Zijlstra, Juri Lelli,
	Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
	Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider,
	linux-kernel



On 1/26/24 8:06 AM, Ricardo Neri wrote:
> On Thu, Jan 25, 2024 at 05:35:32PM +0800, kuiliang Shi wrote:
>>
>>
>> On 1/23/24 4:47 PM, Shrikanth Hegde wrote:
>>>
>>>
>>> On 1/17/24 2:27 PM, alexs@kernel.org wrote:
>>>> From: Alex Shi <alexs@kernel.org>
>>>>
>>>> Current function doesn't match it's comments, in fact, core_idle
>>>> checking is only meaningful with non-SMT.
>>>> So make the function right.
>>>>
>>>> Signed-off-by: Alex Shi <alexs@kernel.org>
>>>> 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 | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>>> index 96163ab69ae0..0a321f639c79 100644
>>>> --- a/kernel/sched/fair.c
>>>> +++ b/kernel/sched/fair.c
>>>> @@ -9741,8 +9741,8 @@ group_type group_classify(unsigned int imbalance_pct,
>>>>   */
>>>>  static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
>>>>  {
>>>> -	return (!sched_smt_active()) ||
>>>> -		(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
>>>> +	return	(sd->flags & SD_SHARE_CPUCAPACITY) ||
>>>> +		(!sched_smt_active() && is_core_idle(cpu));
>>>>  }
>>>
>>> This seems wrong. This would always return false for higher than SMT domains 
>>> if smt is active. 
>>>
>>
>> yes, thanks for point out.
>>
>>> Was this meant to be sched_smt_active() && is_core_idle(cpu)? 
>>
>> In theory, yes, it should like this. But I have no ASYM device to test. :(
> 
> This would not work with !SMT and asym_packing.
> 
> I can test your patches on asym_packing + SMT systems if you post a new
> version.
> 

Hi Neri,

Thanks a lot for generous offer! I don't know if my understanding right, but I try my best to have a best guessing in V2 patch for you. :)

Many thanks for the help!

Best regards
Alex 

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

end of thread, other threads:[~2024-01-30 13:18 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-17  8:57 [PATCH 1/5] sched/fair: add SD_CLUSTER in comments alexs
2024-01-17  8:57 ` [PATCH 2/5] sched/fair: remove unused parameters alexs
2024-01-26  0:18   ` Ricardo Neri
2024-01-17  8:57 ` [PATCH 3/5] sched/fair: cleanup sched_use_asym_prio alexs
2024-01-17  8:57 ` [PATCH 4/5] sched/fair: add a func _sched_asym alexs
2024-01-25 21:56   ` Ricardo Neri
2024-01-26  1:57     ` kuiliang Shi
2024-01-17  8:57 ` [PATCH 5/5] sched/fair: narrow the sched_use_asym_prio checking scenario alexs
2024-01-23  8:47   ` Shrikanth Hegde
2024-01-25  9:35     ` kuiliang Shi
2024-01-26  0:06       ` Ricardo Neri
2024-01-30 13:18         ` kuiliang Shi
2024-01-26  0:04     ` Ricardo Neri

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.