From: Vincent Donnefort <vincent.donnefort@arm.com>
To: Valentin Schneider <Valentin.Schneider@arm.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>,
peterz@infradead.org, mingo@redhat.com,
linux-kernel@vger.kernel.org, mgorman@techsingularity.net,
dietmar.eggemann@arm.com
Subject: Re: [PATCH] sched/fair: Fix detection of per-CPU kthreads waking a task
Date: Fri, 26 Nov 2021 17:18:17 +0000 [thread overview]
Message-ID: <20211126171817.GA3798214@ubiquitous> (raw)
In-Reply-To: <87pmqmc16f.mognet@arm.com>
On Fri, Nov 26, 2021 at 04:49:12PM +0000, Valentin Schneider wrote:
> On 26/11/21 15:40, Vincent Guittot wrote:
> > On Fri, 26 Nov 2021 at 14:32, Valentin Schneider
> > <Valentin.Schneider@arm.com> wrote:
> >> /*
> >> - * Allow a per-cpu kthread to stack with the wakee if the
> >> - * kworker thread and the tasks previous CPUs are the same.
> >> - * The assumption is that the wakee queued work for the
> >> - * per-cpu kthread that is now complete and the wakeup is
> >> - * essentially a sync wakeup. An obvious example of this
> >> + * Allow a per-cpu kthread to stack with the wakee if the kworker thread
> >> + * and the tasks previous CPUs are the same. The assumption is that the
> >> + * wakee queued work for the per-cpu kthread that is now complete and
> >> + * the wakeup is essentially a sync wakeup. An obvious example of this
> >> * pattern is IO completions.
> >> + *
> >> + * Ensure the wakeup is issued by the kthread itself, and don't match
> >> + * against the idle task because that could override the
> >> + * available_idle_cpu(target) check done higher up.
> >> */
> >> - if (is_per_cpu_kthread(current) &&
> >> + if (is_per_cpu_kthread(current) && !is_idle_task(current) &&
> >
> > still i don't see the need of !is_idle_task(current)
> >
>
> Admittedly, belts and braces. The existing condition checks rq->nr_running <= 1
> which can lead to coscheduling when the wakeup is issued by the idle task
> (or even if rq->nr_running == 0, you can have rq->ttwu_pending without
> having sent an IPI due to polling). Essentially this overrides the first
> check in sis() that uses idle_cpu(target) (prev == smp_processor_id() ==
> target).
>
> I couldn't prove such wakeups can happen right now, but if/when they do
> (AIUI it would just take someone to add a wake_up_process() down some
> smp_call_function() callback) then we'll need the above. If you're still
> not convinced by now, I won't push it further.
From a quick experiment, even with the asym_fits_capacity(), I can trigger
the following:
[ 0.118855] select_idle_sibling: wakee=kthreadd:2 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
[ 0.128214] select_idle_sibling: wakee=rcu_gp:3 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
[ 0.137327] select_idle_sibling: wakee=rcu_par_gp:4 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
[ 0.147221] select_idle_sibling: wakee=kworker/u16:0:7 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
[ 0.156994] select_idle_sibling: wakee=mm_percpu_wq:8 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
[ 0.171943] select_idle_sibling: wakee=rcu_sched:10 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
So the in_task() condition doesn't appear to be enough to filter wakeups
while we have the swapper as a current.
>
> >
> >> + in_task() &&
> >> prev == smp_processor_id() &&
> >> this_rq()->nr_running <= 1) {
> >> return prev;
> >>
next prev parent reply other threads:[~2021-11-26 17:20 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-24 15:42 [PATCH] sched/fair: Fix detection of per-CPU kthreads waking a task Vincent Donnefort
2021-11-24 16:28 ` Valentin Schneider
2021-11-25 9:05 ` Vincent Guittot
2021-11-25 11:16 ` Valentin Schneider
2021-11-25 13:17 ` Dietmar Eggemann
2021-11-25 13:23 ` Vincent Guittot
2021-11-25 15:30 ` Valentin Schneider
2021-11-26 8:23 ` Vincent Guittot
2021-11-26 13:32 ` Valentin Schneider
2021-11-26 14:40 ` Vincent Guittot
2021-11-26 16:49 ` Valentin Schneider
2021-11-26 17:18 ` Vincent Donnefort [this message]
2021-11-29 15:49 ` Vincent Guittot
2021-11-29 16:54 ` Vincent Donnefort
2021-11-30 13:35 ` Dietmar Eggemann
2021-11-30 15:42 ` Vincent Guittot
2021-12-01 14:40 ` Vincent Donnefort
2021-12-01 16:19 ` Vincent Guittot
2021-11-29 8:36 ` [sched/fair] 8d0920b981: stress-ng.sem.ops_per_sec 11.9% improvement kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211126171817.GA3798214@ubiquitous \
--to=vincent.donnefort@arm.com \
--cc=Valentin.Schneider@arm.com \
--cc=dietmar.eggemann@arm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mgorman@techsingularity.net \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=vincent.guittot@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).