Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 10:48:16 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     Beata Michalska <beata.michalska@arm.com>
Cc:     linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com,
        mark.rutland@arm.com, will@kernel.org, rafael@kernel.org,
        sudeep.holla@arm.com, ionela.voinescu@arm.com, sumitg@nvidia.com,
        yang@os.amperecomputing.com, Len Brown <len.brown@intel.com>,
        vincent.guittot@linaro.org
Subject: Re: [PATCH] arm64: Provide an AMU-based version of
 arch_freq_get_on_cpu
Message-ID: <20230608051816.2ww7ncg65qo7kcuk@vireshk-i7>
References: <20230606155754.245998-1-beata.michalska@arm.com>
 <20230608051509.h4a6gn572mjgdusv@vireshk-i7>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230608051509.h4a6gn572mjgdusv@vireshk-i7>
Precedence: bulk

+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