diff mbox series

[v2,06/22] sched/fair: Collect load-balancing stats for IPC classes

Message ID 20221128132100.30253-7-ricardo.neri-calderon@linux.intel.com
State New
Headers
Series sched: Introduce IPC classes for load balance |

Commit Message

Ricardo Neri Nov. 28, 2022, 1:20 p.m. UTC 
  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(+)

Comments

Dietmar Eggemann Dec. 7, 2022, 5 p.m. UTC | #1 
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?

[...]
Ionela Voinescu Dec. 8, 2022, 8:50 a.m. UTC | #2 
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)) */
> +};
[..]
Ricardo Neri Dec. 12, 2022, 9:41 p.m. UTC | #3 
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.
> 
> [...]
Ricardo Neri Dec. 14, 2022, 12:31 a.m. UTC | #4 
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 mbox series

Patch

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;