[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230608051509.h4a6gn572mjgdusv@vireshk-i7>
Date: Thu, 8 Jun 2023 10:45:09 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Beata Michalska <beata.michalska@....com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, catalin.marinas@....com,
mark.rutland@....com, will@...nel.org, rafael@...nel.org,
sudeep.holla@....com, ionela.voinescu@....com, sumitg@...dia.com,
yang@...amperecomputing.com, Len Brown <len.brown@...el.com>
Subject: Re: [PATCH] arm64: Provide an AMU-based version of
arch_freq_get_on_cpu
+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")
Powered by blists - more mailing lists