| Message ID | 20221128132100.30253-7-ricardo.neri-calderon@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | sched: Introduce IPC classes for load balance | expand |
On 28/11/2022 14:20, Ricardo Neri wrote: [...] > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 224107278471..3a1d6c50a19b 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -9100,6 +9100,57 @@ group_type group_classify(unsigned int imbalance_pct, > return group_has_spare; > } > > +struct sg_lb_ipcc_stats { > + int min_score; /* Min(score(rq->curr->ipcc)) */ > + int min_ipcc; /* Min(rq->curr->ipcc) */ > + long sum_score; /* Sum(score(rq->curr->ipcc)) */ > +}; Wouldn't it be cleaner to put `min_score`, `min_ipcc` and `sum_score` into `struct sg_lb_stats` next to `ipcc_score_{after, before}` under the same #ifdef CONFIG_IPC_CLASSES? Looks like those IPCC stats would only be needed in the specific condition under which update_sg_lb_stats_scores() is called? > +#ifdef CONFIG_IPC_CLASSES > +static void init_rq_ipcc_stats(struct sg_lb_ipcc_stats *sgcs) > +{ > + *sgcs = (struct sg_lb_ipcc_stats) { > + .min_score = INT_MAX, > + }; > +} > + > +/** Called only if cpu_of(@rq) is not idle and has tasks running. */ > +static void update_sg_lb_ipcc_stats(struct sg_lb_ipcc_stats *sgcs, > + struct rq *rq) > +{ > + struct task_struct *curr; > + unsigned short ipcc; > + int score; > + > + if (!sched_ipcc_enabled()) > + return; > + > + curr = rcu_dereference(rq->curr); > + if (!curr || (curr->flags & PF_EXITING) || is_idle_task(curr)) So the Idle task is excluded but RT, DL, (Stopper) tasks are not. Looks weird if non-CFS tasks could influence CFS load-balancing. AFAICS, RT and DL tasks could have p->ipcc != IPC_CLASS_UNCLASSIFIED? [...]
Hi, On Monday 28 Nov 2022 at 05:20:44 (-0800), Ricardo Neri wrote: [..] > > +struct sg_lb_ipcc_stats { > + int min_score; /* Min(score(rq->curr->ipcc)) */ > + int min_ipcc; /* Min(rq->curr->ipcc) */ Nit: this is not the minimum IPCC between the current tasks of all runqueues, but the IPCC specific to the task with the minimum score. Possibly there's not much to be done about the variable name, but the comment can be made more clear. Thanks, Ionela. > + long sum_score; /* Sum(score(rq->curr->ipcc)) */ > +}; [..]
On Wed, Dec 07, 2022 at 06:00:32PM +0100, Dietmar Eggemann wrote: > On 28/11/2022 14:20, Ricardo Neri wrote: > > [...] > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > index 224107278471..3a1d6c50a19b 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -9100,6 +9100,57 @@ group_type group_classify(unsigned int imbalance_pct, > > return group_has_spare; > > } > > > > +struct sg_lb_ipcc_stats { > > + int min_score; /* Min(score(rq->curr->ipcc)) */ > > + int min_ipcc; /* Min(rq->curr->ipcc) */ > > + long sum_score; /* Sum(score(rq->curr->ipcc)) */ > > +}; > > Wouldn't it be cleaner to put `min_score`, `min_ipcc` and `sum_score` > into `struct sg_lb_stats` next to `ipcc_score_{after, before}` under the > same #ifdef CONFIG_IPC_CLASSES? Yes, that is a good observation. I initially wanted to hide these intermediate and only expose the end result ipcc_score_{after, before} to struct sg_lb_stats. I agree, it would look cleaner as you suggest. > > Looks like those IPCC stats would only be needed in the specific > condition under which update_sg_lb_stats_scores() is called? True. > > > +#ifdef CONFIG_IPC_CLASSES > > +static void init_rq_ipcc_stats(struct sg_lb_ipcc_stats *sgcs) > > +{ > > + *sgcs = (struct sg_lb_ipcc_stats) { > > + .min_score = INT_MAX, > > + }; > > +} > > + > > +/** Called only if cpu_of(@rq) is not idle and has tasks running. */ > > +static void update_sg_lb_ipcc_stats(struct sg_lb_ipcc_stats *sgcs, > > + struct rq *rq) > > +{ > > + struct task_struct *curr; > > + unsigned short ipcc; > > + int score; > > + > > + if (!sched_ipcc_enabled()) > > + return; > > + > > + curr = rcu_dereference(rq->curr); > > + if (!curr || (curr->flags & PF_EXITING) || is_idle_task(curr)) > > So the Idle task is excluded but RT, DL, (Stopper) tasks are not. Looks > weird if non-CFS tasks could influence CFS load-balancing. > AFAICS, RT and DL tasks could have p->ipcc != IPC_CLASS_UNCLASSIFIED? Agreed. Perhaps I can also check for !(task_is_realtime()), which see seems to cover all these cases. > > [...]
On Thu, Dec 08, 2022 at 08:50:23AM +0000, Ionela Voinescu wrote: > Hi, > > On Monday 28 Nov 2022 at 05:20:44 (-0800), Ricardo Neri wrote: > [..] > > > > +struct sg_lb_ipcc_stats { > > + int min_score; /* Min(score(rq->curr->ipcc)) */ > > + int min_ipcc; /* Min(rq->curr->ipcc) */ > > Nit: this is not the minimum IPCC between the current tasks of all > runqueues, but the IPCC specific to the task with the minimum score. > Possibly there's not much to be done about the variable name, but the > comment can be made more clear. Very true. I will make the change. Thanks and BR, Ricardo
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 224107278471..3a1d6c50a19b 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -9100,6 +9100,57 @@ group_type group_classify(unsigned int imbalance_pct, return group_has_spare; } +struct sg_lb_ipcc_stats { + int min_score; /* Min(score(rq->curr->ipcc)) */ + int min_ipcc; /* Min(rq->curr->ipcc) */ + long sum_score; /* Sum(score(rq->curr->ipcc)) */ +}; + +#ifdef CONFIG_IPC_CLASSES +static void init_rq_ipcc_stats(struct sg_lb_ipcc_stats *sgcs) +{ + *sgcs = (struct sg_lb_ipcc_stats) { + .min_score = INT_MAX, + }; +} + +/** Called only if cpu_of(@rq) is not idle and has tasks running. */ +static void update_sg_lb_ipcc_stats(struct sg_lb_ipcc_stats *sgcs, + struct rq *rq) +{ + struct task_struct *curr; + unsigned short ipcc; + int score; + + if (!sched_ipcc_enabled()) + return; + + curr = rcu_dereference(rq->curr); + if (!curr || (curr->flags & PF_EXITING) || is_idle_task(curr)) + return; + + ipcc = curr->ipcc; + score = arch_get_ipcc_score(ipcc, cpu_of(rq)); + + sgcs->sum_score += score; + + if (score < sgcs->min_score) { + sgcs->min_score = score; + sgcs->min_ipcc = ipcc; + } +} + +#else /* CONFIG_IPC_CLASSES */ +static void update_sg_lb_ipcc_stats(struct sg_lb_ipcc_stats *sgcs, + struct rq *rq) +{ +} + +static void init_rq_ipcc_stats(struct sg_lb_ipcc_stats *class_sgs) +{ +} +#endif /* CONFIG_IPC_CLASSES */ + /** * asym_smt_can_pull_tasks - Check whether the load balancing CPU can pull tasks * @dst_cpu: Destination CPU of the load balancing @@ -9212,9 +9263,11 @@ static inline void update_sg_lb_stats(struct lb_env *env, struct sg_lb_stats *sgs, int *sg_status) { + struct sg_lb_ipcc_stats sgcs; int i, nr_running, local_group; memset(sgs, 0, sizeof(*sgs)); + init_rq_ipcc_stats(&sgcs); local_group = group == sds->local; @@ -9264,6 +9317,8 @@ static inline void update_sg_lb_stats(struct lb_env *env, if (sgs->group_misfit_task_load < load) sgs->group_misfit_task_load = load; } + + update_sg_lb_ipcc_stats(&sgcs, rq); } sgs->group_capacity = group->sgc->capacity;
When selecting a busiest scheduling group, the IPC class of the current task can be used to select between two scheduling groups of equal asym_packing priority and number of running tasks. Compute a new IPC class performance score for a scheduling group. It is the sum of the performance of the current tasks of all the runqueues. Also, keep track of the task with the lowest IPC class score on the scheduling group. These two metrics will be used during idle load balancing to compute the current and the prospective task-class performance of a scheduling group. Cc: Ben Segall <bsegall@google.com> Cc: Daniel Bristot de Oliveira <bristot@redhat.com> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> Cc: Joel Fernandes (Google) <joel@joelfernandes.org> Cc: Len Brown <len.brown@intel.com> Cc: Mel Gorman <mgorman@suse.de> 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: 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 v1: * Implemented cleanups and reworks from PeterZ. Thanks! * Used the new interface names. --- kernel/sched/fair.c | 55 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)