2020-04-16 01:08:17

by Valentin Schneider

[permalink] [raw]
Subject: [PATCH v3 9/9] sched/topology: Define and use shortcut pointers for wakeup sd_flag scan

Reworking select_task_rq_fair()'s domain walk exposed that !want_affine
wakeups only look for highest sched_domain with the required sd_flag
set. This is something we can cache at sched domain build time to slightly
optimize select_task_rq_fair(). Note that this isn't a "free" optimization:
it costs us 3 pointers per CPU.

Add cached per-CPU pointers for the highest domains with SD_BALANCE_WAKE,
SD_BALANCE_EXEC and SD_BALANCE_FORK. Use them in select_task_rq_fair().

Signed-off-by: Valentin Schneider <[email protected]>
---
kernel/sched/fair.c | 25 +++++++++++++------------
kernel/sched/sched.h | 3 +++
kernel/sched/topology.c | 12 ++++++++++++
3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 6f8cdb99f4a0..db4fb29a88d9 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6631,17 +6631,6 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
int want_affine = 0;
int sd_flag;

- switch (wake_flags & (WF_TTWU | WF_FORK | WF_EXEC)) {
- case WF_TTWU:
- sd_flag = SD_BALANCE_WAKE;
- break;
- case WF_FORK:
- sd_flag = SD_BALANCE_FORK;
- break;
- default:
- sd_flag = SD_BALANCE_EXEC;
- }
-
if (wake_flags & WF_TTWU) {
record_wakee(p);

@@ -6657,7 +6646,19 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)

rcu_read_lock();

- sd = highest_flag_domain(cpu, sd_flag);
+ switch (wake_flags & (WF_TTWU | WF_FORK | WF_EXEC)) {
+ case WF_TTWU:
+ sd_flag = SD_BALANCE_WAKE;
+ sd = rcu_dereference(per_cpu(sd_balance_wake, cpu));
+ break;
+ case WF_FORK:
+ sd_flag = SD_BALANCE_FORK;
+ sd = rcu_dereference(per_cpu(sd_balance_fork, cpu));
+ break;
+ default:
+ sd_flag = SD_BALANCE_EXEC;
+ sd = rcu_dereference(per_cpu(sd_balance_exec, cpu));
+ }

/*
* If !want_affine, we just look for the highest domain where
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 448f5d630544..4b014103affb 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1423,6 +1423,9 @@ DECLARE_PER_CPU(int, sd_llc_size);
DECLARE_PER_CPU(int, sd_llc_id);
DECLARE_PER_CPU(struct sched_domain_shared __rcu *, sd_llc_shared);
DECLARE_PER_CPU(struct sched_domain __rcu *, sd_numa);
+DECLARE_PER_CPU(struct sched_domain __rcu *, sd_balance_wake);
+DECLARE_PER_CPU(struct sched_domain __rcu *, sd_balance_fork);
+DECLARE_PER_CPU(struct sched_domain __rcu *, sd_balance_exec);
DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
extern struct static_key_false sched_asym_cpucapacity;
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 1d7b446fac7d..66763c539bbd 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -618,6 +618,9 @@ DEFINE_PER_CPU(int, sd_llc_size);
DEFINE_PER_CPU(int, sd_llc_id);
DEFINE_PER_CPU(struct sched_domain_shared __rcu *, sd_llc_shared);
DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa);
+DEFINE_PER_CPU(struct sched_domain __rcu *, sd_balance_wake);
+DEFINE_PER_CPU(struct sched_domain __rcu *, sd_balance_fork);
+DEFINE_PER_CPU(struct sched_domain __rcu *, sd_balance_exec);
DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity);
@@ -644,6 +647,15 @@ static void update_top_cache_domain(int cpu)
sd = lowest_flag_domain(cpu, SD_NUMA);
rcu_assign_pointer(per_cpu(sd_numa, cpu), sd);

+ sd = highest_flag_domain(cpu, SD_BALANCE_WAKE);
+ rcu_assign_pointer(per_cpu(sd_balance_wake, cpu), sd);
+
+ sd = highest_flag_domain(cpu, SD_BALANCE_FORK);
+ rcu_assign_pointer(per_cpu(sd_balance_fork, cpu), sd);
+
+ sd = highest_flag_domain(cpu, SD_BALANCE_EXEC);
+ rcu_assign_pointer(per_cpu(sd_balance_exec, cpu), sd);
+
sd = highest_flag_domain(cpu, SD_ASYM_PACKING);
rcu_assign_pointer(per_cpu(sd_asym_packing, cpu), sd);

--
2.24.0


2020-04-16 07:50:37

by Vincent Guittot

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] sched/topology: Define and use shortcut pointers for wakeup sd_flag scan

On Wed, 15 Apr 2020 at 23:05, Valentin Schneider
<[email protected]> wrote:
>
> Reworking select_task_rq_fair()'s domain walk exposed that !want_affine
> wakeups only look for highest sched_domain with the required sd_flag
> set. This is something we can cache at sched domain build time to slightly
> optimize select_task_rq_fair(). Note that this isn't a "free" optimization:
> it costs us 3 pointers per CPU.
>
> Add cached per-CPU pointers for the highest domains with SD_BALANCE_WAKE,
> SD_BALANCE_EXEC and SD_BALANCE_FORK. Use them in select_task_rq_fair().
>
> Signed-off-by: Valentin Schneider <[email protected]>
> ---
> kernel/sched/fair.c | 25 +++++++++++++------------
> kernel/sched/sched.h | 3 +++
> kernel/sched/topology.c | 12 ++++++++++++
> 3 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 6f8cdb99f4a0..db4fb29a88d9 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6631,17 +6631,6 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
> int want_affine = 0;
> int sd_flag;
>
> - switch (wake_flags & (WF_TTWU | WF_FORK | WF_EXEC)) {
> - case WF_TTWU:
> - sd_flag = SD_BALANCE_WAKE;
> - break;
> - case WF_FORK:
> - sd_flag = SD_BALANCE_FORK;
> - break;
> - default:
> - sd_flag = SD_BALANCE_EXEC;
> - }
> -
> if (wake_flags & WF_TTWU) {
> record_wakee(p);
>
> @@ -6657,7 +6646,19 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
>
> rcu_read_lock();
>
> - sd = highest_flag_domain(cpu, sd_flag);
> + switch (wake_flags & (WF_TTWU | WF_FORK | WF_EXEC)) {
> + case WF_TTWU:
> + sd_flag = SD_BALANCE_WAKE;
> + sd = rcu_dereference(per_cpu(sd_balance_wake, cpu));

It's worth having a direct pointer for the fast path which we always
try to keep short but the other paths are already slow and will not
get any benefit of this per cpu pointer.
We should keep the loop for the slow paths

> + break;
> + case WF_FORK:
> + sd_flag = SD_BALANCE_FORK;
> + sd = rcu_dereference(per_cpu(sd_balance_fork, cpu));
> + break;
> + default:
> + sd_flag = SD_BALANCE_EXEC;
> + sd = rcu_dereference(per_cpu(sd_balance_exec, cpu));
> + }
>
> /*
> * If !want_affine, we just look for the highest domain where
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 448f5d630544..4b014103affb 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1423,6 +1423,9 @@ DECLARE_PER_CPU(int, sd_llc_size);
> DECLARE_PER_CPU(int, sd_llc_id);
> DECLARE_PER_CPU(struct sched_domain_shared __rcu *, sd_llc_shared);
> DECLARE_PER_CPU(struct sched_domain __rcu *, sd_numa);
> +DECLARE_PER_CPU(struct sched_domain __rcu *, sd_balance_wake);
> +DECLARE_PER_CPU(struct sched_domain __rcu *, sd_balance_fork);
> +DECLARE_PER_CPU(struct sched_domain __rcu *, sd_balance_exec);
> DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
> DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> extern struct static_key_false sched_asym_cpucapacity;
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 1d7b446fac7d..66763c539bbd 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -618,6 +618,9 @@ DEFINE_PER_CPU(int, sd_llc_size);
> DEFINE_PER_CPU(int, sd_llc_id);
> DEFINE_PER_CPU(struct sched_domain_shared __rcu *, sd_llc_shared);
> DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa);
> +DEFINE_PER_CPU(struct sched_domain __rcu *, sd_balance_wake);
> +DEFINE_PER_CPU(struct sched_domain __rcu *, sd_balance_fork);
> +DEFINE_PER_CPU(struct sched_domain __rcu *, sd_balance_exec);
> DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
> DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity);
> @@ -644,6 +647,15 @@ static void update_top_cache_domain(int cpu)
> sd = lowest_flag_domain(cpu, SD_NUMA);
> rcu_assign_pointer(per_cpu(sd_numa, cpu), sd);
>
> + sd = highest_flag_domain(cpu, SD_BALANCE_WAKE);
> + rcu_assign_pointer(per_cpu(sd_balance_wake, cpu), sd);
> +
> + sd = highest_flag_domain(cpu, SD_BALANCE_FORK);
> + rcu_assign_pointer(per_cpu(sd_balance_fork, cpu), sd);
> +
> + sd = highest_flag_domain(cpu, SD_BALANCE_EXEC);
> + rcu_assign_pointer(per_cpu(sd_balance_exec, cpu), sd);
> +
> sd = highest_flag_domain(cpu, SD_ASYM_PACKING);
> rcu_assign_pointer(per_cpu(sd_asym_packing, cpu), sd);
>
> --
> 2.24.0
>

2020-04-16 11:49:27

by Valentin Schneider

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] sched/topology: Define and use shortcut pointers for wakeup sd_flag scan


On 16/04/20 08:46, Vincent Guittot wrote:
>> @@ -6657,7 +6646,19 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
>>
>> rcu_read_lock();
>>
>> - sd = highest_flag_domain(cpu, sd_flag);
>> + switch (wake_flags & (WF_TTWU | WF_FORK | WF_EXEC)) {
>> + case WF_TTWU:
>> + sd_flag = SD_BALANCE_WAKE;
>> + sd = rcu_dereference(per_cpu(sd_balance_wake, cpu));
>
> It's worth having a direct pointer for the fast path which we always
> try to keep short but the other paths are already slow and will not
> get any benefit of this per cpu pointer.
> We should keep the loop for the slow paths
>

Which fast/slow paths are you referring to here? want_affine vs
!want_affine? If so, do you then mean that we should do the switch case
only when !want_affine, and otherwise look for the domain via the
for_each_domain() loop?

>> + break;
>> + case WF_FORK:
>> + sd_flag = SD_BALANCE_FORK;
>> + sd = rcu_dereference(per_cpu(sd_balance_fork, cpu));
>> + break;
>> + default:
>> + sd_flag = SD_BALANCE_EXEC;
>> + sd = rcu_dereference(per_cpu(sd_balance_exec, cpu));
>> + }
>>
>> /*
>> * If !want_affine, we just look for the highest domain where

2020-04-16 13:07:14

by Dietmar Eggemann

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] sched/topology: Define and use shortcut pointers for wakeup sd_flag scan

On 16.04.20 12:24, Valentin Schneider wrote:
>
> On 16/04/20 08:46, Vincent Guittot wrote:
>>> @@ -6657,7 +6646,19 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
>>>
>>> rcu_read_lock();
>>>
>>> - sd = highest_flag_domain(cpu, sd_flag);
>>> + switch (wake_flags & (WF_TTWU | WF_FORK | WF_EXEC)) {
>>> + case WF_TTWU:
>>> + sd_flag = SD_BALANCE_WAKE;
>>> + sd = rcu_dereference(per_cpu(sd_balance_wake, cpu));
>>
>> It's worth having a direct pointer for the fast path which we always
>> try to keep short but the other paths are already slow and will not
>> get any benefit of this per cpu pointer.
>> We should keep the loop for the slow paths
>>
>
> Which fast/slow paths are you referring to here? want_affine vs
> !want_affine? If so, do you then mean that we should do the switch case
> only when !want_affine, and otherwise look for the domain via the
> for_each_domain() loop?

Coming back to the v2 discussion on this patch

https://lore.kernel.org/r/[email protected]

SD_BALANCE_WAKE is not used in mainline anymore, so wakeups are always
fast today.

I.e. you wouldn't need a per_cpu(sd_balance_wake, cpu) since it's always
NULL.

I.e. want_affine logic and the 'for_each_domain(cpu, tmp)' isn't needed
anymore.

This will dramatically simplify the code in select_task_rq_fair().

But I guess Vincent wants to keep the functionality so we're able to
enable SD_BALANCE_WAKE on certain sd's?

2020-04-16 20:09:49

by Vincent Guittot

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] sched/topology: Define and use shortcut pointers for wakeup sd_flag scan

On Thu, 16 Apr 2020 at 15:04, Dietmar Eggemann <[email protected]> wrote:
>
> On 16.04.20 12:24, Valentin Schneider wrote:
> >
> > On 16/04/20 08:46, Vincent Guittot wrote:
> >>> @@ -6657,7 +6646,19 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
> >>>
> >>> rcu_read_lock();
> >>>
> >>> - sd = highest_flag_domain(cpu, sd_flag);
> >>> + switch (wake_flags & (WF_TTWU | WF_FORK | WF_EXEC)) {
> >>> + case WF_TTWU:
> >>> + sd_flag = SD_BALANCE_WAKE;
> >>> + sd = rcu_dereference(per_cpu(sd_balance_wake, cpu));
> >>
> >> It's worth having a direct pointer for the fast path which we always
> >> try to keep short but the other paths are already slow and will not
> >> get any benefit of this per cpu pointer.
> >> We should keep the loop for the slow paths
> >>
> >
> > Which fast/slow paths are you referring to here? want_affine vs
> > !want_affine? If so, do you then mean that we should do the switch case
> > only when !want_affine, and otherwise look for the domain via the
> > for_each_domain() loop?
>
> Coming back to the v2 discussion on this patch
>
> https://lore.kernel.org/r/[email protected]
>
> SD_BALANCE_WAKE is not used in mainline anymore, so wakeups are always
> fast today.
>
> I.e. you wouldn't need a per_cpu(sd_balance_wake, cpu) since it's always
> NULL.
>
> I.e. want_affine logic and the 'for_each_domain(cpu, tmp)' isn't needed
> anymore.
>
> This will dramatically simplify the code in select_task_rq_fair().
>
> But I guess Vincent wants to keep the functionality so we're able to
> enable SD_BALANCE_WAKE on certain sd's?

I looked too quickly what was done by this patch. I thought that it
was adding a per_cpu pointer for all cases including the fast path
with wake affine but it only skips the for_each_domain loop for the
slow paths which don't need it because they are already slow.

It would be better to keep the for_each_domain loop for slow paths and
to use a per_cpu pointer for fast_path/wake affine. Regarding the
wake_affine path, we don't really care about looping all domains and
we could directly use the highest domain because wake_affine() that is
used in the loop, only uses the imbalance_pct of the sched domain for
wake_affine_weight() and it should not harm to use only the highest
domain and then select_idle_sibling doesn't use it but the llc or
asym_capacity pointer instead.

2020-04-16 20:35:04

by Valentin Schneider

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] sched/topology: Define and use shortcut pointers for wakeup sd_flag scan


On 16/04/20 14:36, Vincent Guittot wrote:
>> Coming back to the v2 discussion on this patch
>>
>> https://lore.kernel.org/r/[email protected]
>>
>> SD_BALANCE_WAKE is not used in mainline anymore, so wakeups are always
>> fast today.
>>
>> I.e. you wouldn't need a per_cpu(sd_balance_wake, cpu) since it's always
>> NULL.
>>
>> I.e. want_affine logic and the 'for_each_domain(cpu, tmp)' isn't needed
>> anymore.
>>
>> This will dramatically simplify the code in select_task_rq_fair().
>>
>> But I guess Vincent wants to keep the functionality so we're able to
>> enable SD_BALANCE_WAKE on certain sd's?
>
> I looked too quickly what was done by this patch. I thought that it
> was adding a per_cpu pointer for all cases including the fast path
> with wake affine but it only skips the for_each_domain loop for the
> slow paths which don't need it because they are already slow.
>
> It would be better to keep the for_each_domain loop for slow paths and
> to use a per_cpu pointer for fast_path/wake affine. Regarding the
> wake_affine path, we don't really care about looping all domains and
> we could directly use the highest domain because wake_affine() that is
> used in the loop, only uses the imbalance_pct of the sched domain for
> wake_affine_weight() and it should not harm to use only the highest
> domain and then select_idle_sibling doesn't use it but the llc or
> asym_capacity pointer instead.

So Dietmar's pointing out that sd will always be NULL for want_affine,
because want_affine can only be true at wakeups and we don't have any
topologies with SD_BALANCE_WAKE anymore. We would still want to call
wake_affine() though, because that can change new_cpu.

What you are adding on top is that the only sd field used in wake_affine()
is the imbalance_pct, so we could take a shortcut and just go for the
highest domain with SD_WAKE_AFFINE; i.e. something like this:

---
if (want_affine) {
// We can cache that at topology buildup
sd = highest_flag_domain(cpu, SD_WAKE_AFFINE);

if (cpumask_test_cpu(prev_cpu, sched_domain_span(sd) &&
cpu != prev_cpu)
new_cpu = wake_affine();

// Directly go to select_idle_sibling()
goto sis;
}

// !want_affine logic here
---

As for the !want_affine part, we could either keep the current domain walk
(IIUC this is what you are suggesting) or go for the extra cached pointers
I'm introducing.

Now if we are a bit bolder than that, because there are no more
(mainline) topologies w/ SD_BALANCE_WAKE, we could even turn the above
into:

---
if (wake_flags & WF_TTWU) {
if (want_affine) {
// We can cache that at topology buildup
sd = highest_flag_domain(cpu, SD_WAKE_AFFINE);

if (cpumask_test_cpu(prev_cpu, sched_domain_span(sd) &&
cpu != prev_cpu)
new_cpu = wake_affine();

}
// Directly go to select_idle_sibling()
goto sis;
}

// !want_affine logic here
---

This in turns mean we could get rid of SD_BALANCE_WAKE entirely... I'm a
bit more reluctant to that only because the last SD_BALANCE_WAKE setter was
removed fairly recently, see
a526d466798d ("sched/topology: Remove SD_BALANCE_WAKE on asymmetric capacity systems")

2020-04-16 20:38:44

by Vincent Guittot

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] sched/topology: Define and use shortcut pointers for wakeup sd_flag scan

On Thu, 16 Apr 2020 at 17:27, Valentin Schneider
<[email protected]> wrote:
>
>
> On 16/04/20 14:36, Vincent Guittot wrote:
> >> Coming back to the v2 discussion on this patch
> >>
> >> https://lore.kernel.org/r/[email protected]
> >>
> >> SD_BALANCE_WAKE is not used in mainline anymore, so wakeups are always
> >> fast today.
> >>
> >> I.e. you wouldn't need a per_cpu(sd_balance_wake, cpu) since it's always
> >> NULL.
> >>
> >> I.e. want_affine logic and the 'for_each_domain(cpu, tmp)' isn't needed
> >> anymore.
> >>
> >> This will dramatically simplify the code in select_task_rq_fair().
> >>
> >> But I guess Vincent wants to keep the functionality so we're able to
> >> enable SD_BALANCE_WAKE on certain sd's?
> >
> > I looked too quickly what was done by this patch. I thought that it
> > was adding a per_cpu pointer for all cases including the fast path
> > with wake affine but it only skips the for_each_domain loop for the
> > slow paths which don't need it because they are already slow.
> >
> > It would be better to keep the for_each_domain loop for slow paths and
> > to use a per_cpu pointer for fast_path/wake affine. Regarding the
> > wake_affine path, we don't really care about looping all domains and
> > we could directly use the highest domain because wake_affine() that is
> > used in the loop, only uses the imbalance_pct of the sched domain for
> > wake_affine_weight() and it should not harm to use only the highest
> > domain and then select_idle_sibling doesn't use it but the llc or
> > asym_capacity pointer instead.
>
> So Dietmar's pointing out that sd will always be NULL for want_affine,
> because want_affine can only be true at wakeups and we don't have any
> topologies with SD_BALANCE_WAKE anymore. We would still want to call
> wake_affine() though, because that can change new_cpu.
>
> What you are adding on top is that the only sd field used in wake_affine()
> is the imbalance_pct, so we could take a shortcut and just go for the
> highest domain with SD_WAKE_AFFINE; i.e. something like this:
>
> ---
> if (want_affine) {
> // We can cache that at topology buildup
> sd = highest_flag_domain(cpu, SD_WAKE_AFFINE);

Yes and this one should be cached at topology buildup

>
> if (cpumask_test_cpu(prev_cpu, sched_domain_span(sd) &&
> cpu != prev_cpu)
> new_cpu = wake_affine();
>
> // Directly go to select_idle_sibling()
> goto sis;
> }
>
> // !want_affine logic here
> ---
>
> As for the !want_affine part, we could either keep the current domain walk
> (IIUC this is what you are suggesting) or go for the extra cached pointers
> I'm introducing.
>
> Now if we are a bit bolder than that, because there are no more
> (mainline) topologies w/ SD_BALANCE_WAKE, we could even turn the above
> into:
>
> ---
> if (wake_flags & WF_TTWU) {
> if (want_affine) {
> // We can cache that at topology buildup
> sd = highest_flag_domain(cpu, SD_WAKE_AFFINE);
>
> if (cpumask_test_cpu(prev_cpu, sched_domain_span(sd) &&
> cpu != prev_cpu)
> new_cpu = wake_affine();
>
> }
> // Directly go to select_idle_sibling()
> goto sis;
> }
>
> // !want_affine logic here
> ---
>
> This in turns mean we could get rid of SD_BALANCE_WAKE entirely... I'm a
> bit more reluctant to that only because the last SD_BALANCE_WAKE setter was

For now, we should probably skip the additional test above: "if
(wake_flags & WF_TTWU) {" and keep SD_BALANCE_WAKE so we will continue
to loop in case of !want_affine.

We can imagine that we might want at the end to be a bit more smart
for SD_BALANCE_WAKE and the slow path... like with the latency nice
proposal and latency-nice=19 as a example

> removed fairly recently, see
> a526d466798d ("sched/topology: Remove SD_BALANCE_WAKE on asymmetric capacity systems")

2020-04-16 20:59:27

by Valentin Schneider

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] sched/topology: Define and use shortcut pointers for wakeup sd_flag scan


On 16/04/20 16:58, Vincent Guittot wrote:
>> ---
>> if (wake_flags & WF_TTWU) {
>> if (want_affine) {
>> // We can cache that at topology buildup
>> sd = highest_flag_domain(cpu, SD_WAKE_AFFINE);
>>
>> if (cpumask_test_cpu(prev_cpu, sched_domain_span(sd) &&
>> cpu != prev_cpu)
>> new_cpu = wake_affine();
>>
>> }
>> // Directly go to select_idle_sibling()
>> goto sis;
>> }
>>
>> // !want_affine logic here
>> ---
>>
>> This in turns mean we could get rid of SD_BALANCE_WAKE entirely... I'm a
>> bit more reluctant to that only because the last SD_BALANCE_WAKE setter was
>
> For now, we should probably skip the additional test above: "if
> (wake_flags & WF_TTWU) {" and keep SD_BALANCE_WAKE so we will continue
> to loop in case of !want_affine.
>
> We can imagine that we might want at the end to be a bit more smart
> for SD_BALANCE_WAKE and the slow path... like with the latency nice
> proposal and latency-nice=19 as a example
>

Good point. I'll go for the first option and see where I end up; I'd like
to cache the other domain pointers if possible, I'll do some benchmarking
and see if I can do that without another switch case.

>> removed fairly recently, see
>> a526d466798d ("sched/topology: Remove SD_BALANCE_WAKE on asymmetric capacity systems")