[PATCH v3] cpufreq: CPPC: Update FIE arch_freq_scale in ticks for non-PCC regs
Jie Zhan
zhanjie9 at hisilicon.com
Tue Nov 11 03:30:09 PST 2025
On 11/11/2025 12:49 AM, Beata Michalska wrote:
> Hi Jie,
> On Tue, Nov 04, 2025 at 02:50:39PM +0800, Jie Zhan wrote:
>> Currently, the CPPC Frequency Invariance Engine (FIE) is invoked from the
>> scheduler tick but defers the update of arch_freq_scale to a separate
>> thread because cppc_get_perf_ctrs() would sleep if the CPC regs are in PCC.
>>
>> However, this deferred update mechanism is unnecessary and introduces extra
>> overhead for non-PCC register spaces (e.g. System Memory or FFH), where
>> accessing the regs won't sleep and can be safely performed from the tick
>> context.
>>
>> Furthermore, with the CPPC FIE registered, it throws repeated warnings of
>> "cppc_scale_freq_workfn: failed to read perf counters" on our platform with
>> the CPC regs in System Memory and a power-down idle state enabled. That's
>> because the remote CPU can be in a power-down idle state, and reading its
>> perf counters returns 0. Moving the FIE handling back to the scheduler
>> tick process makes the CPU handle its own perf counters, so it won't be
>> idle and the issue would be inherently solved.
>>
>> To address the above issues, update arch_freq_scale directly in ticks for
>> non-PCC regs and keep the deferred update mechanism for PCC regs.
> Something about it just didn’t sit right with me, and apparently, it needed some
> time to settle down - thus the delay.
>
> It all looks sensible though it might be worth to considered applying
> the change on a per-CPU basis, as, in theory at least, different address
> spaces are allowed for different registers (at least according to the ACPI
> spec, if I read it right).
> So I was thinking about smth along the lines of:
Beata,
Right, I see what you want to do.
Some comments inline.
Would you like to make it a full patch so I can include it in the next
version? or some other way?
Jie
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 6c684e54fe01..07f4e59f2f0a 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1431,38 +1431,47 @@ EXPORT_SYMBOL_GPL(cppc_get_perf_caps);
> *
> * Return: true if any of the counters are in PCC regions, false otherwise
> */
> -bool cppc_perf_ctrs_in_pcc(void)
> +bool cppc_perf_ctrs_in_pcc(unsigned int cpu)
> {
> - int cpu;
> + struct cpc_register_resource *ref_perf_reg;
> + struct cpc_desc *cpc_desc;
>
> - for_each_present_cpu(cpu) {
> - struct cpc_register_resource *ref_perf_reg;
> - struct cpc_desc *cpc_desc;
> + cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>
> - cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> + if (CPC_IN_PCC(&cpc_desc->cpc_regs[DELIVERED_CTR]) ||
> + CPC_IN_PCC(&cpc_desc->cpc_regs[REFERENCE_CTR]) ||
> + CPC_IN_PCC(&cpc_desc->cpc_regs[CTR_WRAP_TIME]))
> + return true;
>
> - if (CPC_IN_PCC(&cpc_desc->cpc_regs[DELIVERED_CTR]) ||
> - CPC_IN_PCC(&cpc_desc->cpc_regs[REFERENCE_CTR]) ||
> - CPC_IN_PCC(&cpc_desc->cpc_regs[CTR_WRAP_TIME]))
> - return true;
>
> + ref_perf_reg = &cpc_desc->cpc_regs[REFERENCE_PERF];
>
> - ref_perf_reg = &cpc_desc->cpc_regs[REFERENCE_PERF];
> + /*
> + * If reference perf register is not supported then we should
> + * use the nominal perf value
> + */
> + if (!CPC_SUPPORTED(ref_perf_reg))
> + ref_perf_reg = &cpc_desc->cpc_regs[NOMINAL_PERF];
Though not related to this issue, I'm confused that this sort of workaround
appears here - it should be in some init function.
>
> - /*
> - * If reference perf register is not supported then we should
> - * use the nominal perf value
> - */
> - if (!CPC_SUPPORTED(ref_perf_reg))
> - ref_perf_reg = &cpc_desc->cpc_regs[NOMINAL_PERF];
> + if (CPC_IN_PCC(ref_perf_reg))
> + return true;
> +
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(cppc_perf_ctrs_in_pcc);
>
> - if (CPC_IN_PCC(ref_perf_reg))
> +bool cppc_any_perf_ctrs_in_pcc(void)
> +{
> + int cpu;
> +
> + for_each_present_cpu(cpu) {
> + if (cppc_perf_ctrs_in_pcc(cpu))
> return true;
> }
>
> return false;
> }
> -EXPORT_SYMBOL_GPL(cppc_perf_ctrs_in_pcc);
> +EXPORT_SYMBOL_GPL(cppc_any_perf_ctrs_in_pcc);
>
> /**
> * cppc_get_perf_ctrs - Read a CPU's performance feedback counters.
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 4fcaec7e2034..fdf5a49c04ed 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -48,7 +48,6 @@ struct cppc_freq_invariance {
> };
>
> static DEFINE_PER_CPU(struct cppc_freq_invariance, cppc_freq_inv);
> -static bool perf_ctrs_in_pcc;
> static struct kthread_worker *kworker_fie;
>
> static int cppc_perf_from_fbctrs(struct cppc_perf_fb_ctrs *fb_ctrs_t0,
> @@ -132,7 +131,12 @@ static void cppc_scale_freq_tick_pcc(void)
>
> static void cppc_scale_freq_tick(void)
> {
> - __cppc_scale_freq_tick(&per_cpu(cppc_freq_inv, smp_processor_id()));
> + unsigned int cpu = smp_processor_id();
> +
> + cppc_perf_ctrs_in_pcc(cpu) ? cppc_scale_freq_tick_pcc()
Calling cppc_perf_ctrs_in_pcc() could be expensive here.
I'd prefer something like a static branch or a determined callback for each
cpu.
> + : __cppc_scale_freq_tick(
> + &per_cpu(cppc_freq_inv,
> + cpu));
> }
>
> static struct scale_freq_data cppc_sftd = {
> @@ -152,7 +156,7 @@ static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
> cppc_fi = &per_cpu(cppc_freq_inv, cpu);
> cppc_fi->cpu = cpu;
> cppc_fi->cpu_data = policy->driver_data;
> - if (perf_ctrs_in_pcc) {
> + if (cppc_perf_ctrs_in_pcc(cpu)) {
> kthread_init_work(&cppc_fi->work, cppc_scale_freq_workfn);
> init_irq_work(&cppc_fi->irq_work, cppc_irq_work);
> }
> @@ -193,10 +197,9 @@ static void cppc_cpufreq_cpu_fie_exit(struct cpufreq_policy *policy)
> /* policy->cpus will be empty here, use related_cpus instead */
> topology_clear_scale_freq_source(SCALE_FREQ_SOURCE_CPPC, policy->related_cpus);
>
> - if (!perf_ctrs_in_pcc)
> - return;
> -
> for_each_cpu(cpu, policy->related_cpus) {
> + if (!cppc_perf_ctrs_in_pcc(cpu))
> + continue;
> cppc_fi = &per_cpu(cppc_freq_inv, cpu);
> irq_work_sync(&cppc_fi->irq_work);
> kthread_cancel_work_sync(&cppc_fi->work);
> @@ -218,14 +221,11 @@ static void __init cppc_freq_invariance_init(void)
> .sched_deadline = 10 * NSEC_PER_MSEC,
> .sched_period = 10 * NSEC_PER_MSEC,
> };
> + bool perf_ctrs_in_pcc = cppc_any_perf_ctrs_in_pcc();
> int ret;
>
> - perf_ctrs_in_pcc = cppc_perf_ctrs_in_pcc();
> -
> if (fie_disabled != FIE_ENABLED && fie_disabled != FIE_DISABLED) {
> - if (!perf_ctrs_in_pcc) {
> - fie_disabled = FIE_ENABLED;
> - } else {
> + if (perf_ctrs_in_pcc) {
> pr_info("FIE not enabled on systems with registers in PCC\n");
> fie_disabled = FIE_DISABLED;
> }
> @@ -234,12 +234,12 @@ static void __init cppc_freq_invariance_init(void)
> if (fie_disabled || !perf_ctrs_in_pcc)
> return;
>
> - cppc_sftd.set_freq_scale = cppc_scale_freq_tick_pcc;
>
> kworker_fie = kthread_run_worker(0, "cppc_fie");
> if (IS_ERR(kworker_fie)) {
> pr_warn("%s: failed to create kworker_fie: %ld\n", __func__,
> PTR_ERR(kworker_fie));
> + kworker_fie = NULL;
> fie_disabled = FIE_DISABLED;
> return;
> }
> @@ -255,10 +255,8 @@ static void __init cppc_freq_invariance_init(void)
>
> static void cppc_freq_invariance_exit(void)
> {
> - if (fie_disabled || !perf_ctrs_in_pcc)
> - return;
> -
> - kthread_destroy_worker(kworker_fie);
> + if (kworker_fie)
> + kthread_destroy_worker(kworker_fie);
> }
>
> #else
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 13fa81504844..3af503b12f60 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -154,7 +154,8 @@ extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
> extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
> extern int cppc_set_enable(int cpu, bool enable);
> extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
> -extern bool cppc_perf_ctrs_in_pcc(void);
> +extern bool cppc_perf_ctrs_in_pcc(unsigned int cpu);
> +extern bool cppc_any_perf_ctrs_in_pcc(void);
would be slightly better to keep cppc_perf_ctrs_in_pcc(void) and add a new
function, e.g. cppc_perf_ctrs_in_pcc_cpu(unsigned int cpu), such that the
old ABI is unchanged.
> extern unsigned int cppc_perf_to_khz(struct cppc_perf_caps *caps, unsigned int perf);
> extern unsigned int cppc_khz_to_perf(struct cppc_perf_caps *caps, unsigned int freq);
> extern bool acpi_cpc_valid(void);
> @@ -204,7 +205,11 @@ static inline int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps)
> {
> return -EOPNOTSUPP;
> }
> -static inline bool cppc_perf_ctrs_in_pcc(void)
> +static inline bool cppc_perf_ctrs_in_pcc(unsigned int cpu)
> +{
> + return false;
> +}
> +static inline bool cppc_any_perf_ctrs_in_pcc(void)
> {
> return false;
> }
>
>
> Additionally, it might be worth to get rid of (at least) some messages printed
> on the path of reading the counters in case it is being done in tick context.
Cool, will have a look.
>
> Also , I do not have access to any machine using PCC, and it would be good to
> double check that as well.
>
> ---
> BR
> Beata
>>
>> Signed-off-by: Jie Zhan <zhanjie9 at hisilicon.com>
>> ---
>> We have tested this on Kunpeng SoCs with the CPC regs both in System Memory
>> and FFH. More tests on other platforms are welcome.
>>
>> Changelog:
>>
>> v3:
>> - Stash the state of 'cppc_perf_ctrs_in_pcc' so it won't have to check the CPC
>> regs of all CPUs everywhere (Thanks to the suggestion from Beata Michalska).
>> - Update the commit log, explaining more on the warning issue caused by
>> accessing perf counters on remote CPUs.
>> - Drop Patch 1 that has been accepted, and rebase Patch 2 on that.
>>
>> v2:
>> https://lore.kernel.org/linux-pm/20250828110212.2108653-1-zhanjie9@hisilicon.com/
>> - Update the cover letter and the commit log based on v1 discussion
>> - Update FIE arch_freq_scale in ticks for non-PCC regs
>>
>> v1:
>> https://lore.kernel.org/linux-pm/20250730032312.167062-1-yubowen8@huawei.com/
>> ---
>> drivers/cpufreq/cppc_cpufreq.c | 60 ++++++++++++++++++++++++----------
>> 1 file changed, 42 insertions(+), 18 deletions(-)
...
More information about the linux-arm-kernel
mailing list