Messages in this thread | | | Date | Tue, 27 Jun 2023 16:19:14 +0100 | From | Ionela Voinescu <> | Subject | Re: [PATCH v4 07/24] sched/fair: Compute IPC class scores for load balancing |
| |
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@google.com> > > > Cc: Daniel Bristot de Oliveira <bristot@redhat.com> > > > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > > > Cc: Ionela Voinescu <ionela.voinescu@arm.com> > > > Cc: Joel Fernandes (Google) <joel@joelfernandes.org> > > > Cc: Len Brown <len.brown@intel.com> > > > Cc: Lukasz Luba <lukasz.luba@arm.com> > > > Cc: Mel Gorman <mgorman@suse.de> > > > Cc: Perry Yuan <Perry.Yuan@amd.com> > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > > Cc: Steven Rostedt <rostedt@goodmis.org> > > > Cc: Tim C. Chen <tim.c.chen@intel.com> > > > Cc: Valentin Schneider <vschneid@redhat.com> > > > Cc: Zhao Liu <zhao1.liu@linux.intel.com> > > > Cc: x86@kernel.org > > > Cc: linux-pm@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org > > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > > --- > > > 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
| |