From: Vincent Donnefort <vincent.donnefort@arm.com>
To: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Valentin Schneider <Valentin.Schneider@arm.com>,
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: Wed, 1 Dec 2021 14:40:05 +0000 [thread overview]
Message-ID: <20211201144005.GA479680@ubiquitous> (raw)
In-Reply-To: <CAKfTPtCU3TQC06j-nUgsv-7+Vn+XyKwJFv5EwCcTDrPObGttEw@mail.gmail.com>
On Tue, Nov 30, 2021 at 04:42:03PM +0100, Vincent Guittot wrote:
> On Mon, 29 Nov 2021 at 17:54, Vincent Donnefort
> <vincent.donnefort@arm.com> wrote:
> >
> > [...]
> >
> > > > > >
> > > > > > 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
> > >
> > > Timestamp shows its booting phase and thread name above shows per cpu
> > > thread. Could it happen just while creating per cpu thread at boot and
> > > as a result not relevant ?
> >
> > I have more of those logs a bit later in the boot:
> >
> > [ 0.484791] select_idle_sibling: wakee=kthreadd:2 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
> > [ 0.516495] select_idle_sibling: wakee=kthreadd:2 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
> > [ 0.525758] select_idle_sibling: wakee=kthreadd:2 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
> > [ 0.535078] select_idle_sibling: wakee=kthreadd:2 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
> > [ 0.547486] select_idle_sibling: wakee=kthreadd:2 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
> > [ 0.579192] select_idle_sibling: wakee=kthreadd:2 nr_cpus_allowed=8 current=swapper/0:1 in_task=1
> >
> > The nr_cpus_allowed=8 suggest that none of the threads from the logs I
> > shared are per-CPU. Sorry if the format is confusing, I used:
> >
> > wakee=<comm>:<pid> current=<comm>:<pid>.
> >
> > >
> > > Can you see similar things later after booting ?
> >
> > I tried few scenarios other than the boot time but none of them produced
> > "current=swapper/X:1 in_task=1"
> >
> > >
> > > I have tried to trigger the situation but failed to get wrong
> > > sequence. All are coming from interrupt while idle.
> > > After adding in_task() condition, I haven't been able to trigger the
> > > warn() that I added to catch the wrong situations on SMP, Heterogenous
> > > or NUMA system. Could you share more details on your setup ?
> > >
> >
> > This is just my Hikey960 with the asym_fits_capacity() fix [1] to make sure I
> > don't simply hit the other issue with asym platforms.
>
> I ran my previous tests on dragonboard 845c which is dynamiQ and I
> have tried on my hikey960 since but without any success so far. This
> is what i use:
>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6397,9 +6397,12 @@ static int select_idle_sibling(struct
> task_struct *p, int prev, int target)
> * essentially a sync wakeup. An obvious example of this
> * pattern is IO completions.
> */
> - if (is_per_cpu_kthread(current) &&
> + if (in_task() &&
> + is_per_cpu_kthread(current) &&
> prev == smp_processor_id() &&
> this_rq()->nr_running <= 1) {
> +
> + WARN(is_idle_task(current), "idle per cpu kthread: cpu
> %d task: %s", prev, p->comm);
> return prev;
> }
>
>
> Without in_task() condition, i've got warnings from interrupt context
> but nothing else.
> Note that I don't even have the asym_fits_capacity() condition
I could not find a setup reproducing that issue outside of the boot time. So
following our conversation, I made a v2 that switch !is_idle_task() to in_task().
>
> >
> > Then I just added my log in the per-CPU kthread wakee stacking exit path
> >
> > printk("%s: wakee=%s:%d nr_cpus_allowed=%d current=%s:%d in_task=%d\n",
> > __func__, p->comm, p->pid, p->nr_cpus_allowed, current->comm, current->pid, in_task());
> >
> >
> > [1] https://lore.kernel.org/all/20211125101239.3248857-1-vincent.donnefort@arm.com/
> >
> >
> > From the same logs I also see:
> >
> > wakee=xfsaild/mmcblk0:4855 nr_cpus_allowed=8 current=kworker/1:1:1070 in_task=0
> >
> > Doesn't that look like a genuine wakeup that would escape the per-CPU kthread
> > stacking exit path because of the in_task test?
My bad, I checked and this is not a genuine one...
next prev parent reply other threads:[~2021-12-01 14:44 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
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 [this message]
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=20211201144005.GA479680@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).