From:   Valentin Schneider <Valentin.Schneider@arm.com>
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     Vincent Donnefort <Vincent.Donnefort@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
In-Reply-To: <CAKfTPtC4iXXaptm9+2bHvX2E3xAWU4M3xN0ZuwpFQ1RyXAyxyA@mail.gmail.com>
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>
Date:   Fri, 26 Nov 2021 16:49:12 +0000
Message-ID: <87pmqmc16f.mognet@arm.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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.

>
>> +           in_task() &&
>>             prev == smp_processor_id() &&
>>             this_rq()->nr_running <= 1) {
>>                 return prev;
>>