Received-SPF: pass (google.com: domain of linux-kernel+bounces-199959-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 4 Jun 2024 10:10:29 +0800
From: Chen Yu <yu.c.chen@intel.com>
To: Tim Chen <tim.c.chen@linux.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>
Subject: Re: [PATCH] sched/balance: Skip unnecessary updates to idle load
 balancer's flags
Message-ID: <Zl53lXjEmnWhlW8p@chenyu5-mobl2>
References: <20240531205452.65781-1-tim.c.chen@linux.intel.com>
 <Zlygeqy+SVs1KZYW@chenyu5-mobl2>
 <c3d2a49e580cea9ae86e692f72094119310adc8f.camel@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <c3d2a49e580cea9ae86e692f72094119310adc8f.camel@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?DyeCDPtRS4jsGe3Ch62PO/DtIc+WnF7TuCVHizl2yNLhZ40P8xzJHHHfGr1I?=
 =?us-ascii?Q?GjHAbnTOXVjZNuP4ZrGT1b+Kf4/UjOCWuObf24meQEHBqOaSSo6E6Js0T8mR?=
 =?us-ascii?Q?MJOzEMIkKqi4v9QmyPu1gBBwnNGZa1YKJAnWgcHz6wBxbXf2SDkyDBKcW3Aw?=
 =?us-ascii?Q?hy/JevW1d2JzalcUicLpO460QWwZRo2UPOPfXS/md4pUFhvtiXZ9uLzbiZP9?=
 =?us-ascii?Q?wogCCHxxVptfJPvUyJSJ+nkFlc5R3cSAPm9QM2qwc+Qzru6ZeGQ3Kle1pzO6?=
 =?us-ascii?Q?wcCG1sI0gPI1iPhYMuJ1IIlSWRcZAtk4ZCQGLrR4QUIwUcwmFNRZKUJj7m2r?=
 =?us-ascii?Q?BLmWhC+j9Dt0OVC9XdOdOJYOzKlhZMQwBTb6HYSoEWgtVHw+yNi+SiAIUcpO?=
 =?us-ascii?Q?I7ElSzTaONd7pPFscJLO56joyKW2f9yRVXSdgSrXAYuWNvURdYac+QZF0zji?=
 =?us-ascii?Q?l2cezP3xEvWUPQox9+m6D0TLmclYUtd4IDr4k7uYuIyj6AeAB8yihatsLxIL?=
 =?us-ascii?Q?QoOV9d83dJ/YsmI9VyPEKcFtaXQMRp17QxWgiLU0kSgBYOFwj2nVwDgNnmoD?=
 =?us-ascii?Q?X6cV2CH+lQ3YzW266jdQYXC7eglNBMU/Qm/qgw2b0xagQPWMl2UummLn2OVT?=
 =?us-ascii?Q?5e2uAzXGV0RzKlFWdrgHsVMgXIF6Fm1H8+5etDdIoeUvoGsuUzic0L/BTuIb?=
 =?us-ascii?Q?ZvI90R/hx1/pUAfu/M1PoX7bMrwJFdVX6rmVWp3+kTpQCEI6dIESHZje/KG9?=
 =?us-ascii?Q?b7VuYbIJeV6TDvCcyeOGnRUwMqq/ZdvngdtdulehfbQcgU1QPxZQW6JJK26D?=
 =?us-ascii?Q?PSjVon1Ru7KHpjK2HDkQqlFAiWrNj6ZqvmJZRUUEdgzMzrv1no7V2taCwguh?=
 =?us-ascii?Q?JEaWjkx6jWZFBugLf10WeNxd2RatxQEHQmX0nAPHU2CtiGGtubBg8gkK5VCU?=
 =?us-ascii?Q?wLlaEs2VvvOXMJ9d0fUMRwh/GgIpPpgYwbIcEmzYOppkpWmKRgScsdOBCibI?=
 =?us-ascii?Q?Y6oN0rVK/ZO0kaCC7nfg1ss9B5jBwp6X25UlgBR442+PtqAqdz8LPcY5qnIl?=
 =?us-ascii?Q?zgyYR4b5eTsLtzLILn8XEegR4dwwcWc0D5xmGaphksZzqBgrj4iOj5TZJucg?=
 =?us-ascii?Q?kBXQC85FEuffIY/fB3qNJjy3zdwlH+8S7q6WK+IgFeKSvcXBKEbr4iK3c3VR?=
 =?us-ascii?Q?wjRgUHRDx7rnVdcn/3xPCcT6TsdjYcxmN5uAAUe4UDGzHO27STySgS/fmiGM?=
 =?us-ascii?Q?lHsyX7ZViX36qccuAglT2zJe+ynmEmBppxZXMpGBktFduN3z4BmvddjSpJAE?=
 =?us-ascii?Q?lRsnRoWqpja0q2yWkvvu0GWobpzwf5ltSNb05H5t13kcyplc+uihIEgO+TNb?=
 =?us-ascii?Q?vKRrhd3fa0C9iY1Iqx5ymFjo9MpsvKsOX1OSeRD/mLoiuXxlLNinfRnig5yK?=
 =?us-ascii?Q?vOZ0HI3izwH/Z6yJ9aQ5f+iapr0jUziE/HLskX6wpleIadoYtjz8tvNvy0Dv?=
 =?us-ascii?Q?5y/LlplwKoFaXEkIOyL9oGqJNIQFzQW4qfWAP6D7zl7WoUYrb8Ut86KRQHjp?=
 =?us-ascii?Q?hLj5KsPdKKlHnBQadDmssTZ4RM32oaXKejf0GZXO?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6a57708d-4630-4fc4-4d47-08dc843b8839
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB6005.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jun 2024 02:10:40.4203
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: k/sPYAI2l3i33szhqITXv1Wtggjv84BjOM0mwNmDypJiYu/rJKS+81FaVLMr29gBODi5GJDrRUqn+qFoOMJrSA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4595
X-OriginatorOrg: intel.com

On 2024-06-03 at 09:13:47 -0700, Tim Chen wrote:
> On Mon, 2024-06-03 at 00:40 +0800, Chen Yu wrote:
> > > 
> > > 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_cpu
> > > has a chance to run NOHZ load balance.
> > > 
> > > 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 extra
> > > atomic read but it is less expensive than frequent unnecessary atomic
> > > updates that generate cache bounces.
> > 
> > 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 first
> > 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), this_cpu+1) 
> > so different ilb_cpu might be found by different CPUs.
> > Then the extra atomic read could brings less cache bounces.
> > 
> 
> 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.
>

I see, from power point of view, current solution is better.
 
> > > 
> > > 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%.
> > > 
> > > Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> > > ---
> > >  kernel/sched/fair.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > 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;
> > >  
> > > +	/*
> > > +	 * 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) == flags)
> > 
> > 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 directly
> > 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).  
> 
> The comment's intention is to say that if the above statement is true, then
> the later operation of 
> 
> 	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).

Understand. My previous thought was that, what if the current nohz_flags(ilb_cpu) is 0 or
NOHZ_NEWILB_KICK. If yes, return directly might miss one ipi to the ilb_cpu(because
the current code checks flags & NOHZ_KICK_MASK to return directly). But from the current
logic when we reach kick_ilb(), the flag is not 0, and the flag passed by nohz_balancer_kick()
satisfy (flags & NOHZ_KICK_MASK), so returns here is ok.

> 
> 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).
> 	 */
>

Yes, this is ok.


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