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

From: Vincent Guittot <vincent.guittot@linaro.org>
To: Valentin Schneider <valentin.schneider@arm.com>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v3 9/9] sched/topology: Define and use shortcut pointers for wakeup sd_flag scan
Date: Thu, 16 Apr 2020 17:58:47 +0200	[thread overview]
Message-ID: <CAKfTPtDFD7KMcSK-2_LP-_APN2m05pWVhzKvJOVnD+Nacn846w@mail.gmail.com> (raw)
In-Reply-To: <jhjv9lz78nv.mognet@arm.com>

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")