MIME-Version: 1.0
References: <20211124154239.3191366-1-vincent.donnefort@arm.com>
 <CAKfTPtDX8sOfguZhJt5QV3j5D_JetcgncuF2w+uLa0XDk7UXkw@mail.gmail.com>
 <8735nkcwov.mognet@arm.com> <CAKfTPtDPskVdEd-KQ_cwe-R_zVFPQOgdbk9x+3eD12pKs8fGFw@mail.gmail.com>
 <87zgpsb6de.mognet@arm.com> <CAKfTPtCnusWJXJLDEudQ_q8MWaZYbPJK-QjAbBYWFW8Nw-J+Ww@mail.gmail.com>
 <87sfvjavqk.mognet@arm.com> <CAKfTPtC4iXXaptm9+2bHvX2E3xAWU4M3xN0ZuwpFQ1RyXAyxyA@mail.gmail.com>
 <87pmqmc16f.mognet@arm.com> <20211126171817.GA3798214@ubiquitous>
In-Reply-To: <20211126171817.GA3798214@ubiquitous>
From:   Vincent Guittot <vincent.guittot@linaro.org>
Date:   Mon, 29 Nov 2021 16:49:58 +0100
Message-ID: <CAKfTPtCGyp8JZq1EOgEhTeD+PBV2rMnTQ=uV-ZgsaN1RVmPk0w@mail.gmail.com>
Subject: Re: [PATCH] sched/fair: Fix detection of per-CPU kthreads waking a task
To:     Vincent Donnefort <Vincent.Donnefort@arm.com>
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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 26 Nov 2021 at 18:18, Vincent Donnefort
<vincent.donnefort@arm.com> wrote:
>
> 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

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 ?

Can you see similar things later after booting ?

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 ?


> [    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;
> > >>