[PATCH v4 1/4] sched/topology: Remove duplicate descriptions from TOPOLOGY_SD_FLAGS

[PATCH v4 1/4] sched/topology: Remove duplicate descriptions from TOPOLOGY_SD_FLAGS

[alexs]

From: Alex Shi <alexs@kernel.org>

These flags are already documented in include/linux/sched/sd_flags.h.
Also add missing SD_CLUSTER and keep the comment on SD_ASYM_PACKING
as it is a special case.

Suggested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 10d1391e7416..0b33f7b05d21 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1551,11 +1551,12 @@ 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:
+ * function. For details, see include/linux/sched/sd_flags.h.
  *
- *   SD_SHARE_CPUCAPACITY   - describes SMT topologies
- *   SD_SHARE_PKG_RESOURCES - describes shared caches
- *   SD_NUMA                - describes NUMA topologies
+ *   SD_SHARE_CPUCAPACITY
+ *   SD_SHARE_PKG_RESOURCES
+ *   SD_CLUSTER
+ *   SD_NUMA
  *
  * Odd one out, which beside describing the topology has a quirk also
  * prescribes the desired behaviour that goes along with it:
-- 
2.43.0

[PATCH v4 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.

Fixes: c9ca07886aaa ("sched/fair: Do not even the number of busy CPUs via asym_packing")
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 533547e3c90a..607dc310b355 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9749,7 +9749,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
  *
@@ -9768,8 +9767,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))
@@ -9940,7 +9938,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 v4 3/4] sched/fair: Rework sched_use_asym_prio() and sched_asym_prefer()

[alexs]

From: Alex Shi <alexs@kernel.org>

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().
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 | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 607dc310b355..942b6358f683 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9746,8 +9746,18 @@ 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)
+{
+	/*
+	 * 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
@@ -9767,22 +9777,17 @@ 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;
-	}
+	if ((group->flags & SD_SHARE_CPUCAPACITY) &&
+	    (sgs->group_weight - sgs->idle_cpus != 1))
+		return false;
 
-	return sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu);
+	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 */
@@ -9938,7 +9943,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;
 	}
 
@@ -11037,8 +11042,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;
 
@@ -11908,8 +11912,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 v4 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: linux-kernel@vger.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 | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 942b6358f683..10ae28e1c088 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9740,6 +9740,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;
 
@@ -9941,11 +9944,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))
@@ -11041,9 +11042,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) {
@@ -11139,8 +11138,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) &&
-	       sched_use_asym_prio(env->sd, env->dst_cpu) &&
+	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 v4 5/5] sched: rename SD_SHARE_PKG_RESOURCES to SD_SHARE_LLC

[alexs]

From: Alex Shi <alexs@kernel.org>

SD_CLUSTER shares the CPU resources like llc tags or l2 cache, that's
easy confuse with SD_SHARE_PKG_RESOURCES. So let's specifical point
what the latter shares: LLC. That would reduce some confusing.

Suggested-by: Valentin Schneider <vschneid@redhat.com>
Signed-off-by: Alex Shi <alexs@kernel.org>
To: linux-kernel@vger.kernel.org
To: linuxppc-dev@lists.ozlabs.org
To: Miaohe Lin <linmiaohe@huawei.com>
To: Barry Song <song.bao.hua@hisilicon.com>
To: Mark Rutland <mark.rutland@arm.com>
To: Frederic Weisbecker <frederic@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: Juri Lelli <juri.lelli@redhat.com>
To: Ingo Molnar <mingo@redhat.com>
To: "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>
To: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
To: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: "Gautham R. Shenoy" <gautham.shenoy@amd.com>
Cc: Yicong Yang <yangyicong@hisilicon.com>
Cc: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Valentin Schneider <vschneid@redhat.com>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/smp.c      |  6 +++---
 include/linux/sched/sd_flags.h |  4 ++--
 include/linux/sched/topology.h |  6 +++---
 kernel/sched/fair.c            |  2 +-
 kernel/sched/topology.c        | 16 ++++++++--------
 5 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 693334c20d07..a60e4139214b 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -984,7 +984,7 @@ static bool shared_caches __ro_after_init;
 /* cpumask of CPUs with asymmetric SMT dependency */
 static int powerpc_smt_flags(void)
 {
-	int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
+	int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_LLC;
 
 	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
 		printk_once(KERN_INFO "Enabling Asymmetric SMT scheduling\n");
@@ -1010,9 +1010,9 @@ static __ro_after_init DEFINE_STATIC_KEY_FALSE(splpar_asym_pack);
 static int powerpc_shared_cache_flags(void)
 {
 	if (static_branch_unlikely(&splpar_asym_pack))
-		return SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING;
+		return SD_SHARE_LLC | SD_ASYM_PACKING;
 
-	return SD_SHARE_PKG_RESOURCES;
+	return SD_SHARE_LLC;
 }
 
 static int powerpc_shared_proc_flags(void)
diff --git a/include/linux/sched/sd_flags.h b/include/linux/sched/sd_flags.h
index a8b28647aafc..b04a5d04dee9 100644
--- a/include/linux/sched/sd_flags.h
+++ b/include/linux/sched/sd_flags.h
@@ -117,13 +117,13 @@ SD_FLAG(SD_SHARE_CPUCAPACITY, SDF_SHARED_CHILD | SDF_NEEDS_GROUPS)
 SD_FLAG(SD_CLUSTER, SDF_NEEDS_GROUPS)
 
 /*
- * Domain members share CPU package resources (i.e. caches)
+ * Domain members share CPU Last Level Caches
  *
  * SHARED_CHILD: Set from the base domain up until spanned CPUs no longer share
  *               the same cache(s).
  * NEEDS_GROUPS: Caches are shared between groups.
  */
-SD_FLAG(SD_SHARE_PKG_RESOURCES, SDF_SHARED_CHILD | SDF_NEEDS_GROUPS)
+SD_FLAG(SD_SHARE_LLC, SDF_SHARED_CHILD | SDF_NEEDS_GROUPS)
 
 /*
  * Only a single load balancing instance
diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
index a6e04b4a21d7..191b122158fb 100644
--- a/include/linux/sched/topology.h
+++ b/include/linux/sched/topology.h
@@ -38,21 +38,21 @@ extern const struct sd_flag_debug sd_flag_debug[];
 #ifdef CONFIG_SCHED_SMT
 static inline int cpu_smt_flags(void)
 {
-	return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
+	return SD_SHARE_CPUCAPACITY | SD_SHARE_LLC;
 }
 #endif
 
 #ifdef CONFIG_SCHED_CLUSTER
 static inline int cpu_cluster_flags(void)
 {
-	return SD_CLUSTER | SD_SHARE_PKG_RESOURCES;
+	return SD_CLUSTER | SD_SHARE_LLC;
 }
 #endif
 
 #ifdef CONFIG_SCHED_MC
 static inline int cpu_core_flags(void)
 {
-	return SD_SHARE_PKG_RESOURCES;
+	return SD_SHARE_LLC;
 }
 #endif
 
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 10ae28e1c088..188597640b1f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -10695,7 +10695,7 @@ static inline void calculate_imbalance(struct lb_env *env, struct sd_lb_stats *s
 	 */
 	if (local->group_type == group_has_spare) {
 		if ((busiest->group_type > group_fully_busy) &&
-		    !(env->sd->flags & SD_SHARE_PKG_RESOURCES)) {
+		    !(env->sd->flags & SD_SHARE_LLC)) {
 			/*
 			 * If busiest is overloaded, try to fill spare
 			 * capacity. This might end up creating spare capacity
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 0b33f7b05d21..e877730219d3 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -684,7 +684,7 @@ static void update_top_cache_domain(int cpu)
 	int id = cpu;
 	int size = 1;
 
-	sd = highest_flag_domain(cpu, SD_SHARE_PKG_RESOURCES);
+	sd = highest_flag_domain(cpu, SD_SHARE_LLC);
 	if (sd) {
 		id = cpumask_first(sched_domain_span(sd));
 		size = cpumask_weight(sched_domain_span(sd));
@@ -1554,7 +1554,7 @@ static struct cpumask		***sched_domains_numa_masks;
  * function. For details, see include/linux/sched/sd_flags.h.
  *
  *   SD_SHARE_CPUCAPACITY
- *   SD_SHARE_PKG_RESOURCES
+ *   SD_SHARE_LLC
  *   SD_CLUSTER
  *   SD_NUMA
  *
@@ -1566,7 +1566,7 @@ static struct cpumask		***sched_domains_numa_masks;
 #define TOPOLOGY_SD_FLAGS		\
 	(SD_SHARE_CPUCAPACITY	|	\
 	 SD_CLUSTER		|	\
-	 SD_SHARE_PKG_RESOURCES |	\
+	 SD_SHARE_LLC		|	\
 	 SD_NUMA		|	\
 	 SD_ASYM_PACKING)
 
@@ -1609,7 +1609,7 @@ sd_init(struct sched_domain_topology_level *tl,
 					| 0*SD_BALANCE_WAKE
 					| 1*SD_WAKE_AFFINE
 					| 0*SD_SHARE_CPUCAPACITY
-					| 0*SD_SHARE_PKG_RESOURCES
+					| 0*SD_SHARE_LLC
 					| 0*SD_SERIALIZE
 					| 1*SD_PREFER_SIBLING
 					| 0*SD_NUMA
@@ -1646,7 +1646,7 @@ sd_init(struct sched_domain_topology_level *tl,
 	if (sd->flags & SD_SHARE_CPUCAPACITY) {
 		sd->imbalance_pct = 110;
 
-	} else if (sd->flags & SD_SHARE_PKG_RESOURCES) {
+	} else if (sd->flags & SD_SHARE_LLC) {
 		sd->imbalance_pct = 117;
 		sd->cache_nice_tries = 1;
 
@@ -1671,7 +1671,7 @@ sd_init(struct sched_domain_topology_level *tl,
 	 * For all levels sharing cache; connect a sched_domain_shared
 	 * instance.
 	 */
-	if (sd->flags & SD_SHARE_PKG_RESOURCES) {
+	if (sd->flags & SD_SHARE_LLC) {
 		sd->shared = *per_cpu_ptr(sdd->sds, sd_id);
 		atomic_inc(&sd->shared->ref);
 		atomic_set(&sd->shared->nr_busy_cpus, sd_weight);
@@ -2446,8 +2446,8 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att
 		for (sd = *per_cpu_ptr(d.sd, i); sd; sd = sd->parent) {
 			struct sched_domain *child = sd->child;
 
-			if (!(sd->flags & SD_SHARE_PKG_RESOURCES) && child &&
-			    (child->flags & SD_SHARE_PKG_RESOURCES)) {
+			if (!(sd->flags & SD_SHARE_LLC) && child &&
+			    (child->flags & SD_SHARE_LLC)) {
 				struct sched_domain __rcu *top_p;
 				unsigned int nr_llcs;
 
-- 
2.43.0

Re: [PATCH v4 1/4] sched/topology: Remove duplicate descriptions from TOPOLOGY_SD_FLAGS

[Ricardo Neri]

On Wed, Feb 07, 2024 at 11:47:01AM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> These flags are already documented in include/linux/sched/sd_flags.h.
> Also add missing SD_CLUSTER and keep the comment on SD_ASYM_PACKING

s/Also/Also,/

with that, FWIW,

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

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

[Ricardo Neri]

On Wed, Feb 07, 2024 at 11:47:02AM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>

A few nits:

subject: s/remove/Remove/

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

The argument sds is not used in function sched_asym(). Remove it.

> 
> Fixes: c9ca07886aaa ("sched/fair: Do not even the number of busy CPUs via asym_packing")
> Signed-off-by: Alex Shi <alexs@kernel.org>
> Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

With that, my tag stands, FWIW.

Re: [PATCH v4 1/4] sched/topology: Remove duplicate descriptions from TOPOLOGY_SD_FLAGS

[Valentin Schneider]

On 07/02/24 11:47, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
>
> These flags are already documented in include/linux/sched/sd_flags.h.
> Also add missing SD_CLUSTER and keep the comment on SD_ASYM_PACKING
> as it is a special case.
>
> Suggested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> Signed-off-by: Alex Shi <alexs@kernel.org>

Reviewed-by: Valentin Schneider <vschneid@redhat.com>

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

[Valentin Schneider]

On 07/02/24 11:47, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
>
> sds isn't used in function sched_asym(), so remove it to cleanup code.
>
> Fixes: c9ca07886aaa ("sched/fair: Do not even the number of busy CPUs via asym_packing")
> Signed-off-by: Alex Shi <alexs@kernel.org>
> Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Reviewed-by: Valentin Schneider <vschneid@redhat.com>

Re: [PATCH v4 5/5] sched: rename SD_SHARE_PKG_RESOURCES to SD_SHARE_LLC

[Valentin Schneider]

On 07/02/24 11:58, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
>
> SD_CLUSTER shares the CPU resources like llc tags or l2 cache, that's
> easy confuse with SD_SHARE_PKG_RESOURCES. So let's specifical point
> what the latter shares: LLC. That would reduce some confusing.
>
> Suggested-by: Valentin Schneider <vschneid@redhat.com>
> Signed-off-by: Alex Shi <alexs@kernel.org>

AFAICT it's just missing the below replacement (I've stretched the comments
to go up to 80 chars while at it), otherwise LGTM.

Reviewed-by: Valentin Schneider <vschneid@redhat.com>

---

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index e877730219d38..99ea5986038ce 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -657,13 +657,13 @@ static void destroy_sched_domains(struct sched_domain *sd)
 }
 
 /*
- * Keep a special pointer to the highest sched_domain that has
- * SD_SHARE_PKG_RESOURCE set (Last Level Cache Domain) for this
- * allows us to avoid some pointer chasing select_idle_sibling().
+ * Keep a special pointer to the highest sched_domain that has SD_SHARE_LLC set
+ * (Last Level Cache Domain) for this allows us to avoid some pointer chasing
+ * select_idle_sibling().
  *
- * Also keep a unique ID per domain (we use the first CPU number in
- * the cpumask of the domain), this allows us to quickly tell if
- * two CPUs are in the same cache domain, see cpus_share_cache().
+ * Also keep a unique ID per domain (we use the first CPU number in the cpumask
+ * of the domain), this allows us to quickly tell if two CPUs are in the same
+ * cache domain, see cpus_share_cache().
  */
 DEFINE_PER_CPU(struct sched_domain __rcu *, sd_llc);
 DEFINE_PER_CPU(int, sd_llc_size);

Re: [PATCH v4 3/4] sched/fair: Rework sched_use_asym_prio() and sched_asym_prefer()

[Ricardo Neri]

On Wed, Feb 07, 2024 at 11:47:03AM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> sched_use_asym_prio() sched_asym_prefer() are used together in various

s/prio() sched/prio() and sched/

> 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().
> 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 | 37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 607dc310b355..942b6358f683 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9746,8 +9746,18 @@ 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)
> +{
> +	/*
> +	 * 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

After renaming and changing this function now its documentation has become
obsolete. Can you update it?
 

Re: [PATCH v4 5/5] sched: rename SD_SHARE_PKG_RESOURCES to SD_SHARE_LLC

[Ricardo Neri]

On Wed, Feb 07, 2024 at 11:58:40AM +0800, alexs@kernel.org wrote:
> From: Alex Shi <alexs@kernel.org>
> 
> SD_CLUSTER shares the CPU resources like llc tags or l2 cache, that's
> easy confuse with SD_SHARE_PKG_RESOURCES. So let's specifical point
> what the latter shares: LLC. That would reduce some confusing.
> 
> Suggested-by: Valentin Schneider <vschneid@redhat.com>
> Signed-off-by: Alex Shi <alexs@kernel.org>

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

Re: [PATCH v4 3/4] sched/fair: Rework sched_use_asym_prio() and sched_asym_prefer()

[kuiliang Shi]

On 2/9/24 10:47 AM, Ricardo Neri wrote:
> On Wed, Feb 07, 2024 at 11:47:03AM +0800, alexs@kernel.org wrote:
>> From: Alex Shi <alexs@kernel.org>
>>
>> sched_use_asym_prio() sched_asym_prefer() are used together in various
> 
> s/prio() sched/prio() and sched/

Will take it, Thanks!

> 
>> 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().
>> 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 | 37 ++++++++++++++++++++-----------------
>>  1 file changed, 20 insertions(+), 17 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 607dc310b355..942b6358f683 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -9746,8 +9746,18 @@ 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)
>> +{
>> +	/*
>> +	 * 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
> 
> After renaming and changing this function now its documentation has become
> obsolete. Can you update it?

Since the function sched_use_asym_prio() and sched_asym_prefer() both give detailed comments for their actions, as long as this function does, could we just remove the bit obsolete comments here?


@@ -9765,14 +9765,6 @@ static inline bool sched_asym(struct sched_domain *sd, int dst_cpu, int src_cpu)
  * @env::dst_cpu can do asym_packing if it has higher priority than the
  * preferred CPU of @group.
  *
- * SMT is a special case. If we are balancing load between cores, @env::dst_cpu
- * can do asym_packing balance only if all its SMT siblings are idle. Also, it
- * can only do it if @group is an SMT group and has exactly on busy CPU. Larger
- * imbalances in the number of CPUS are dealt with in find_busiest_group().
- *
- * If we are balancing load within an SMT core, or at PKG domain level, always
- * proceed.
- *
  * Return: true if @env::dst_cpu can do with asym_packing load balance. False
  * otherwise.
>  

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

[kuiliang Shi]

Hi Valentin&Ricardo,

Any more comment for this patch? or Reviewed-by from you as a Chinese new year gift for this patch? :)

Thanks
Alex

On 2/7/24 11:47 AM, 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.
> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> To: linux-kernel@vger.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 | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 942b6358f683..10ae28e1c088 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9740,6 +9740,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;
>  
> @@ -9941,11 +9944,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))
> @@ -11041,9 +11042,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) {
> @@ -11139,8 +11138,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) &&
> -	       sched_use_asym_prio(env->sd, env->dst_cpu) &&
> +	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));
>  }

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

[Ricardo Neri]

On Fri, Feb 09, 2024 at 07:12:10PM +0800, kuiliang Shi wrote:
> Hi Valentin&Ricardo,
> 
> Any more comment for this patch? or Reviewed-by from you as a Chinese new year gift for this patch? :)

I will give you a Tested-by tag ;). I have started testing your patches but
I am not done yet.

> 
> Thanks
> Alex
> 
> On 2/7/24 11:47 AM, 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.
> > 
> > Signed-off-by: Alex Shi <alexs@kernel.org>
> > To: linux-kernel@vger.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 | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 942b6358f683..10ae28e1c088 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -9740,6 +9740,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;
> >  
> > @@ -9941,11 +9944,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))
> > @@ -11041,9 +11042,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) {
> > @@ -11139,8 +11138,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) &&
> > -	       sched_use_asym_prio(env->sd, env->dst_cpu) &&
> > +	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));
> >  }

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

[Ricardo Neri]

On Wed, Feb 07, 2024 at 11:47:04AM +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.

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

Tested on Alder Lake and Meteor Lake, which use asym_packing.

Tested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

> 
> Signed-off-by: Alex Shi <alexs@kernel.org>
> To: linux-kernel@vger.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 | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 942b6358f683..10ae28e1c088 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9740,6 +9740,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;
>  
> @@ -9941,11 +9944,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))
> @@ -11041,9 +11042,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) {
> @@ -11139,8 +11138,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) &&
> -	       sched_use_asym_prio(env->sd, env->dst_cpu) &&
> +	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 v4 3/4] sched/fair: Rework sched_use_asym_prio() and sched_asym_prefer()

[Ricardo Neri]

On Fri, Feb 09, 2024 at 07:08:52PM +0800, kuiliang Shi wrote:
> 
> 
> On 2/9/24 10:47 AM, Ricardo Neri wrote:
> > On Wed, Feb 07, 2024 at 11:47:03AM +0800, alexs@kernel.org wrote:
> >> From: Alex Shi <alexs@kernel.org>
> >>
> >> sched_use_asym_prio() sched_asym_prefer() are used together in various
> > 
> > s/prio() sched/prio() and sched/
> 
> Will take it, Thanks!
> 
> > 
> >> 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().
> >> 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 | 37 ++++++++++++++++++++-----------------
> >>  1 file changed, 20 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index 607dc310b355..942b6358f683 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -9746,8 +9746,18 @@ 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)
> >> +{
> >> +	/*
> >> +	 * 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
> > 
> > After renaming and changing this function now its documentation has become
> > obsolete. Can you update it?
> 
> Since the function sched_use_asym_prio() and sched_asym_prefer() both give detailed comments for their actions, as long as this function does, could we just remove the bit obsolete comments here?
> 
> 
> @@ -9765,14 +9765,6 @@ static inline bool sched_asym(struct sched_domain *sd, int dst_cpu, int src_cpu)
>   * @env::dst_cpu can do asym_packing if it has higher priority than the
>   * preferred CPU of @group.
>   *
> - * SMT is a special case. If we are balancing load between cores, @env::dst_cpu
> - * can do asym_packing balance only if all its SMT siblings are idle. Also, it
> - * can only do it if @group is an SMT group and has exactly on busy CPU. Larger
> - * imbalances in the number of CPUS are dealt with in find_busiest_group().
> - *
> - * If we are balancing load within an SMT core, or at PKG domain level, always
> - * proceed.
> - *
>   * Return: true if @env::dst_cpu can do with asym_packing load balance. False
>   * otherwise.

Fine with me.

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

Tested on Alder Lake and Meteor Lake, which use asym_packing.

Tested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> >  

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

[Alex Shi]

Ricardo Neri <ricardo.neri-calderon@linux.intel.com> 于2024年2月10日周六 09:11写道：
>
> On Wed, Feb 07, 2024 at 11:47:04AM +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.
>
> Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
>
> Tested on Alder Lake and Meteor Lake, which use asym_packing.
>
> Tested-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

It's the best gift for my lunar new year! :)
Next version with your Tested and Reviewed is coming.

Thanks a lot!
Alex
>
> >
> > Signed-off-by: Alex Shi <alexs@kernel.org>
> > To: linux-kernel@vger.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 | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 942b6358f683..10ae28e1c088 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -9740,6 +9740,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;
> >
> > @@ -9941,11 +9944,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))
> > @@ -11041,9 +11042,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) {
> > @@ -11139,8 +11138,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) &&
> > -            sched_use_asym_prio(env->sd, env->dst_cpu) &&
> > +     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
> >