| Message ID | 20250218192412.2072619-1-beata.michalska@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Utilize for_each_cpu_wrap for reference lookup | expand |
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. > 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; > + } > + if (!idle_cpu(ref_cpu)) > + break; > + } > > cpufreq_cpu_put(policy); > > -- > 2.25.1
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
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 (!idle_cpu(ref_cpu)) + break; + } cpufreq_cpu_put(policy);
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. 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(-)