It's uncertain whether idle cores exist or not if shared sched-domains
are not ready, so returning "no idle cores" usually makes sense.
While __update_idle_core() is an exception, it checks status of this
core and set to shared sched-domain if necessary. So the whole logic
depends on the existence of shared domain, and can bail out early if
it isn't available. Modern compilers seems capable of handling such
cases, so remove the tricky self-defined default return value.
Signed-off-by: Abel Wu <[email protected]>
---
kernel/sched/fair.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 8bed75757e65..e5e8453e3ffb 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1595,11 +1595,11 @@ numa_type numa_classify(unsigned int imbalance_pct,
#ifdef CONFIG_SCHED_SMT
/* Forward declarations of select_idle_sibling helpers */
-static inline bool test_idle_cores(int cpu, bool def);
+static inline bool test_idle_cores(int cpu);
static inline int numa_idle_core(int idle_core, int cpu)
{
if (!static_branch_likely(&sched_smt_present) ||
- idle_core >= 0 || !test_idle_cores(cpu, false))
+ idle_core >= 0 || !test_idle_cores(cpu))
return idle_core;
/*
@@ -6206,7 +6206,7 @@ static inline void set_idle_cores(int cpu, int val)
WRITE_ONCE(sds->has_idle_cores, val);
}
-static inline bool test_idle_cores(int cpu, bool def)
+static inline bool test_idle_cores(int cpu)
{
struct sched_domain_shared *sds;
@@ -6214,7 +6214,7 @@ static inline bool test_idle_cores(int cpu, bool def)
if (sds)
return READ_ONCE(sds->has_idle_cores);
- return def;
+ return false;
}
/*
@@ -6230,7 +6230,7 @@ void __update_idle_core(struct rq *rq)
int cpu;
rcu_read_lock();
- if (test_idle_cores(core, true))
+ if (test_idle_cores(core))
goto unlock;
for_each_cpu(cpu, cpu_smt_mask(core)) {
@@ -6306,9 +6306,9 @@ static inline void set_idle_cores(int cpu, int val)
{
}
-static inline bool test_idle_cores(int cpu, bool def)
+static inline bool test_idle_cores(int cpu)
{
- return def;
+ return false;
}
static inline int select_idle_core(struct task_struct *p, int core, struct cpumask *cpus, int *idle_cpu)
@@ -6535,7 +6535,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
return target;
if (sched_smt_active()) {
- has_idle_core = test_idle_cores(target, false);
+ has_idle_core = test_idle_cores(target);
if (!has_idle_core && cpus_share_cache(prev, target)) {
i = select_idle_smt(p, sd, prev);
--
2.31.1
On Sun, Jun 19, 2022 at 5:05 AM Abel Wu <[email protected]> wrote:
>
> It's uncertain whether idle cores exist or not if shared sched-domains
> are not ready, so returning "no idle cores" usually makes sense.
>
> While __update_idle_core() is an exception, it checks status of this
> core and set to shared sched-domain if necessary. So the whole logic
> depends on the existence of shared domain, and can bail out early if
> it isn't available. Modern compilers seems capable of handling such
> cases, so remove the tricky self-defined default return value.
I don't think the compiler will be able to bail out of the smt
iteration early, since it'll have to do another rcu dereference for
the sd_llc in set(). But I also don't think this case needs
optimization, since it should be transient while the domain isn't
ready.
Reviewed-by: Josh Don <[email protected]>
Hi Josh, thanks for your comments!
On 6/28/22 6:53 AM, Josh Don Wrote:
> On Sun, Jun 19, 2022 at 5:05 AM Abel Wu <[email protected]> wrote:
>>
>> It's uncertain whether idle cores exist or not if shared sched-domains
>> are not ready, so returning "no idle cores" usually makes sense.
>>
>> While __update_idle_core() is an exception, it checks status of this
>> core and set to shared sched-domain if necessary. So the whole logic
>> depends on the existence of shared domain, and can bail out early if
>> it isn't available. Modern compilers seems capable of handling such
>> cases, so remove the tricky self-defined default return value.
>
> I don't think the compiler will be able to bail out of the smt
> iteration early, since it'll have to do another rcu dereference for
> the sd_llc in set(). But I also don't think this case needs
> optimization, since it should be transient while the domain isn't
> ready.
>
> Reviewed-by: Josh Don <[email protected]>
Obviously I failed to comprehend the difference between the changed
assembly code, my bad..
Thanks,
Abel