Re: [PATCH] arm64: Utilize for_each_cpu_wrap for reference lookup

[Beata Michalska <beata.michalska@arm.com>]

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@arm.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