linux-kernel - Re: [PATCH] arm64: Utilize for_each_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z7WULBZPsh8QK3DV@arm.com>
Date: Wed, 19 Feb 2025 09:19:56 +0100
From: Beata Michalska <beata.michalska@....com>
To: Yury Norov <yury.norov@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	sudeep.holla@....com, will@...nel.org, catalin.marinas@....com,
	sfr@...b.auug.org.au, ionela.voinescu@....com,
	linux-next@...r.kernel.org, sumitg@...dia.com,
	yang@...amperecomputing.com, vanshikonda@...amperecomputing.com,
	lihuisong@...wei.com, zhanjie9@...ilicon.com,
	ptsm@...ux.microsoft.com
Subject: Re: [PATCH] arm64: Utilize for_each_cpu_wrap for reference lookup

On Tue, Feb 18, 2025 at 03:14:23PM -0500, Yury Norov wrote:
> On Tue, Feb 18, 2025 at 07:24:12PM +0000, Beata Michalska wrote:
> > While searching for a reference CPU within a given policy,
> > arch_freq_get_on_cpu relies on cpumask_next_wrap to iterate over
> > all available CPUs and to ensure each is verified only once.
> > Recent changes to cpumask_next_wrap will handle the latter no more,
> > so switching to for_each_cpu_wrap, which  preserves expected behavior
> > while ensuring compatibility with the updates.
> 
> This is technically correct, but I would rather point that for
> iterating over each CPU, it's better to use a dedicated iterator
> instead of opencoded loop.

I can certainly add that.
> 
> > Fixes: 16d1e27475f6 ("arm64: Provide an AMU-based version of arch_freq_get_on_cpu")
> > Signed-off-by: Beata Michalska <beata.michalska@....com>
> > ---
> >  based on arm64 for-next/amuv1-avg-freq
> > 
> >  arch/arm64/kernel/topology.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index a09b0551ec59..9e3583720668 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -254,7 +254,7 @@ int arch_freq_get_on_cpu(int cpu)
> >  		if (!housekeeping_cpu(cpu, HK_TYPE_TICK) ||
> >  		    time_is_before_jiffies(last_update + msecs_to_jiffies(AMU_SAMPLE_EXP_MS))) {
> >  			struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> > -			int ref_cpu = cpu;
> > +			int ref_cpu;
> >  
> >  			if (!policy)
> >  				return -EINVAL;
> > @@ -265,11 +265,15 @@ int arch_freq_get_on_cpu(int cpu)
> >  				return -EOPNOTSUPP;
> >  			}
> >  
> > -			do {
> > -				ref_cpu = cpumask_next_wrap(ref_cpu, policy->cpus,
> > -							    start_cpu, true);
> > -
> > -			} while (ref_cpu < nr_cpu_ids && idle_cpu(ref_cpu));
> > +			for_each_cpu_wrap(ref_cpu, policy->cpus, cpu + 1) {
> > +				if (ref_cpu == start_cpu) {
> > +					/* Prevent verifying same CPU twice */
> > +					ref_cpu = nr_cpu_ids;
> > +					break;
> 
> If start_cpu == cpu, and you begin with 'cpu + 1', you will never
> check the 'cpu' for idle, right? Maybe like this?
> 
>         unsigned int start_cpu = cpu + 1;
> 
This is not entirely a pure for-each case here.
If this loop is reached, it means the start_cpu did not meet the criteria, and
we are trying to find another CPU within the policy that might. Which is why we
pick up the next in line and check whether it is suitable or not.
Testing for idle is just a shortcut, as an idle CPU will most probably not be
considered a good reference either way.

---
BR
Beata
> > +				}
> > +				if (!idle_cpu(ref_cpu))
> > +					break;
> > +			}
> >  
> >  			cpufreq_cpu_put(policy);
> >  
> > -- 
> > 2.25.1