[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKfTPtCzHf_R4SwR29FsnxXTv2J4Xrmh3gfoHcVPu3KU5OLDcQ@mail.gmail.com>
Date: Tue, 5 Mar 2024 15:05:47 +0100
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Shrikanth Hegde <sshegde@...ux.ibm.com>
Cc: mingo@...nel.org, peterz@...radead.org, yu.c.chen@...el.com,
dietmar.eggemann@....com, linux-kernel@...r.kernel.org, nysal@...ux.ibm.com,
aboorvad@...ux.ibm.com, srikar@...ux.ibm.com, vschneid@...hat.com,
pierre.gondois@....com, morten.rasmussen@....com, qyousef@...alina.io
Subject: Re: [PATCH v4 2/2] sched/fair: Use helper function to access rd->overutilized
On Fri, 1 Mar 2024 at 16:18, Shrikanth Hegde <sshegde@...ux.ibm.com> wrote:
>
> Overutilized field is accessed directly in multiple places.
> So it could use a helper function. That way one might be more
> informed that it needs to be used only in case of EAS.
>
> No change in functionality intended.
>
> Signed-off-by: Shrikanth Hegde <sshegde@...ux.ibm.com>
> ---
> kernel/sched/fair.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index a71f8a1506e4..650909a648d0 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6670,6 +6670,15 @@ static inline bool cpu_overutilized(int cpu)
> return !util_fits_cpu(cpu_util_cfs(cpu), rq_util_min, rq_util_max, cpu);
> }
>
> +/*
> + * Ensure that caller can do EAS. overutilized value
> + * make sense only if EAS is enabled
> + */
> +static inline int is_rd_overutilized(struct root_domain *rd)
> +{
> + return READ_ONCE(rd->overutilized);
> +}
It seems that is_rd_overutilized() is always used with
sched_energy_enabled() in the pattern:
If (sched_energy_enabled() && !is_rd_overutilized(rd))
do something
This pattern includes feec() case where we have in select_task_rq_fair():
If (sched_energy_enabled())
feec():
|-> if (is_rd_overutilized())
|-> goto unlock
which could be changed into
If (sched_energy_enabled() && !is_rd_overutilized(rd))
feec()
Then you can create the function is_rd_not_overutilized() instead of
is_rd_overutilized()
-static inline int is_rd_overutilized(struct root_domain *rd)
+static inline int is_rd_not_overutilized(struct root_domain *rd)
{
- return READ_ONCE(rd->overutilized);
+ return sched_energy_enabled() && READ_ONCE(rd->overutilized);
}
and use is_rd_not_overutilized() instead
> +
> static inline void set_rd_overutilized_status(struct root_domain *rd,
> unsigned int status)
> {
> @@ -6686,13 +6695,14 @@ static inline void check_update_overutilized_status(struct rq *rq)
> if (!sched_energy_enabled())
> return;
>
> - if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu))
> + if (!is_rd_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
> set_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);
> }
> #else
> static inline void check_update_overutilized_status(struct rq *rq) { }
> static inline void set_rd_overutilized_status(struct root_domain *rd,
> unsigned int status) { }
> +static inline int is_rd_overutilized(struct root_domain *rd) { }
It should be
static inline int is_rd_overutilized(struct root_domain *rd) { return 0; }
> #endif
>
> /* Runqueue only has SCHED_IDLE tasks enqueued */
> @@ -7974,7 +7984,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>
> rcu_read_lock();
> pd = rcu_dereference(rd->pd);
> - if (!pd || READ_ONCE(rd->overutilized))
> + if (!pd || is_rd_overutilized(rd))
> goto unlock;
>
> /*
> @@ -10859,7 +10869,7 @@ static struct sched_group *find_busiest_group(struct lb_env *env)
> if (sched_energy_enabled()) {
> struct root_domain *rd = env->dst_rq->rd;
>
> - if (rcu_dereference(rd->pd) && !READ_ONCE(rd->overutilized))
> + if (rcu_dereference(rd->pd) && !is_rd_overutilized(rd))
> goto out_balanced;
> }
>
> --
> 2.39.3
>
Powered by blists - more mailing lists