From: Alex Shi <[email protected]>
Packing the func sched_use_asym_prio/sched_asym_prefer into one, Using
new func to simply code. No function change.
Thanks Ricardo Neri for func rename and comments suggestion!
Signed-off-by: Alex Shi <[email protected]>
To: Ricardo Neri <[email protected]>
To: Valentin Schneider <[email protected]>
To: Vincent Guittot <[email protected]>
To: Peter Zijlstra <[email protected]>
To: Ingo Molnar <[email protected]>
---
kernel/sched/fair.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index ebd659af2d78..835dbe77b260 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -9745,8 +9745,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
(sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
}
+static inline bool sched_asym(struct sched_domain *sd, int dst_cpu, int src_cpu)
+{
+ /* Check if asym balance applicable, then check priorities.*/
+ return sched_use_asym_prio(sd, dst_cpu) &&
+ sched_asym_prefer(dst_cpu, src_cpu);
+}
+
/**
- * sched_asym - Check if the destination CPU can do asym_packing load balance
+ * sched_group_asym - Check if the destination CPU can do asym_packing balance
* @env: The load balancing environment
* @sgs: Load-balancing statistics of the candidate busiest group
* @group: The candidate busiest group
@@ -9766,22 +9773,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
* otherwise.
*/
static inline bool
-sched_asym(struct lb_env *env, struct sg_lb_stats *sgs, struct sched_group *group)
+sched_group_asym(struct lb_env *env, struct sg_lb_stats *sgs, struct sched_group *group)
{
- /* Ensure that the whole local core is idle, if applicable. */
- if (!sched_use_asym_prio(env->sd, env->dst_cpu))
- return false;
-
/*
- * CPU priorities does not make sense for SMT cores with more than one
+ * CPU priorities do not make sense for SMT cores with more than one
* busy sibling.
*/
- if (group->flags & SD_SHARE_CPUCAPACITY) {
- if (sgs->group_weight - sgs->idle_cpus != 1)
- return false;
- }
-
- return sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu);
+ return !(group->flags & SD_SHARE_CPUCAPACITY &&
+ sgs->group_weight - sgs->idle_cpus != 1) &&
+ sched_asym(env->sd, env->dst_cpu, group->asym_prefer_cpu);
}
/* One group has more than one SMT CPU while the other group does not */
@@ -9937,7 +9937,7 @@ static inline void update_sg_lb_stats(struct lb_env *env,
/* Check if dst CPU is idle and preferred to this group */
if (!local_group && env->sd->flags & SD_ASYM_PACKING &&
env->idle != CPU_NOT_IDLE && sgs->sum_h_nr_running &&
- sched_asym(env, sgs, group)) {
+ sched_group_asym(env, sgs, group)) {
sgs->group_asym_packing = 1;
}
@@ -11036,8 +11036,7 @@ static struct rq *find_busiest_queue(struct lb_env *env,
* SMT cores with more than one busy sibling.
*/
if ((env->sd->flags & SD_ASYM_PACKING) &&
- sched_use_asym_prio(env->sd, i) &&
- sched_asym_prefer(i, env->dst_cpu) &&
+ sched_asym(env->sd, i, env->dst_cpu) &&
nr_running == 1)
continue;
@@ -11907,8 +11906,7 @@ static void nohz_balancer_kick(struct rq *rq)
* preferred CPU must be idle.
*/
for_each_cpu_and(i, sched_domain_span(sd), nohz.idle_cpus_mask) {
- if (sched_use_asym_prio(sd, i) &&
- sched_asym_prefer(i, cpu)) {
+ if (sched_asym(sd, i, cpu)) {
flags = NOHZ_STATS_KICK | NOHZ_BALANCE_KICK;
goto unlock;
}
--
2.43.0
On Tue, Jan 30, 2024 at 09:17:06PM +0800, [email protected] wrote:
> From: Alex Shi <[email protected]>
>
> Packing the func sched_use_asym_prio/sched_asym_prefer into one, Using
> new func to simply code. No function change.
You should use imperative mood when writing changelogs. Also, you should
avoid abbreviations. When referring to functions,
please use (). In this specific instance, you could probably say:
"Consolidate the functions sched_use_asym_prio() and
sched_asym_prefer() into one. This makes the code easier to
read. No functional changes."
>
> Thanks Ricardo Neri for func rename and comments suggestion!
Thanks for the mention! But no need. :)
Perhaps you could explain the reasons for renaming the functions as you
did. Explaining why you make changes is an essential part of the
changelog.
>
> Signed-off-by: Alex Shi <[email protected]>
> To: Ricardo Neri <[email protected]>
> To: Valentin Schneider <[email protected]>
> To: Vincent Guittot <[email protected]>
> To: Peter Zijlstra <[email protected]>
> To: Ingo Molnar <[email protected]>
> ---
> kernel/sched/fair.c | 34 ++++++++++++++++------------------
> 1 file changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index ebd659af2d78..835dbe77b260 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9745,8 +9745,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
> (sd->flags & SD_SHARE_CPUCAPACITY) || is_core_idle(cpu);
> }
>
> +static inline bool sched_asym(struct sched_domain *sd, int dst_cpu, int src_cpu)
> +{
> + /* Check if asym balance applicable, then check priorities.*/
> + return sched_use_asym_prio(sd, dst_cpu) &&
> + sched_asym_prefer(dst_cpu, src_cpu);
> +}
> +
> /**
> - * sched_asym - Check if the destination CPU can do asym_packing load balance
> + * sched_group_asym - Check if the destination CPU can do asym_packing balance
> * @env: The load balancing environment
> * @sgs: Load-balancing statistics of the candidate busiest group
> * @group: The candidate busiest group
> @@ -9766,22 +9773,15 @@ static bool sched_use_asym_prio(struct sched_domain *sd, int cpu)
> * otherwise.
> */
> static inline bool
> -sched_asym(struct lb_env *env, struct sg_lb_stats *sgs, struct sched_group *group)
> +sched_group_asym(struct lb_env *env, struct sg_lb_stats *sgs, struct sched_group *group)
> {
> - /* Ensure that the whole local core is idle, if applicable. */
> - if (!sched_use_asym_prio(env->sd, env->dst_cpu))
> - return false;
> -
> /*
> - * CPU priorities does not make sense for SMT cores with more than one
> + * CPU priorities do not make sense for SMT cores with more than one
> * busy sibling.
> */
> - if (group->flags & SD_SHARE_CPUCAPACITY) {
> - if (sgs->group_weight - sgs->idle_cpus != 1)
> - return false;
> - }
> -
> - return sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu);
> + return !(group->flags & SD_SHARE_CPUCAPACITY &&
> + sgs->group_weight - sgs->idle_cpus != 1) &&
> + sched_asym(env->sd, env->dst_cpu, group->asym_prefer_cpu);
I am having second thoughts about compressing these conditions into a
single line. For instance, the comment above only applies to the first
condition and it is clear how the condition is met.
Checking the conditions separately makes the funcion easier to read, IMO.