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

[Ricardo Neri]

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

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

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

Thanks for the mention! But no need. :)

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

> 
> Signed-off-by: Alex Shi <alexs@xxxxxxxxxx>
> To: Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx>
> To: Valentin Schneider <vschneid@xxxxxxxxxx>
> To: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> To: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> To: Ingo Molnar <mingo@xxxxxxxxxx>
> ---
>  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.