Received-SPF: pass (google.com: domain of linux-kernel+bounces-31713-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 20 Jan 2024 14:17:38 +0800
From: Ming Lei <ming.lei@redhat.com>
To: Yury Norov <yury.norov@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Breno Leitao <leitao@debian.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Zi Yan <ziy@nvidia.com>, ming.lei@redhat.com
Subject: Re: [PATCH 4/9] lib/group_cpus: optimize outer loop in
 grp_spread_init_one()
Message-ID: <ZatlggW/8SH6od9O@fedora>
References: <20240120025053.684838-1-yury.norov@gmail.com>
 <20240120025053.684838-5-yury.norov@gmail.com>
 <ZatDXvhvt0mLTi2m@fedora>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZatDXvhvt0mLTi2m@fedora>

On Sat, Jan 20, 2024 at 11:51:58AM +0800, Ming Lei wrote:
> On Fri, Jan 19, 2024 at 06:50:48PM -0800, Yury Norov wrote:
> > Similarly to the inner loop, in the outer loop we can use for_each_cpu()
> > macro, and skip CPUs that have been moved.
> > 
> > With this patch, the function becomes O(1), despite that it's a
> > double-loop.
> > 
> > While here, add a comment why we can't merge outer logic into the inner
> > loop.
> > 
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > ---
> >  lib/group_cpus.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/lib/group_cpus.c b/lib/group_cpus.c
> > index 0a8ac7cb1a5d..952aac9eaa81 100644
> > --- a/lib/group_cpus.c
> > +++ b/lib/group_cpus.c
> > @@ -17,16 +17,17 @@ static void grp_spread_init_one(struct cpumask *irqmsk, struct cpumask *nmsk,
> >  	const struct cpumask *siblmsk;
> >  	int cpu, sibl;
> >  
> > -	for ( ; cpus_per_grp > 0; ) {
> > -		cpu = cpumask_first(nmsk);
> > -
> > -		/* Should not happen, but I'm too lazy to think about it */
> > -		if (cpu >= nr_cpu_ids)
> > +	for_each_cpu(cpu, nmsk) {
> > +		if (cpus_per_grp-- == 0)
> >  			return;
> >  
> > +		/*
> > +		 * If a caller wants to spread IRQa on offline CPUs, we need to
> > +		 * take care of it explicitly because those offline CPUS are not
> > +		 * included in siblings cpumask.
> > +		 */
> >  		__cpumask_clear_cpu(cpu, nmsk);
> >  		__cpumask_set_cpu(cpu, irqmsk);
> > -		cpus_per_grp--;
> >  
> >  		/* If the cpu has siblings, use them first */
> >  		siblmsk = topology_sibling_cpumask(cpu);
> > @@ -38,6 +39,7 @@ static void grp_spread_init_one(struct cpumask *irqmsk, struct cpumask *nmsk,
> >  
> >  			__cpumask_clear_cpu(sibl, nmsk);
> >  			__cpumask_set_cpu(sibl, irqmsk);
> > +			cpu = sibl + 1;
> 
> It has been tricky enough to update condition variable of for_each_cpu()
> (such kind of pattern can't build in Rust at all), and the above line could
> be more tricky actually.

Not only the above line is tricky, but also it is wrong, because 'cpu'
local variable should always point to the 1st bit in 'nmsk'. However, if
you set it to 'sibl + 1', some bits in 'nmsk' are skipped in the loop,
aren't they?


Thanks,
Ming