Received-SPF: pass (google.com: domain of linux-kernel+bounces-144466-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 10:03:12 +0800
Message-ID: <CAD8CoPDxgL+dkUM_6oAGuAT-GQVdfrckJB2yOjufvbFE14hBxg@mail.gmail.com>
Subject: Re: [PATCH] sched: Improve rq selection for a blocked task when its
 affinity changes
To: 20240313085817.48892-1-zegao@tencent.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>, 
	Madadi Vineeth Reddy <vineethr@linux.ibm.com>
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.

My best guess is that you may have hit the code path for running tasks
(taskset happens right after the task is woken up). Should that happen,
the picking is done via:

        dest_cpu =3D cpumask_any_and_distribute(cpu_valid_mask, ctx->new_ma=
sk);

and it also makes sense that select_fallback_rq() returns 4 since that happ=
ens
before you change the affinities.

you may need to rule out this case first :)

Regards,
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>
> > ---
>