Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Tue, 10 Oct 2023 19:32:05 +0000
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     Ingo Molnar <mingo@kernel.org>, linux-kernel@vger.kernel.org,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        Vineeth Pillai <vineethrp@google.com>,
        Suleiman Souhlal <suleiman@google.com>,
        Hsin Yi <hsinyi@google.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        "Paul E . McKenney" <paulmck@kernel.org>
Subject: Re: [PATCH RFC] sched/fair: Avoid unnecessary IPIs for ILB
Message-ID: <20231010193205.GA4011084@google.com>
References: <20231005161727.1855004-1-joel@joelfernandes.org>
 <ZR/mvd8Uw8PG+jx0@gmail.com>
 <20231008173535.GD2338308@google.com>
 <CAKfTPtAmoG-QRs0uJJoe1Y0+MFOzH-yrqTFb6Jdrnjyo-N98BQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAKfTPtAmoG-QRs0uJJoe1Y0+MFOzH-yrqTFb6Jdrnjyo-N98BQ@mail.gmail.com>
Precedence: bulk


On Tue, Oct 10, 2023 at 3:15 AM Vincent Guittot <vincent.guittot@linaro.org> wrote:
>
> On Sun, 8 Oct 2023 at 19:35, Joel Fernandes <joel@joelfernandes.org> wrote:
[...]
> > One thing I am confused about in the original code is:
> >
> > tick_nohz_idle_stop_tick() is what sets the nohz.idle_cpus_mask.
> > However, nohz_run_idle_balance() is called before that can happen, in
> > do_idle(). So it is possible that NOHZ_NEWILB_KICK is set for a CPU but it is
> > not yet in the mask.
>
> 2 things:
> - update of nohz.idle_cpus_mask is not strictly aligned with cpu
> entering/exiting idle state. It is set when entering but only cleared
> during the next tick on the cpu because nohz.idle_cpus_mask is a
> bottleneck when all CPUs enter/exit idle at high rate (usec). This
> implies that a cpu entering idle can already be set in
> nohz.idle_cpus_mask
> - NOHZ_NEWILB_KICK is more about updating blocked load of others
> already idle CPUs than the one entering idle which has already updated
> its blocked load in newidle_balance()
>
> The goal of nohz_run_idle_balance() is to run ILB only for updating
> the blocked load of already idle CPUs without waking up one of those
> idle CPUs and outside the preempt disable / irq off phase of the local
> cpu about to enter idle because it can take a long time.

This makes complete sense, thank you for the background on this!

Vineeth was just telling me in a 1:1 that he also tried doing the removal of
the CPU from the idle mask in the restart-tick path. The result was that even
though the mask modification is not as often as when doing it during the CPU
coming out of idle, it is still higher than just doing it from the next busy
tick, like in current mainline.

As next steps we are looking into:
1. Monitor how often we set NEXT_KICK -- we think we can reduce the frequency
of these even more and keep the overhead low.

2. Look more into the parallelism of nohz.next_balance updates (due to our
next NEXT_KICK setting) and handle any race conditions. We are at the moment
looking into if nohz.next_balance does not get set to the earliest value
because of a race, and if so retry the operation.

Something like (untested):

if (likely(update_next_balance)) {
  do {
	WRITE_ONCE(nohz.next_balance, next_balance);
	if (likely(READ_ONCE(nohz.next_balance) <= next_balance)) {
		break;
	}
	cpu_relax();
  }
}

Or a try_cmpxchg loop.

I think after these items and a bit more testing, we should be good to send v2.

Thanks.