Messages in this thread | | | From | Vincent Guittot <> | Date | Thu, 16 Apr 2020 17:58:47 +0200 | 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 <valentin.schneider@arm.com> wrote: > > > On 16/04/20 14:36, Vincent Guittot wrote: > >> Coming back to the v2 discussion on this patch > >> > >> https://lore.kernel.org/r/20200311181601.18314-10-valentin.schneider@arm.com > >> > >> 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")
| |