Received-SPF: pass (google.com: domain of linux-kernel+bounces-34863-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240119084548.2788-1-kprateek.nayak@amd.com> <b4f5ac150685456cf45a342e3bb1f28cdd557a53.camel@linux.intel.com>
 <21c8694c-26e4-3bc1-edd8-2267b0164a09@amd.com>
In-Reply-To: <21c8694c-26e4-3bc1-edd8-2267b0164a09@amd.com>
From: Vincent Guittot <vincent.guittot@linaro.org>
Date: Tue, 23 Jan 2024 09:06:13 +0100
Message-ID: <CAKfTPtCFJ5TRdsHHiH_fz9R2TC3euz_Rp=LH+aQ9KeZx3uH+ZQ@mail.gmail.com>
Subject: Re: [PATCH] sched/fair: Skip newidle_balance() when an idle CPU is
 woken up to process an IPI
To: K Prateek Nayak <kprateek.nayak@amd.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>, linux-kernel@vger.kernel.org, 
	mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com, 
	dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com, 
	mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com, 
	gautham.shenoy@amd.com, David Vernet <void@manifault.com>
Content-Type: text/plain; charset="UTF-8"

On Tue, 23 Jan 2024 at 05:58, K Prateek Nayak <kprateek.nayak@amd.com> wrote:
>
> Hello Tim,
>
> On 1/23/2024 3:29 AM, Tim Chen wrote:
> > On Fri, 2024-01-19 at 14:15 +0530, K Prateek Nayak wrote:
> >>
> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >> index b803030c3a03..1fedc7e29c98 100644
> >> --- a/kernel/sched/fair.c
> >> +++ b/kernel/sched/fair.c
> >> @@ -8499,6 +8499,16 @@ done: __maybe_unused;
> >>      if (!rf)
> >>              return NULL;
> >>
> >> +    /*
> >> +     * An idle CPU in TIF_POLLING mode might end up here after processing
> >> +     * an IPI when the sender sets the TIF_NEED_RESCHED bit and avoids
> >> +     * sending an actual IPI. In such cases, where an idle CPU was woken
> >> +     * up only to process an interrupt, without necessarily queuing a task
> >> +     * on it, skip newidle_balance() to facilitate faster idle re-entry.
> >> +     */
> >> +    if (prev == rq->idle)
> >> +            return NULL;
> >> +
> >
> > Should we check the call function queue directly to detect that there is
> > an IPI waiting to be processed? something like
> >
> >       if (!llist_empty(&per_cpu(call_single_queue, rq->cpu)))
> >               return NULL;
>
> That could be a valid check too. However, if an IPI is queued right
> after this check, the processing is still delayed since
> newidle_balance() only bails out for scenarios when a wakeup is trying
> to queue a new task on the CPU running the newidle_balance().
>
> >
> > Could there be cases where we want to do idle balance in this code path?
> > Say a cpu is idle and a scheduling tick came in, we may try
> > to look for something to run on the idle cpu.  Seems like after
> > your change above, that would be skipped.
>
> Wouldn't scheduler_tick() do load balancing when the time comes? In my
> testing, I did not see a case where the workloads I tested were
> sensitive to the aspect of newidle_balance() being invoked at scheduler
> tick. Have you come across a workload which might be sensitive to this
> aspect that I can quickly test and verify? Meanwhile, I'll run the
> workloads mentioned in the commit log on an Intel system to see if I
> can spot any sensitivity to this change.

Instead of trying to fix spurious need_resched in the scheduler,
can't we find a way to prevent it from happening ?

Because of TIF_NEED_RESCHED being set when TIF_POLLING is set, idle
load balances are already skipped for a less aggressive newly idle
load balanced:
https://lore.kernel.org/all/CAKfTPtC9Px_W84YRJqnFNkL8oofO15D-P=VTCMUUu7NJr+xwBA@mail.gmail.com/

The root of the problem is that we keep TIF_NEED_RESCHED set

>
> Adding David to the thread too since HHVM seems to be one of those
> workloads that is very sensitive to a successful newidle_balance().
>
> >
> > Tim
> >
> >
> >>      new_tasks = newidle_balance(rq, rf);
> >>
> >>      /*
> >
>
> --
> Thanks and Regards,
> Prateek