Received-SPF: pass (google.com: domain of linux-kernel+bounces-199419-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <c3d2a49e580cea9ae86e692f72094119310adc8f.camel@linux.intel.com>
Subject: Re: [PATCH] sched/balance: Skip unnecessary updates to idle load
 balancer's flags
From: Tim Chen <tim.c.chen@linux.intel.com>
To: Chen Yu <yu.c.chen@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
 Vincent Guittot <vincent.guittot@linaro.org>, linux-kernel@vger.kernel.org,
 Vinicius Gomes <vinicius.gomes@intel.com>
Date: Mon, 03 Jun 2024 09:13:47 -0700
In-Reply-To: <Zlygeqy+SVs1KZYW@chenyu5-mobl2>
References: <20240531205452.65781-1-tim.c.chen@linux.intel.com>
	 <Zlygeqy+SVs1KZYW@chenyu5-mobl2>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-3.fc36) 
Precedence: bulk
MIME-Version: 1.0

On Mon, 2024-06-03 at 00:40 +0800, Chen Yu wrote:
> >=20
> > With instrumentation, we found that 81% of the updates do not result in
> > any change in the ilb_cpu's flags.  That is, multiple cpus are asking
> > the ilb_cpu to do the same things over and over again, before the ilb_c=
pu
> > has a chance to run NOHZ load balance.
> >=20
> > Skip updates to ilb_cpu's flags if no new work needs to be done.
> > Such updates do not change ilb_cpu's NOHZ flags.  This requires an extr=
a
> > atomic read but it is less expensive than frequent unnecessary atomic
> > updates that generate cache bounces.
>=20
> A race condition is that many CPUs choose the same ilb_cpu and ask it to =
trigger
> the nohz idle balance. This is because find_new_ilb() always finds the fi=
rst
> nohz idle CPU. I wonder if we could change the
> for_each_cpu_and(ilb_cpu, nohz.idle_cpus_mask, hk_mask)
> into
> for_each_cpu_wrap(ilb_cpu,  cpumask_and(nohz.idle_cpus_mask, hk_mask), th=
is_cpu+1)=20
> so different ilb_cpu might be found by different CPUs.
> Then the extra atomic read could brings less cache bounces.
>=20

Your proposal improves scaling.  However,
that could result in many idle CPUs getting kicked.  I assume that
current approach of delegating to a common idle CPU will disturb fewer CPUs
and let them stay in deeper idle states, and get the power benefits
from NOHZ scheme.

> >=20
> > We saw that on the OLTP workload, cpu cycles from trigger_load_balance(=
)
> > (or sched_balance_trigger()) got reduced from 0.7% to 0.2%.
> >=20
> > Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> > ---
> >  kernel/sched/fair.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >=20
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 8a5b1ae0aa55..9ab6dff6d8ac 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -11891,6 +11891,13 @@ static void kick_ilb(unsigned int flags)
> >  	if (ilb_cpu < 0)
> >  		return;
> > =20
> > +	/*
> > +	 * Don't bother if no new NOHZ balance work items for ilb_cpu,
> > +	 * i.e. all bits in flags are already set in ilb_cpu.
> > +	 */
> > +	if ((atomic_read(nohz_flags(ilb_cpu)) & flags) =3D=3D flags)
>=20
> Maybe also mention in the comment that when above statement is true, the
> current ilb_cpu's flags is not 0 and in NOHZ_KICK_MASK, so return directl=
y
> here is safe(anyway just 2 cents)

Not sure I follow your comments about return being safe.  Let me explain
in details.

We will return directly if and only if the bits set in flags are also set
in nohz_flags(ilb_cpu). =20

The comment's intention is to say that if the above statement is true, then
the later operation of=20

	atomic_fetch_or(flags, nohz_flags(ilb_cpu))

will be useless and not result in any change to nohz_flags(ilb_cpu), since =
all the set bits
in flags are already set in nohz_flags(ilb_cpu).

So will it be clearer if I say

	/*
	 * Don't bother if no new NOHZ balance work items for ilb_cpu,
	 * i.e. all bits in flags are already set in ilb_cpu.
	 * Later OR of flags to nohz_flags(ilb_cpu)
	 * will not change nohz_flags(ilb_cpu).
	 */

Thanks.


Tim

> Reviewed-by: Chen Yu <yu.c.chen@intel.com>
>=20
> thanks,
> Chenyu
>=20
> > +		return;
> > +
> >  	/*
> >  	 * Access to rq::nohz_csd is serialized by NOHZ_KICK_MASK; he who set=
s
> >  	 * the first flag owns it; cleared by nohz_csd_func().
> > --=20
> > 2.32.0
> >=20