* [PATCH] sched/fair: Combine EAS check with overutilized access
@ 2024-03-26 15:26 Shrikanth Hegde
2024-03-26 19:32 ` Ingo Molnar
2024-03-28 9:50 ` [tip: sched/core] sched/fair: Combine EAS check with root_domain::overutilized access tip-bot2 for Shrikanth Hegde
0 siblings, 2 replies; 6+ messages in thread
From: Shrikanth Hegde @ 2024-03-26 15:26 UTC (permalink / raw)
To: mingo, peterz, vincent.guittot
Cc: sshegde, dietmar.eggemann, qyousef, linux-kernel, vschneid
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/fair: Combine EAS check with overutilized access
2024-03-26 15:26 [PATCH] sched/fair: Combine EAS check with overutilized access Shrikanth Hegde
@ 2024-03-26 19:32 ` Ingo Molnar
2024-03-26 20:15 ` Vincent Guittot
2024-03-28 9:50 ` [tip: sched/core] sched/fair: Combine EAS check with root_domain::overutilized access tip-bot2 for Shrikanth Hegde
1 sibling, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2024-03-26 19:32 UTC (permalink / raw)
To: Shrikanth Hegde
Cc: peterz, vincent.guittot, dietmar.eggemann, qyousef, linux-kernel,
vschneid
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/fair: Combine EAS check with overutilized access
2024-03-26 19:32 ` Ingo Molnar
@ 2024-03-26 20:15 ` Vincent Guittot
2024-03-28 9:43 ` Ingo Molnar
2024-03-28 21:33 ` Qais Yousef
0 siblings, 2 replies; 6+ messages in thread
From: Vincent Guittot @ 2024-03-26 20:15 UTC (permalink / raw)
To: Ingo Molnar
Cc: Shrikanth Hegde, peterz, dietmar.eggemann, qyousef, linux-kernel,
vschneid
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/fair: Combine EAS check with overutilized access
2024-03-26 20:15 ` Vincent Guittot
@ 2024-03-28 9:43 ` Ingo Molnar
2024-03-28 21:33 ` Qais Yousef
1 sibling, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2024-03-28 9:43 UTC (permalink / raw)
To: Vincent Guittot
Cc: Shrikanth Hegde, peterz, dietmar.eggemann, qyousef, linux-kernel,
vschneid
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: sched/core] sched/fair: Combine EAS check with root_domain::overutilized access
2024-03-26 15:26 [PATCH] sched/fair: Combine EAS check with overutilized access Shrikanth Hegde
2024-03-26 19:32 ` Ingo Molnar
@ 2024-03-28 9:50 ` tip-bot2 for Shrikanth Hegde
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot2 for Shrikanth Hegde @ 2024-03-28 9:50 UTC (permalink / raw)
To: linux-tip-commits
Cc: Vincent Guittot, Shrikanth Hegde, Ingo Molnar, x86, linux-kernel
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sched/fair: Combine EAS check with overutilized access
2024-03-26 20:15 ` Vincent Guittot
2024-03-28 9:43 ` Ingo Molnar
@ 2024-03-28 21:33 ` Qais Yousef
1 sibling, 0 replies; 6+ messages in thread
From: Qais Yousef @ 2024-03-28 21:33 UTC (permalink / raw)
To: Vincent Guittot
Cc: Ingo Molnar, Shrikanth Hegde, peterz, dietmar.eggemann,
linux-kernel, vschneid
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-28 21:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-26 15:26 [PATCH] sched/fair: Combine EAS check with overutilized access Shrikanth Hegde
2024-03-26 19:32 ` Ingo Molnar
2024-03-26 20:15 ` Vincent Guittot
2024-03-28 9:43 ` Ingo Molnar
2024-03-28 21:33 ` Qais Yousef
2024-03-28 9:50 ` [tip: sched/core] sched/fair: Combine EAS check with root_domain::overutilized access tip-bot2 for Shrikanth Hegde
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.