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

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

[Shrikanth Hegde]

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

[Ingo Molnar]

* 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

[Vincent Guittot]

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

[Ingo Molnar]

* 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

[Qais Yousef]

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.

[tip: sched/core] sched/fair: Combine EAS check with root_domain::overutilized access

[tip-bot2 for Shrikanth Hegde]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     902e786c4a54a2c4f7462b9026bb56610888db3d
Gitweb:        https://git.kernel.org/tip/902e786c4a54a2c4f7462b9026bb56610888db3d
Author:        Shrikanth Hegde <sshegde@linux.ibm.com>
AuthorDate:    Tue, 26 Mar 2024 20:56:16 +05:30
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 Mar 2024 10:39:18 +01:00

sched/fair: Combine EAS check with root_domain::overutilized access

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

  if (sched_energy_enabled && !overutilized)
         do something

So modify the helper function to utilize 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>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20240326152616.380999-1-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 1856e58..3846230 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)