Mailing List Archive: [PATCH] sched/fair: Combine EAS check with overutilized access

[PATCH] sched/fair: Combine EAS check with overutilized access

Mar 26, 2024, 8:26 AM

Post #1 of 5 (3 views)

Access to overutilized is always used with sched_energy_enabled in
the pattern:

if (sched_energy_enabled && !overutilized)
do something

So modify the helper function to return this pattern. This is more
readable code as it would say, do something when root domain is not
overutilized. This function always return true when EAS is disabled.

No change in functionality intended.

Suggested-by: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
---
kernel/sched/fair.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 24a7530a7d3f..e222e3ad4cfe 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6686,12 +6686,11 @@ static inline bool cpu_overutilized(int cpu)
}

/*
- * Ensure that caller can do EAS. overutilized value
- * make sense only if EAS is enabled
+ * overutilized value make sense only if EAS is enabled
*/
static inline int is_rd_overutilized(struct root_domain *rd)
{
- return READ_ONCE(rd->overutilized);
+ return !sched_energy_enabled() || READ_ONCE(rd->overutilized);
}

static inline void set_rd_overutilized_status(struct root_domain *rd,
@@ -6710,8 +6709,6 @@ static inline void check_update_overutilized_status(struct rq *rq)
* overutilized field is used for load balancing decisions only
* if energy aware scheduler is being used
*/
- if (!sched_energy_enabled())
- return;

if (!is_rd_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
set_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);
@@ -7999,7 +7996,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)

rcu_read_lock();
pd = rcu_dereference(rd->pd);
- if (!pd || is_rd_overutilized(rd))
+ if (!pd)
goto unlock;

/*
@@ -8202,7 +8199,7 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
cpumask_test_cpu(cpu, p->cpus_ptr))
return cpu;

- if (sched_energy_enabled()) {
+ if (!is_rd_overutilized(this_rq()->rd)) {
new_cpu = find_energy_efficient_cpu(p, prev_cpu);
if (new_cpu >= 0)
return new_cpu;
@@ -10903,12 +10900,9 @@ static struct sched_group *sched_balance_find_src_group(struct lb_env *env)
if (busiest->group_type == group_misfit_task)
goto force_balance;

- if (sched_energy_enabled()) {
- struct root_domain *rd = env->dst_rq->rd;
-
- if (rcu_dereference(rd->pd) && !is_rd_overutilized(rd))
- goto out_balanced;
- }
+ if (!is_rd_overutilized(env->dst_rq->rd) &&
+ rcu_dereference(env->dst_rq->rd->pd))
+ goto out_balanced;

/* ASYM feature bypasses nice load balance check */
if (busiest->group_type == group_asym_packing)
--
2.39.3

Re: [PATCH] sched/fair: Combine EAS check with overutilized access [ In reply to ]

mingo at kernel

Mar 26, 2024, 12:32 PM

Post #2 of 5 (3 views)

Permalink

* Shrikanth Hegde <sshegde@linux.ibm.com> wrote:

> Access to overutilized is always used with sched_energy_enabled in
> the pattern:
>
> if (sched_energy_enabled && !overutilized)
> do something
>
> So modify the helper function to return this pattern. This is more
> readable code as it would say, do something when root domain is not
> overutilized. This function always return true when EAS is disabled.
>
> No change in functionality intended.
>
> Suggested-by: Vincent Guittot <vincent.guittot@linaro.org>
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
> kernel/sched/fair.c | 20 +++++++-------------
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 24a7530a7d3f..e222e3ad4cfe 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6686,12 +6686,11 @@ static inline bool cpu_overutilized(int cpu)
> }
>
> /*
> - * Ensure that caller can do EAS. overutilized value
> - * make sense only if EAS is enabled
> + * overutilized value make sense only if EAS is enabled
> */
> static inline int is_rd_overutilized(struct root_domain *rd)
> {
> - return READ_ONCE(rd->overutilized);
> + return !sched_energy_enabled() || READ_ONCE(rd->overutilized);
> }
>
> static inline void set_rd_overutilized_status(struct root_domain *rd,
> @@ -6710,8 +6709,6 @@ static inline void check_update_overutilized_status(struct rq *rq)
> * overutilized field is used for load balancing decisions only
> * if energy aware scheduler is being used
> */
> - if (!sched_energy_enabled())
> - return;
>
> if (!is_rd_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
> set_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);

On a second thought, I'm not sure removing the open-coded
sched_energy_enabled() branches is a good idea: the current code makes it
really, really clear when we are within EAS code paths.

Hiding it within is_rd_overutilized() makes it a lot less obvious IMO.

And this one:

> @@ -8202,7 +8199,7 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
> cpumask_test_cpu(cpu, p->cpus_ptr))
> return cpu;
>
> - if (sched_energy_enabled()) {
> + if (!is_rd_overutilized(this_rq()->rd)) {
> new_cpu = find_energy_efficient_cpu(p, prev_cpu);
> if (new_cpu >= 0)
> return new_cpu;

Didn't have a root_domain::overutilized check before?

Thanks,

Ingo

Re: [PATCH] sched/fair: Combine EAS check with overutilized access [ In reply to ]

vincent.guittot at linaro

Mar 26, 2024, 1:15 PM

Post #3 of 5 (3 views)

Permalink

On Tue, 26 Mar 2024 at 20:32, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Shrikanth Hegde <sshegde@linux.ibm.com> wrote:
>
> > Access to overutilized is always used with sched_energy_enabled in
> > the pattern:
> >
> > if (sched_energy_enabled && !overutilized)
> > do something
> >
> > So modify the helper function to return this pattern. This is more
> > readable code as it would say, do something when root domain is not
> > overutilized. This function always return true when EAS is disabled.
> >
> > No change in functionality intended.
> >
> > Suggested-by: Vincent Guittot <vincent.guittot@linaro.org>
> > Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> > ---
> > kernel/sched/fair.c | 20 +++++++-------------
> > 1 file changed, 7 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 24a7530a7d3f..e222e3ad4cfe 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -6686,12 +6686,11 @@ static inline bool cpu_overutilized(int cpu)
> > }
> >
> > /*
> > - * Ensure that caller can do EAS. overutilized value
> > - * make sense only if EAS is enabled
> > + * overutilized value make sense only if EAS is enabled
> > */
> > static inline int is_rd_overutilized(struct root_domain *rd)
> > {
> > - return READ_ONCE(rd->overutilized);
> > + return !sched_energy_enabled() || READ_ONCE(rd->overutilized);
> > }
> >
> > static inline void set_rd_overutilized_status(struct root_domain *rd,
> > @@ -6710,8 +6709,6 @@ static inline void check_update_overutilized_status(struct rq *rq)
> > * overutilized field is used for load balancing decisions only
> > * if energy aware scheduler is being used
> > */
> > - if (!sched_energy_enabled())
> > - return;
> >
> > if (!is_rd_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
> > set_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);
>
> On a second thought, I'm not sure removing the open-coded
> sched_energy_enabled() branches is a good idea: the current code makes it
> really, really clear when we are within EAS code paths.
>
> Hiding it within is_rd_overutilized() makes it a lot less obvious IMO.

That's probably a matter of pov. I prefer embedding everything to make
sure to have useless access to rd->overutilized

>
> And this one:
>
> > @@ -8202,7 +8199,7 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
> > cpumask_test_cpu(cpu, p->cpus_ptr))
> > return cpu;
> >
> > - if (sched_energy_enabled()) {
> > + if (!is_rd_overutilized(this_rq()->rd)) {
> > new_cpu = find_energy_efficient_cpu(p, prev_cpu);
> > if (new_cpu >= 0)
> > return new_cpu;
>
> Didn't have a root_domain::overutilized check before?

This is the one that was in find_energy_efficient_cpu() before.

>
> Thanks,
>
> Ingo

Re: [PATCH] sched/fair: Combine EAS check with overutilized access [ In reply to ]

mingo at kernel

Mar 28, 2024, 2:43 AM

Post #4 of 5 (3 views)

Permalink

* Vincent Guittot <vincent.guittot@linaro.org> wrote:

> On Tue, 26 Mar 2024 at 20:32, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Shrikanth Hegde <sshegde@linux.ibm.com> wrote:
> >
> > > Access to overutilized is always used with sched_energy_enabled in
> > > the pattern:
> > >
> > > if (sched_energy_enabled && !overutilized)
> > > do something
> > >
> > > So modify the helper function to return this pattern. This is more
> > > readable code as it would say, do something when root domain is not
> > > overutilized. This function always return true when EAS is disabled.
> > >
> > > No change in functionality intended.
> > >
> > > Suggested-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> > > ---
> > > kernel/sched/fair.c | 20 +++++++-------------
> > > 1 file changed, 7 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 24a7530a7d3f..e222e3ad4cfe 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -6686,12 +6686,11 @@ static inline bool cpu_overutilized(int cpu)
> > > }
> > >
> > > /*
> > > - * Ensure that caller can do EAS. overutilized value
> > > - * make sense only if EAS is enabled
> > > + * overutilized value make sense only if EAS is enabled
> > > */
> > > static inline int is_rd_overutilized(struct root_domain *rd)
> > > {
> > > - return READ_ONCE(rd->overutilized);
> > > + return !sched_energy_enabled() || READ_ONCE(rd->overutilized);
> > > }
> > >
> > > static inline void set_rd_overutilized_status(struct root_domain *rd,
> > > @@ -6710,8 +6709,6 @@ static inline void check_update_overutilized_status(struct rq *rq)
> > > * overutilized field is used for load balancing decisions only
> > > * if energy aware scheduler is being used
> > > */
> > > - if (!sched_energy_enabled())
> > > - return;
> > >
> > > if (!is_rd_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
> > > set_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);
> >
> > On a second thought, I'm not sure removing the open-coded
> > sched_energy_enabled() branches is a good idea: the current code makes it
> > really, really clear when we are within EAS code paths.
> >
> > Hiding it within is_rd_overutilized() makes it a lot less obvious IMO.
>
> That's probably a matter of pov. I prefer embedding everything to make
> sure to have useless access to rd->overutilized

Ok, fair enough - applied to tip:sched/core, thanks guys!

Ingo

Re: [PATCH] sched/fair: Combine EAS check with overutilized access [ In reply to ]

qyousef at layalina

Mar 28, 2024, 2:33 PM

Post #5 of 5 (3 views)

Permalink

On 03/26/24 21:15, Vincent Guittot wrote:
> On Tue, 26 Mar 2024 at 20:32, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Shrikanth Hegde <sshegde@linux.ibm.com> wrote:
> >
> > > Access to overutilized is always used with sched_energy_enabled in
> > > the pattern:
> > >
> > > if (sched_energy_enabled && !overutilized)
> > > do something
> > >
> > > So modify the helper function to return this pattern. This is more
> > > readable code as it would say, do something when root domain is not
> > > overutilized. This function always return true when EAS is disabled.
> > >
> > > No change in functionality intended.
> > >
> > > Suggested-by: Vincent Guittot <vincent.guittot@linaro.org>
> > > Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> > > ---
> > > kernel/sched/fair.c | 20 +++++++-------------
> > > 1 file changed, 7 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 24a7530a7d3f..e222e3ad4cfe 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -6686,12 +6686,11 @@ static inline bool cpu_overutilized(int cpu)
> > > }
> > >
> > > /*
> > > - * Ensure that caller can do EAS. overutilized value
> > > - * make sense only if EAS is enabled
> > > + * overutilized value make sense only if EAS is enabled
> > > */
> > > static inline int is_rd_overutilized(struct root_domain *rd)
> > > {
> > > - return READ_ONCE(rd->overutilized);
> > > + return !sched_energy_enabled() || READ_ONCE(rd->overutilized);
> > > }
> > >
> > > static inline void set_rd_overutilized_status(struct root_domain *rd,
> > > @@ -6710,8 +6709,6 @@ static inline void check_update_overutilized_status(struct rq *rq)
> > > * overutilized field is used for load balancing decisions only
> > > * if energy aware scheduler is being used
> > > */
> > > - if (!sched_energy_enabled())
> > > - return;
> > >
> > > if (!is_rd_overutilized(rq->rd) && cpu_overutilized(rq->cpu))
> > > set_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);
> >
> > On a second thought, I'm not sure removing the open-coded
> > sched_energy_enabled() branches is a good idea: the current code makes it
> > really, really clear when we are within EAS code paths.
> >
> > Hiding it within is_rd_overutilized() makes it a lot less obvious IMO.
>
> That's probably a matter of pov. I prefer embedding everything to make
> sure to have useless access to rd->overutilized

I do think it is better this way too.

>
> >
> > And this one:
> >
> > > @@ -8202,7 +8199,7 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
> > > cpumask_test_cpu(cpu, p->cpus_ptr))
> > > return cpu;
> > >
> > > - if (sched_energy_enabled()) {
> > > + if (!is_rd_overutilized(this_rq()->rd)) {
> > > new_cpu = find_energy_efficient_cpu(p, prev_cpu);
> > > if (new_cpu >= 0)
> > > return new_cpu;
> >
> > Didn't have a root_domain::overutilized check before?
>
> This is the one that was in find_energy_efficient_cpu() before.

But not sure about moving is_rd_overutilized() call out of
find_energy_efficient_cpu() is a good call here. Are we trying to save the
function call? Looking at disassembly the function was completely inlined into
select_task_rq_fair() on my recent compilation on Apple M1 system (6.8 kernel).

I see it is applied now. I agree it is a matter of PoV. So no big deal.