Received-SPF: pass (google.com: domain of linux-kernel+bounces-144548-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240313085817.48892-1-zegao@tencent.com> <65b1691d-8d90-4057-8ad0-da546a0ac8a1@linux.ibm.com>
In-Reply-To: <65b1691d-8d90-4057-8ad0-da546a0ac8a1@linux.ibm.com>
From: Ze Gao <zegao2021@gmail.com>
Date: Mon, 15 Apr 2024 13:40:13 +0800
Message-ID: <CAD8CoPDajt_EciU0x8G_BocajfaJrQ1FKLYkp_nLSfF9+9FJVw@mail.gmail.com>
Subject: Re: [PATCH] sched: Improve rq selection for a blocked task when its
 affinity changes
To: Madadi Vineeth Reddy <vineethr@linux.ibm.com>
Cc: linux-kernel@vger.kernel.org, Ze Gao <zegao@tencent.com>, 
	Ben Segall <bsegall@google.com>, Daniel Bristot de Oliveira <bristot@redhat.com>, 
	Dietmar Eggemann <dietmar.eggemann@arm.com>, Ingo Molnar <mingo@redhat.com>, 
	Juri Lelli <juri.lelli@redhat.com>, Mel Gorman <mgorman@suse.de>, 
	Peter Zijlstra <peterz@infradead.org>, Steven Rostedt <rostedt@goodmis.org>, 
	Valentin Schneider <vschneid@redhat.com>, Vincent Guittot <vincent.guittot@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Apr 13, 2024 at 12:59=E2=80=AFAM Madadi Vineeth Reddy
<vineethr@linux.ibm.com> wrote:
>
> Hi Ze Gao,
>
> On 13/03/24 14:28, Ze Gao wrote:
> > We observered select_idle_sibling() is likely to return the *target* cp=
u
> > early which is likely to be the previous cpu this task is running on ev=
en
> > when it's actually not within the affinity list newly set, from where a=
fter
> > we can only rely on select_fallback_rq() to choose one for us at its wi=
ll
> > (the first valid mostly for now).
> >
> > However, the one chosen by select_fallback_rq() is highly likely not a
> > good enough candidate, sometimes it has to rely on load balancer to kic=
k
> > in to place itself to a better cpu, which adds one or more unnecessary
> > migrations in no doubt. For example, this is what I get when I move tas=
k
> > 3964 to cpu 23-24 where cpu 23 has a cpu bound work pinned already:
> >
> >         swapper       0 [013]   959.791829: sched:sched_migrate_task: c=
omm=3Dstress-ng-cpu pid=3D3964 prio=3D120 orig_cpu=3D13 dest_cpu=3D23
> > kworker/24:2-mm    1014 [024]   959.806148: sched:sched_migrate_task: c=
omm=3Dstress-ng-cpu pid=3D3964 prio=3D120 orig_cpu=3D23 dest_cpu=3D24
> >
>
> I am able to reproduce this scenario of having an extra migration through=
 load balance
> swapper       0 [031] 398764.057232: sched:sched_migrate_task: comm=3Dloo=
p pid=3D178687 prio=3D120 orig_cpu=3D31 dest_cpu=3D33
> ksoftirqd/0  13 [000] 398764.356138: sched:sched_migrate_task: comm=3Dloo=
p pid=3D178687 prio=3D120 orig_cpu=3D33 dest_cpu=3D34
>
> I wrote a simple c program that blocks for few seconds, meanwhile I tasks=
et it to CPUs 33,34 while I already have a
> busy task running on CPU 33.
>
> > The thing is we can actually do better if we do checks early and take m=
ore
> > advantages of the *target* in select_idle_sibling(). That is, we contin=
ue
> > the idle cpu selection if *target* fails the test of cpumask_test_cpu(
> > *target*, p->cpus_ptr). By doing so, we are likely to pick a good candi=
date,
> > especially when the newly allowed cpu set shares some cpu resources wit=
h
> > *target*.
> >
> > And with this change, we clearly see the improvement when I move task 3=
964
> > to cpu 25-26 where cpu 25 has a cpu bound work pinned already.
> >
> >         swapper       0 [027]  4249.204658: sched:sched_migrate_task: c=
omm=3Dstress-ng-cpu pid=3D3964 prio=3D120 orig_cpu=3D27 dest_cpu=3D26
>
> But after applying this patch, The extra migration is still happening as =
CPU 33 is still chosen by try_to_wake_up.
>
> On placing some perf probes and testing,
>     migration/57     304 [057] 12216.988491:       sched:sched_migrate_ta=
sk: comm=3Dloop pid=3D11172 prio=3D120 orig_cpu=3D57 dest_cpu=3D4
>          swapper       0 [004] 12226.989065: probe:select_idle_sibling_L1=
24: (c0000000001bafc0) i=3D-1 recent_used_cpu=3D-1 prev_aff=3D-1
>          swapper       0 [004] 12226.989071:       probe:select_fallback_=
rq: (c0000000001a2e38) cpu=3D4
>          swapper       0 [004] 12226.989074:       sched:sched_migrate_ta=
sk: comm=3Dloop pid=3D11172 prio=3D120 orig_cpu=3D4 dest_cpu=3D33
>          swapper       0 [000] 12227.007768:       sched:sched_migrate_ta=
sk: comm=3Dloop pid=3D11172 prio=3D120 orig_cpu=3D33 dest_cpu=3D34
>
> It is observed that, select_fallback_rq is still taken in this scenario a=
s default target is returned at the end of select_idle_sibling
> which was CPU 4.

After second thoughts, it indeed could happen if CPU 4 shares nothing
with CPU 33(34),
for example, in different numa nodes.

IOW, it cannot benefit from select_idle_siblings() and has to rely on
select_fallback_rq
as the last resort. Just like what I said in the changelog, this patch
aims to improve rq
selection for cases where the newly allowed cpu set shares some cpu
resources with
the old cpu set.

Sorry for not being able to recall all details immediately, since this
thread has been inactive
for a long while and receives no feedback from sched folks.

Best,
Ze

> In most of my testing, default target is returned at the end of the funct=
ion due to the initial checks. It's possible that there would
> be cases where we can get optimal CPU before we reach end of the select_i=
dle_sibling function but it would be interesting to know if the
> extra time spent in finding an optimal cpu have an impact instead of retu=
rning it earlier if in most of the times we are returning the
> default target at the end.
>
> Thanks and Regards
> Madadi Vineeth Reddy
>
> >
> > Note we do the same check for *prev* in select_idle_sibling() as well.
> >
> > Signed-off-by: Ze Gao <zegao@tencent.com>
> > ---
>