Re: [PATCH v2 3/4] sched/fair: Consider SMT in ASYM_PACKING load balance

[Ricardo Neri]

On Mon, May 03, 2021 at 12:23:36PM +0200, Peter Zijlstra wrote:
> On Mon, May 03, 2021 at 12:02:49PM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 13, 2021 at 07:04:35PM -0700, Ricardo Neri wrote:
> > > @@ -8507,6 +8616,18 @@ static bool update_sd_pick_busiest(struct lb_env *env,
> > >  	if (!sgs->sum_h_nr_running)
> > >  		return false;
> > >  
> > > +	/*
> > > +	 * @sg may have been tentatively classified as group_asym_packing.
> > > +	 * Now that we have sufficient information about @sds.local, reassess
> > > +	 * if asym packing migration can be done. Reclassify @sg. The only
> > > +	 * possible results are group_has_spare and group_fully_busy.
> > > +	 */
> > > +	if (sgs->group_type == group_asym_packing &&
> > > +	    !asym_can_pull_tasks(env->dst_cpu, sds, sgs, sg)) {
> > > +		sgs->group_asym_packing = 0;
> > > +		sgs->group_type = group_classify(env->sd->imbalance_pct, sg, sgs);
> > > +	}
> > 
> > So if this really is all about not having sds.local in
> > update_sd_lb_stats(), then that seems fixable. Let me haz a try.
> 
> How's this then?
> 
> ---
>  kernel/sched/fair.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 3bdc41f22909..e9dcbee5b3d9 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8437,6 +8437,21 @@ group_type group_classify(unsigned int imbalance_pct,
>  	return group_has_spare;
>  }
>  
> +static inline bool
> +sched_asym(struct lb_env *env, struct sd_lb_stats *sds, struct sched_group *group)

Thank you Peter for this code! It worked well. I had to make a couple of
tweaks. My proposed asym_can_pull_tasks() also needs the statistics of
@group. Thus, I added them to the arguments of sched_asym(). Also...

> +{
> +	/*
> +	 * Because sd->groups starts with the local group, anything that isn't
> +	 * the local group will have access to the local state.
> +	 */
> +	if (group == sds->local)
> +		return false;
> +
> +	/* XXX do magic here */
> +
> +	return sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu);
> +}
> +
>  /**
>   * update_sg_lb_stats - Update sched_group's statistics for load balancing.
>   * @env: The load balancing environment.
> @@ -8445,6 +8460,7 @@ group_type group_classify(unsigned int imbalance_pct,
>   * @sg_status: Holds flag indicating the status of the sched_group
>   */
>  static inline void update_sg_lb_stats(struct lb_env *env,
> +				      struct sd_lb_stats *sds,
>  				      struct sched_group *group,
>  				      struct sg_lb_stats *sgs,
>  				      int *sg_status)
> @@ -8453,7 +8469,7 @@ static inline void update_sg_lb_stats(struct lb_env *env,
>  
>  	memset(sgs, 0, sizeof(*sgs));
>  
> -	local_group = cpumask_test_cpu(env->dst_cpu, sched_group_span(group));
> +	local_group = group == sds->local;
>  
>  	for_each_cpu_and(i, sched_group_span(group), env->cpus) {
>  		struct rq *rq = cpu_rq(i);
> @@ -8498,9 +8514,8 @@ static inline void update_sg_lb_stats(struct lb_env *env,
>  
>  	/* Check if dst CPU is idle and preferred to this group */
>  	if (env->sd->flags & SD_ASYM_PACKING &&
> -	    env->idle != CPU_NOT_IDLE &&
> -	    sgs->sum_h_nr_running &&
> -	    sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu)) {
> +	    env->idle != CPU_NOT_IDLE && sgs->sum_h_nr_running &&
> +	    sched_asym(env, sds, group)) {
>  		sgs->group_asym_packing = 1;

... I moved this code to be executed after computing sgs->weight as
asym_can_pull_tasks() needs this datum as well.

May I add your Co-developed-by and Signed-off-by tags to a patch with these
changes in my v3 posting?

BR,
Ricardo