Re: [PATCH v4 07/24] sched/fair: Compute IPC class scores for load balancing

[Ionela Voinescu]

Hey,

On Sunday 25 Jun 2023 at 13:11:55 (-0700), Ricardo Neri wrote:
> On Thu, Jun 22, 2023 at 10:02:44AM +0100, Ionela Voinescu wrote:
> > On Monday 12 Jun 2023 at 21:24:05 (-0700), Ricardo Neri wrote:
> > > When using IPCC scores to break ties between two scheduling groups, it is
> > > necessary to consider both the current score and the score that would
> > > result after load balancing.
> > > 
> > > Compute the combined IPC class score of a scheduling group and the local
> > > scheduling group. Compute both the current score and the prospective score.
> > > 
> > > Collect IPCC statistics only for asym_packing and fully_busy scheduling
> > > groups. These are the only cases that use IPCC scores.
> > > 
> > > These IPCC statistics are used during idle load balancing. The candidate
> > > scheduling group will have one fewer busy CPU after load balancing. This
> > > observation is important for cores with SMT support.
> > > 
> > > The IPCC score of scheduling groups composed of SMT siblings needs to
> > > consider that the siblings share CPU resources. When computing the total
> > > IPCC score of the scheduling group, divide the score of each sibling by
> > > the number of busy siblings.
> > > 
> > > Cc: Ben Segall <bsegall@xxxxxxxxxx>
> > > Cc: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
> > > Cc: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
> > > Cc: Ionela Voinescu <ionela.voinescu@xxxxxxx>
> > > Cc: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> > > Cc: Len Brown <len.brown@xxxxxxxxx>
> > > Cc: Lukasz Luba <lukasz.luba@xxxxxxx>
> > > Cc: Mel Gorman <mgorman@xxxxxxx>
> > > Cc: Perry Yuan <Perry.Yuan@xxxxxxx>
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > > Cc: Tim C. Chen <tim.c.chen@xxxxxxxxx>
> > > Cc: Valentin Schneider <vschneid@xxxxxxxxxx>
> > > Cc: Zhao Liu <zhao1.liu@xxxxxxxxxxxxxxx>
> > > Cc: x86@xxxxxxxxxx
> > > Cc: linux-pm@xxxxxxxxxxxxxxx
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@xxxxxxxxxxxxxxx>
> > > ---
> > > Changes since v3:
> > >  * None
> > > 
> > > Changes since v2:
> > >  * Also collect IPCC stats for fully_busy sched groups.
> > >  * Restrict use of IPCC stats to SD_ASYM_PACKING. (Ionela)
> > >  * Handle errors of arch_get_ipcc_score(). (Ionela)
> > > 
> > > Changes since v1:
> > >  * Implemented cleanups and reworks from PeterZ. I took all his
> > >    suggestions, except the computation of the  IPC score before and after
> > >    load balancing. We are computing not the average score, but the *total*.
> > >  * Check for the SD_SHARE_CPUCAPACITY to compute the throughput of the SMT
> > >    siblings of a physical core.
> > >  * Used the new interface names.
> > >  * Reworded commit message for clarity.
> > > ---
> > >  kernel/sched/fair.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 68 insertions(+)
> > > 
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index c0cab5e501b6..a51c65c9335f 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -9114,6 +9114,8 @@ struct sg_lb_stats {
> > >  	unsigned long min_score; /* Min(score(rq->curr->ipcc)) */
> > >  	unsigned short min_ipcc; /* Class of the task with the minimum IPCC score in the rq */
> > >  	unsigned long sum_score; /* Sum(score(rq->curr->ipcc)) */
> > > +	long ipcc_score_after; /* Prospective IPCC score after load balancing */
> > > +	unsigned long ipcc_score_before; /* IPCC score before load balancing */
> > >  #endif
> > >  };
> > >  
> > > @@ -9452,6 +9454,62 @@ static void update_sg_lb_ipcc_stats(int dst_cpu, struct sg_lb_stats *sgs,
> > >  	}
> > >  }
> > >  
> > > +static void update_sg_lb_stats_scores(struct sg_lb_stats *sgs,
> > > +				      struct sched_group *sg,
> > > +				      struct lb_env *env)
> > > +{
> > > +	unsigned long score_on_dst_cpu, before;
> > > +	int busy_cpus;
> > > +	long after;
> > > +
> > > +	if (!sched_ipcc_enabled())
> > > +		return;
> > > +
> > > +	/*
> > > +	 * IPCC scores are only useful during idle load balancing. For now,
> > > +	 * only asym_packing uses IPCC scores.
> > > +	 */
> > > +	if (!(env->sd->flags & SD_ASYM_PACKING) ||
> > > +	    env->idle == CPU_NOT_IDLE)
> > > +		return;
> > > +
> > > +	/*
> > > +	 * IPCC scores are used to break ties only between these types of
> > > +	 * groups.
> > > +	 */
> > > +	if (sgs->group_type != group_fully_busy &&
> > > +	    sgs->group_type != group_asym_packing)
> > > +		return;
> > > +
> > > +	busy_cpus = sgs->group_weight - sgs->idle_cpus;
> > > +
> > > +	/* No busy CPUs in the group. No tasks to move. */
> > > +	if (!busy_cpus)
> > > +		return;
> > > +
> > > +	score_on_dst_cpu = arch_get_ipcc_score(sgs->min_ipcc, env->dst_cpu);
> > > +
> > > +	/*
> > > +	 * Do not use IPC scores. sgs::ipcc_score_{after, before} will be zero
> > > +	 * and not used.
> > > +	 */
> > > +	if (IS_ERR_VALUE(score_on_dst_cpu))
> > > +		return;
> > > +
> > > +	before = sgs->sum_score;
> > > +	after = before - sgs->min_score;
> > 
> > I don't believe this can end up being negative as the sum of all
> > scores should be higher or equal to the min score, right?
> 
> Yes, I agree. `after` cannot be negative.
> 
> > 
> > I'm just wondering if ipcc_score_after can be made unsigned long as well,
> > just for consistency.
> 
> Sure. I can make it of type unsigned long as well.
> 
> > 
> > > +
> > > +	/* SMT siblings share throughput. */
> > > +	if (busy_cpus > 1 && sg->flags & SD_SHARE_CPUCAPACITY) {
> > > +		before /= busy_cpus;
> > > +		/* One sibling will become idle after load balance. */
> > > +		after /= busy_cpus - 1;
> > > +	}
> > > +
> > > +	sgs->ipcc_score_after = after + score_on_dst_cpu;
> > > +	sgs->ipcc_score_before = before;
> > 
> > Shouldn't the score_on_dst_cpu be added to "after" before being divided
> > between the SMT siblings?
> 
> No, because ipcc_score_after represents the joint score of the busiest
> core and the destination core after load balance has taken place. The
> destination core was previously idle and now contributes to the joint
> score.
> 

Right! score_on_dst_cpu does not contribute to the per-cpu throughput of
the busiest core, but it reflects the improvement in score gained by the
move to the destination.

Thanks,
Ionela.

> Thanks and BR,
> Ricardo