Re: [PATCH] arm64: Provide an AMU-based version of arch_freq_get_on_cpu

[Viresh Kumar <viresh.kumar@linaro.org>]

+Vincent

On 08-06-23, 10:45, Viresh Kumar wrote:
> +Len
> 
> On 06-06-23, 16:57, Beata Michalska wrote:
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > +unsigned int arch_freq_get_on_cpu(int cpu)
> > +{
> > +	unsigned int freq;
> > +	u64 scale;
> > +
> > +	if (!cpumask_test_cpu(cpu, amu_fie_cpus))
> > +		return 0;
> > +
> > +	if (!housekeeping_cpu(cpu, HK_TYPE_TICK)) {
> 
> I am not sure what we are doing in the `if` block here, at least a comment would
> be useful.
> 
> > +		struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> > +		int ref_cpu = nr_cpu_ids;
> > +
> > +		if (cpumask_intersects(housekeeping_cpumask(HK_TYPE_TICK),
> > +				       policy->cpus))
> > +			ref_cpu = cpumask_nth_and(cpu, policy->cpus,
> > +						  housekeeping_cpumask(HK_TYPE_TICK));
> > +		cpufreq_cpu_put(policy);
> > +		if (ref_cpu >= nr_cpu_ids)
> > +			return 0;
> > +		cpu = ref_cpu;
> > +	}
> 
> A blank line here please.
> 
> > +	/*
> > +	 * Reversed computation to the one used to determine
> > +	 * the arch_freq_scale value
> > +	 * (see amu_scale_freq_tick for details)
> > +	 */
> > +	scale = per_cpu(arch_freq_scale, cpu);
> > +	scale *= cpufreq_get_hw_max_freq(cpu);
> > +	freq = scale >> SCHED_CAPACITY_SHIFT;
> > +
> > +	return freq;
> > +}
> > +
> >  #ifdef CONFIG_ACPI_CPPC_LIB
> >  #include <acpi/cppc_acpi.h>
> >  
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 6b52ebe5a890..9f2cf45bf190 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -710,7 +710,8 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
> >  	ssize_t ret;
> >  	unsigned int freq;
> >  
> > -	freq = arch_freq_get_on_cpu(policy->cpu);
> > +	freq = !cpufreq_driver->get ? arch_freq_get_on_cpu(policy->cpu)
> > +				    : 0;
> 
> You may have changed the logic for X86 parts as well here. For a x86 platform
> with setpolicy() and get() callbacks, we will not call arch_freq_get_on_cpu()
> anymore ?
> 
> >  	if (freq)
> >  		ret = sprintf(buf, "%u\n", freq);
> >  	else if (cpufreq_driver->setpolicy && cpufreq_driver->get)
> > @@ -747,7 +748,11 @@ store_one(scaling_max_freq, max);
> >  static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
> >  					char *buf)
> >  {
> > -	unsigned int cur_freq = __cpufreq_get(policy);
> > +	unsigned int cur_freq;
> > +
> > +	cur_freq = arch_freq_get_on_cpu(policy->cpu);
> > +	if (!cur_freq)
> > +		cur_freq = __cpufreq_get(policy);
> 
> For this and the above change, I am not sure what is the right thing to do.
> 
> >From Len's commit [1]:
> 
>     Here we provide an x86 routine to make this calculation
>     on supported hardware, and use it in preference to any
>     driver driver-specific cpufreq_driver.get() routine.
> 
> I am not sure why Len updated `show_scaling_cur_freq()` and not
> `show_cpuinfo_cur_freq()` ? Maybe we should update both these routines ?
> 
> Also, I don't think this is something that should have different logic for ARM
> and X86, we should be consistent here as a cpufreq decision. Since both these
> routines are reached via a read operation to a sysfs file, we shouldn't be
> concerned about performance too.
> 
> What about doing this for both the routines, for all platforms now:
> 
> 	cur_freq = arch_freq_get_on_cpu(policy->cpu);
> 	if (!cur_freq)
>                 ... get freq via policy->get() or policy->cur;
> 
> -- 
> viresh
> 
> [1] commit f8475cef9008 ("x86: use common aperfmperf_khz_on_cpu() to calculate KHz using APERF/MPERF")

-- 
viresh