[PATCH v2 0/2] CPPC: reduce FFH feedback-counter sampling skew on arm64

[PATCH v2 0/2] CPPC: reduce FFH feedback-counter sampling skew on arm64

[Pengjie Zhang]

The legacy CPPC feedback-counter path reads the delivered and reference
performance counters separately.

On arm64 systems using AMU-backed CPPC FFH counters, each FFH read is
served through a cross-CPU counter read helper. Reading the counters
separately therefore widens the sampling window between them and can
skew the delivered/reference ratio used by cpuinfo_cur_freq. Under heavy
load, the skew is observable as transient values that may exceed the
platform maximum, as discussed in [1] and [2].

This series adds a small generic hook for architectures that can obtain
both FFH feedback counters in one operation, while preserving the
existing per-register read path as the fallback.

Patch 1 adds the generic CPPC hook and uses it from cppc_get_perf_ctrs().
Patch 2 implements the hook on arm64 by sampling both AMU counters in a
single operation on the target CPU.

For detailed test results and data demonstrating the observable skew and
the improvements brought by this series, please refer to [3] and [4].

[1] https://lore.kernel.org/all/20231025093847.3740104-4-zengheng4@huawei.com/
[2] https://lore.kernel.org/all/20231212072617.14756-1-lihuisong@huawei.com/
[3] https://lore.kernel.org/all/443104e2-ba6e-454e-8469-909f35817a99@huawei.com/
[4] https://lore.kernel.org/all/317d33d5-8279-4aa8-84b7-6ae1976636ac@huawei.com/

Tested-by: Sumit Gupta <sumitg@nvidia.com>
Reviewed-by: Sumit Gupta <sumitg@nvidia.com>
Tested-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
Reviewed-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
---
Changes in v2:
- Simplified the CPPC generic layer fallback logic to prevent pointless single-read retries.
- Added upfront register validation in the arm64 hook to avoid unnecessary IPI overhead.
- Explicitly flipped the -EOPNOTSUPP error to -ENODEV in the arm64 hook when AMU is unsupported, cleanly bypassing redundant CPPC generic fallbacks.
- Addressed other kernel-doc and naming feedbacks from Beata.
- Added Reviewed-by and Tested-by tags from Vanshidhar and Sumit
- Link to v1: https://lore.kernel.org/all/20260410094145.4132082-1-zhangpengjie2@huawei.com/

Pengjie Zhang (2):
  ACPI: CPPC: add paired FFH feedback-counter read hook
  arm64: topology: read CPPC FFH feedback counters in one operation

 arch/arm64/kernel/topology.c | 92 ++++++++++++++++++++++++++++++++----
 drivers/acpi/cppc_acpi.c     | 50 ++++++++++++++++++--
 include/acpi/cppc_acpi.h     |  7 +++
 3 files changed, 136 insertions(+), 13 deletions(-)

-- 
2.33.0

[PATCH v2 1/2] ACPI: CPPC: add paired FFH feedback-counter read hook

[Pengjie Zhang]

cppc_get_perf_ctrs() reads the delivered and reference performance
counters one at a time.

Allow architectures to provide both FFH feedback counters in one
operation when that either narrows the sampling window or avoids extra
cross-CPU reads. Add a small FFH-specific hook for that case and fall
back to the existing per-register reads when unsupported.

Tested-by: Sumit Gupta <sumitg@nvidia.com>
Reviewed-by: Sumit Gupta <sumitg@nvidia.com>
Tested-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
Reviewed-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
---
 drivers/acpi/cppc_acpi.c | 50 ++++++++++++++++++++++++++++++++++++----
 include/acpi/cppc_acpi.h |  7 ++++++
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 9f572f481241..b6356476a46e 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1004,6 +1004,22 @@ int __weak cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val)
 	return -ENOTSUPP;
 }
 
+/**
+ * cpc_read_ffh_fb_ctrs() - Read FFH feedback counters together
+ * @cpunum:	Target CPU
+ * @reg1:	first CPPC register information
+ * @val1:	place holder for first return value
+ * @reg2:	second CPPC register information
+ * @val2:	place holder for second return value
+ *
+ * Return: 0 on success, error code otherwise
+ */
+int __weak cpc_read_ffh_fb_ctrs(int cpunum, struct cpc_reg *reg1,
+				u64 *val1, struct cpc_reg *reg2, u64 *val2)
+{
+	return -EOPNOTSUPP;
+}
+
 /**
  * cpc_write_ffh() - Write FFH register
  * @cpunum:	CPU number to write
@@ -1496,6 +1512,33 @@ bool cppc_perf_ctrs_in_pcc_cpu(unsigned int cpu)
 }
 EXPORT_SYMBOL_GPL(cppc_perf_ctrs_in_pcc_cpu);
 
+static int cppc_read_fb_ctrs(int cpunum,
+			     struct cpc_register_resource *delivered_reg,
+			     struct cpc_register_resource *reference_reg,
+			     u64 *delivered, u64 *reference)
+{
+	int ret;
+
+	/*
+	 * For FFH feedback counters, try a paired read first to reduce
+	 * sampling skew between delivered and reference counters. Fall
+	 * back to the existing per-register reads if unsupported.
+	 */
+	if (CPC_IN_FFH(delivered_reg) && CPC_IN_FFH(reference_reg)) {
+		ret = cpc_read_ffh_fb_ctrs(cpunum,
+					&delivered_reg->cpc_entry.reg, delivered,
+					&reference_reg->cpc_entry.reg, reference);
+		if (ret != -EOPNOTSUPP)
+			return ret;
+	}
+
+	ret = cpc_read(cpunum, delivered_reg, delivered);
+	if (ret)
+		return ret;
+
+	return cpc_read(cpunum, reference_reg, reference);
+}
+
 /**
  * cppc_perf_ctrs_in_pcc - Check if any perf counters are in a PCC region.
  *
@@ -1561,11 +1604,8 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 		}
 	}
 
-	ret = cpc_read(cpunum, delivered_reg, &delivered);
-	if (ret)
-		goto out_err;
-
-	ret = cpc_read(cpunum, reference_reg, &reference);
+	ret = cppc_read_fb_ctrs(cpunum, delivered_reg, reference_reg,
+				&delivered, &reference);
 	if (ret)
 		goto out_err;
 
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 8693890a7275..5acebe62feac 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -176,6 +176,8 @@ extern int cppc_get_transition_latency(int cpu);
 extern bool cpc_ffh_supported(void);
 extern bool cpc_supported_by_cpu(void);
 extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
+extern int cpc_read_ffh_fb_ctrs(int cpu, struct cpc_reg *reg1, u64 *val1,
+				struct cpc_reg *reg2, u64 *val2);
 extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
 extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf);
 extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
@@ -250,6 +252,11 @@ static inline int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val)
 {
 	return -EOPNOTSUPP;
 }
+static inline int cpc_read_ffh_fb_ctrs(int cpu, struct cpc_reg *reg1, u64 *val1,
+				       struct cpc_reg *reg2, u64 *val2)
+{
+	return -EOPNOTSUPP;
+}
 static inline int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
 {
 	return -EOPNOTSUPP;
-- 
2.33.0

[PATCH v2 2/2] arm64: topology: read CPPC FFH feedback counters in one operation

[Pengjie Zhang]

arm64 implements CPPC FFH feedback-counter reads using AMU counters.
Because those counters must be sampled on the target CPU, reading the
delivered and reference counters separately widens the observation window
between them.

Implement the paired FFH feedback-counter read hook on arm64 and sample
both AMU counters together before decoding the requested CPC register
values.

Also factor the FFH bitfield extraction logic into a helper and reuse
it from the existing single-counter FFH read path.

Tested-by: Sumit Gupta <sumitg@nvidia.com>
Reviewed-by: Sumit Gupta <sumitg@nvidia.com>
Tested-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
Reviewed-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
---
 arch/arm64/kernel/topology.c | 92 ++++++++++++++++++++++++++++++++----
 1 file changed, 84 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index b32f13358fbb..d28438f8b83f 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -373,6 +373,16 @@ core_initcall(init_amu_fie);
 #ifdef CONFIG_ACPI_CPPC_LIB
 #include <acpi/cppc_acpi.h>
 
+struct amu_ffh_ctrs {
+	u64 corecnt;
+	u64 constcnt;
+};
+
+enum cpc_ffh_ctr_id {
+	CPC_FFH_CTR_CORE  = 0x0,
+	CPC_FFH_CTR_CONST = 0x1,
+};
+
 static void cpu_read_corecnt(void *val)
 {
 	/*
@@ -397,7 +407,7 @@ static void cpu_read_constcnt(void *val)
 }
 
 static inline
-int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val)
+int counters_read_on_cpu(int cpu, smp_call_func_t func, void *val)
 {
 	/*
 	 * Abort call on counterless CPU.
@@ -447,24 +457,90 @@ bool cpc_ffh_supported(void)
 	return true;
 }
 
+static void amu_read_core_const_ctrs(void *val)
+{
+	struct amu_ffh_ctrs *ctrs = val;
+
+	/*
+	 * cpu_read_constcnt() incurs slight latency due to the
+	 * ARM64_WORKAROUND_2457168 check. Read it first to minimize
+	 * the sampling skew between the const and core counters.
+	 */
+	cpu_read_constcnt(&ctrs->constcnt);
+	cpu_read_corecnt(&ctrs->corecnt);
+}
+
+static u64 cpc_ffh_extract_bits(const struct cpc_reg *reg, u64 val)
+{
+	val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
+			   reg->bit_offset);
+	val >>= reg->bit_offset;
+
+	return val;
+}
+
+static void cpc_ffh_ctr_value(const struct cpc_reg *reg,
+			      const struct amu_ffh_ctrs *ctrs, u64 *val)
+{
+	switch ((u64)reg->address) {
+	case CPC_FFH_CTR_CORE:
+		*val = ctrs->corecnt;
+		break;
+	case CPC_FFH_CTR_CONST:
+		*val = ctrs->constcnt;
+		break;
+	}
+
+	*val = cpc_ffh_extract_bits(reg, *val);
+}
+
+static bool is_amu_ctr_reg(const struct cpc_reg *reg)
+{
+	return reg->address == CPC_FFH_CTR_CORE ||
+		reg->address == CPC_FFH_CTR_CONST;
+}
+
+int cpc_read_ffh_fb_ctrs(int cpu, struct cpc_reg *reg1, u64 *val1,
+			 struct cpc_reg *reg2, u64 *val2)
+{
+	struct amu_ffh_ctrs ctrs;
+	int ret;
+
+	if (!is_amu_ctr_reg(reg1) || !is_amu_ctr_reg(reg2))
+		return -EINVAL;
+
+	ret = counters_read_on_cpu(cpu, amu_read_core_const_ctrs, &ctrs);
+	if (ret) {
+		/*
+		 * If AMU is unsupported (-EOPNOTSUPP), translate the error
+		 * to -ENODEV. This explicitly tells the generic CPPC layer
+		 * to abort immediately and avoid falling back to pointless
+		 * single-counter reads.
+		 */
+		return ret == -EOPNOTSUPP ? -ENODEV : ret;
+	}
+
+	cpc_ffh_ctr_value(reg1, &ctrs, val1);
+	cpc_ffh_ctr_value(reg2, &ctrs, val2);
+
+	return 0;
+}
+
 int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
 {
 	int ret = -EOPNOTSUPP;
 
 	switch ((u64)reg->address) {
-	case 0x0:
+	case CPC_FFH_CTR_CORE:
 		ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
 		break;
-	case 0x1:
+	case CPC_FFH_CTR_CONST:
 		ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
 		break;
 	}
 
-	if (!ret) {
-		*val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
-				    reg->bit_offset);
-		*val >>= reg->bit_offset;
-	}
+	if (!ret)
+		*val = cpc_ffh_extract_bits(reg, *val);
 
 	return ret;
 }
-- 
2.33.0

Re: [PATCH v2 0/2] CPPC: reduce FFH feedback-counter sampling skew on arm64

[Jeremy Linton]

Hi,

On 7/8/26 3:28 AM, Pengjie Zhang wrote:
> The legacy CPPC feedback-counter path reads the delivered and reference
> performance counters separately.
> 
> On arm64 systems using AMU-backed CPPC FFH counters, each FFH read is
> served through a cross-CPU counter read helper. Reading the counters
> separately therefore widens the sampling window between them and can
> skew the delivered/reference ratio used by cpuinfo_cur_freq. Under heavy
> load, the skew is observable as transient values that may exceed the
> platform maximum, as discussed in [1] and [2].
> 
> This series adds a small generic hook for architectures that can obtain
> both FFH feedback counters in one operation, while preserving the
> existing per-register read path as the fallback.
> 
> Patch 1 adds the generic CPPC hook and uses it from cppc_get_perf_ctrs().
> Patch 2 implements the hook on arm64 by sampling both AMU counters in a
> single operation on the target CPU.
> 
> For detailed test results and data demonstrating the observable skew and
> the improvements brought by this series, please refer to [3] and [4].

So, this set appears to help considerably, I have a small script which 
after setting a fixed CPU frequency proceeds to sample cpuinfo_cur_freq 
and builds the mean/stddev/stderr and worst case error percentage. Its 
really the latter that I've been focusing on. When run under varying 
workloads, local vs cross cpu, t builds what I think is a somewhat 
reasonable picture of the machine behaviors.

Given a machine that at a baseline, has an error percentage that can 
exceed 100% (ex the actual frequency is 2.4Ghz, and it can report > 
5Ghz), this patch appears to bring the worst case down to approximately 
~20%, but usually its somewhere around 5%. This is similar to the v1 of 
the jitter patch I posted, but worse than than the best sample version 
of the jitter patch I've been promising, which does a better job 
selecting the initial sample.

But combining them is magic, the tweaked version reduces that to less 
than a percent.

The combination appears good enough to consistently detect small 
variations between the requested frequencies and the rate the delivered 
clock is claiming (ex request 2600 Mhz, consistently get 2630 Mhz +- 2Mhz).


With that:

Tested-by: Jeremy Linton <jeremy.linton@arm.com>

I have a small nitpik for review, that should only be considered if for 
some reason this gets respun, but otherwise.

Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>




> 
> [1] https://lore.kernel.org/all/20231025093847.3740104-4-zengheng4@huawei.com/
> [2] https://lore.kernel.org/all/20231212072617.14756-1-lihuisong@huawei.com/
> [3] https://lore.kernel.org/all/443104e2-ba6e-454e-8469-909f35817a99@huawei.com/
> [4] https://lore.kernel.org/all/317d33d5-8279-4aa8-84b7-6ae1976636ac@huawei.com/
> 
> Tested-by: Sumit Gupta <sumitg@nvidia.com>
> Reviewed-by: Sumit Gupta <sumitg@nvidia.com>
> Tested-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> Reviewed-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
> ---
> Changes in v2:
> - Simplified the CPPC generic layer fallback logic to prevent pointless single-read retries.
> - Added upfront register validation in the arm64 hook to avoid unnecessary IPI overhead.
> - Explicitly flipped the -EOPNOTSUPP error to -ENODEV in the arm64 hook when AMU is unsupported, cleanly bypassing redundant CPPC generic fallbacks.
> - Addressed other kernel-doc and naming feedbacks from Beata.
> - Added Reviewed-by and Tested-by tags from Vanshidhar and Sumit
> - Link to v1: https://lore.kernel.org/all/20260410094145.4132082-1-zhangpengjie2@huawei.com/
> 
> Pengjie Zhang (2):
>    ACPI: CPPC: add paired FFH feedback-counter read hook
>    arm64: topology: read CPPC FFH feedback counters in one operation
> 
>   arch/arm64/kernel/topology.c | 92 ++++++++++++++++++++++++++++++++----
>   drivers/acpi/cppc_acpi.c     | 50 ++++++++++++++++++--
>   include/acpi/cppc_acpi.h     |  7 +++
>   3 files changed, 136 insertions(+), 13 deletions(-)
> 

Re: [PATCH v2 2/2] arm64: topology: read CPPC FFH feedback counters in one operation

[Jeremy Linton]

Hi,

On 7/8/26 3:28 AM, Pengjie Zhang wrote:
> arm64 implements CPPC FFH feedback-counter reads using AMU counters.
> Because those counters must be sampled on the target CPU, reading the
> delivered and reference counters separately widens the observation window
> between them.
> 
> Implement the paired FFH feedback-counter read hook on arm64 and sample
> both AMU counters together before decoding the requested CPC register
> values.
> 
> Also factor the FFH bitfield extraction logic into a helper and reuse
> it from the existing single-counter FFH read path.
> 
> Tested-by: Sumit Gupta <sumitg@nvidia.com>
> Reviewed-by: Sumit Gupta <sumitg@nvidia.com>
> Tested-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> Reviewed-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
> ---
>   arch/arm64/kernel/topology.c | 92 ++++++++++++++++++++++++++++++++----
>   1 file changed, 84 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index b32f13358fbb..d28438f8b83f 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -373,6 +373,16 @@ core_initcall(init_amu_fie);
>   #ifdef CONFIG_ACPI_CPPC_LIB
>   #include <acpi/cppc_acpi.h>
>   
> +struct amu_ffh_ctrs {
> +	u64 corecnt;
> +	u64 constcnt;
> +};
> +
> +enum cpc_ffh_ctr_id {
> +	CPC_FFH_CTR_CORE  = 0x0,
> +	CPC_FFH_CTR_CONST = 0x1,
> +};
> +
>   static void cpu_read_corecnt(void *val)
>   {
>   	/*
> @@ -397,7 +407,7 @@ static void cpu_read_constcnt(void *val)
>   }
>   
>   static inline
> -int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val)
> +int counters_read_on_cpu(int cpu, smp_call_func_t func, void *val)
>   {
>   	/*
>   	 * Abort call on counterless CPU.
> @@ -447,24 +457,90 @@ bool cpc_ffh_supported(void)
>   	return true;
>   }
>   
> +static void amu_read_core_const_ctrs(void *val)
> +{
> +	struct amu_ffh_ctrs *ctrs = val;
> +
> +	/*
> +	 * cpu_read_constcnt() incurs slight latency due to the
> +	 * ARM64_WORKAROUND_2457168 check. Read it first to minimize
> +	 * the sampling skew between the const and core counters.
> +	 */
> +	cpu_read_constcnt(&ctrs->constcnt);
> +	cpu_read_corecnt(&ctrs->corecnt);
> +}
> +
> +static u64 cpc_ffh_extract_bits(const struct cpc_reg *reg, u64 val)
> +{
> +	val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
> +			   reg->bit_offset);
> +	val >>= reg->bit_offset;
> +
> +	return val;
> +}
> +
> +static void cpc_ffh_ctr_value(const struct cpc_reg *reg,
> +			      const struct amu_ffh_ctrs *ctrs, u64 *val)
> +{
> +	switch ((u64)reg->address) {
> +	case CPC_FFH_CTR_CORE:
> +		*val = ctrs->corecnt;
> +		break;
> +	case CPC_FFH_CTR_CONST:
> +		*val = ctrs->constcnt;
> +		break;
> +	}
> +
> +	*val = cpc_ffh_extract_bits(reg, *val);
> +}
> +
> +static bool is_amu_ctr_reg(const struct cpc_reg *reg)
> +{
> +	return reg->address == CPC_FFH_CTR_CORE ||
> +		reg->address == CPC_FFH_CTR_CONST;
> +}
> +
> +int cpc_read_ffh_fb_ctrs(int cpu, struct cpc_reg *reg1, u64 *val1,
> +			 struct cpc_reg *reg2, u64 *val2)
> +{
> +	struct amu_ffh_ctrs ctrs;
> +	int ret;
> +
> +	if (!is_amu_ctr_reg(reg1) || !is_amu_ctr_reg(reg2))
> +		return -EINVAL;
> +
> +	ret = counters_read_on_cpu(cpu, amu_read_core_const_ctrs, &ctrs);
> +	if (ret) {
> +		/*
> +		 * If AMU is unsupported (-EOPNOTSUPP), translate the error
> +		 * to -ENODEV. This explicitly tells the generic CPPC layer
> +		 * to abort immediately and avoid falling back to pointless
> +		 * single-counter reads.
> +		 */
> +		return ret == -EOPNOTSUPP ? -ENODEV : ret;
> +	}
> +
> +	cpc_ffh_ctr_value(reg1, &ctrs, val1);
> +	cpc_ffh_ctr_value(reg2, &ctrs, val2);
> +
> +	return 0;
> +}
> +
>   int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
>   {
>   	int ret = -EOPNOTSUPP;
>   
>   	switch ((u64)reg->address) {
> -	case 0x0:
> +	case CPC_FFH_CTR_CORE:
>   		ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
>   		break;
> -	case 0x1:
> +	case CPC_FFH_CTR_CONST:
>   		ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
>   		break;
>   	}
>   
> -	if (!ret) {
> -		*val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
> -				    reg->bit_offset);
> -		*val >>= reg->bit_offset;
> -	}
> +	if (!ret)
> +		*val = cpc_ffh_extract_bits(reg, *val);
>   
>   	return ret;
>   }


So, more a nitpik that only applies if this set gets respun, but:

I don't think this FFH counter logic belongs in the arm64 topology.c 
file, its not really topology related.

Re: [PATCH v2 2/2] arm64: topology: read CPPC FFH feedback counters in one operation

[Beata Michalska]

On Thu, Jul 09, 2026 at 01:11:13AM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 7/8/26 3:28 AM, Pengjie Zhang wrote:
> > arm64 implements CPPC FFH feedback-counter reads using AMU counters.
> > Because those counters must be sampled on the target CPU, reading the
> > delivered and reference counters separately widens the observation window
> > between them.
> > 
> > Implement the paired FFH feedback-counter read hook on arm64 and sample
> > both AMU counters together before decoding the requested CPC register
> > values.
> > 
> > Also factor the FFH bitfield extraction logic into a helper and reuse
> > it from the existing single-counter FFH read path.
> > 
> > Tested-by: Sumit Gupta <sumitg@nvidia.com>
> > Reviewed-by: Sumit Gupta <sumitg@nvidia.com>
> > Tested-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> > Reviewed-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> > Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
> > ---
> >   arch/arm64/kernel/topology.c | 92 ++++++++++++++++++++++++++++++++----
> >   1 file changed, 84 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index b32f13358fbb..d28438f8b83f 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -373,6 +373,16 @@ core_initcall(init_amu_fie);
> >   #ifdef CONFIG_ACPI_CPPC_LIB
> >   #include <acpi/cppc_acpi.h>
> > +struct amu_ffh_ctrs {
> > +	u64 corecnt;
> > +	u64 constcnt;
> > +};
> > +
> > +enum cpc_ffh_ctr_id {
> > +	CPC_FFH_CTR_CORE  = 0x0,
> > +	CPC_FFH_CTR_CONST = 0x1,
> > +};
> > +
> >   static void cpu_read_corecnt(void *val)
> >   {
> >   	/*
> > @@ -397,7 +407,7 @@ static void cpu_read_constcnt(void *val)
> >   }
> >   static inline
> > -int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val)
> > +int counters_read_on_cpu(int cpu, smp_call_func_t func, void *val)
> >   {
> >   	/*
> >   	 * Abort call on counterless CPU.
> > @@ -447,24 +457,90 @@ bool cpc_ffh_supported(void)
> >   	return true;
> >   }
> > +static void amu_read_core_const_ctrs(void *val)
> > +{
> > +	struct amu_ffh_ctrs *ctrs = val;
> > +
> > +	/*
> > +	 * cpu_read_constcnt() incurs slight latency due to the
> > +	 * ARM64_WORKAROUND_2457168 check. Read it first to minimize
> > +	 * the sampling skew between the const and core counters.
> > +	 */
> > +	cpu_read_constcnt(&ctrs->constcnt);
> > +	cpu_read_corecnt(&ctrs->corecnt);
> > +}
> > +
> > +static u64 cpc_ffh_extract_bits(const struct cpc_reg *reg, u64 val)
> > +{
> > +	val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
> > +			   reg->bit_offset);
> > +	val >>= reg->bit_offset;
> > +
> > +	return val;
> > +}
> > +
> > +static void cpc_ffh_ctr_value(const struct cpc_reg *reg,
> > +			      const struct amu_ffh_ctrs *ctrs, u64 *val)
> > +{
> > +	switch ((u64)reg->address) {
> > +	case CPC_FFH_CTR_CORE:
> > +		*val = ctrs->corecnt;
> > +		break;
> > +	case CPC_FFH_CTR_CONST:
> > +		*val = ctrs->constcnt;
> > +		break;
> > +	}
> > +
> > +	*val = cpc_ffh_extract_bits(reg, *val);
> > +}
> > +
> > +static bool is_amu_ctr_reg(const struct cpc_reg *reg)
> > +{
> > +	return reg->address == CPC_FFH_CTR_CORE ||
> > +		reg->address == CPC_FFH_CTR_CONST;
> > +}
> > +
> > +int cpc_read_ffh_fb_ctrs(int cpu, struct cpc_reg *reg1, u64 *val1,
> > +			 struct cpc_reg *reg2, u64 *val2)
> > +{
> > +	struct amu_ffh_ctrs ctrs;
> > +	int ret;
> > +
> > +	if (!is_amu_ctr_reg(reg1) || !is_amu_ctr_reg(reg2))
> > +		return -EINVAL;
> > +
> > +	ret = counters_read_on_cpu(cpu, amu_read_core_const_ctrs, &ctrs);
> > +	if (ret) {
> > +		/*
> > +		 * If AMU is unsupported (-EOPNOTSUPP), translate the error
> > +		 * to -ENODEV. This explicitly tells the generic CPPC layer
> > +		 * to abort immediately and avoid falling back to pointless
> > +		 * single-counter reads.
> > +		 */
> > +		return ret == -EOPNOTSUPP ? -ENODEV : ret;
> > +	}
> > +
> > +	cpc_ffh_ctr_value(reg1, &ctrs, val1);
> > +	cpc_ffh_ctr_value(reg2, &ctrs, val2);
> > +
> > +	return 0;
> > +}
> > +
> >   int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val)
> >   {
> >   	int ret = -EOPNOTSUPP;
> >   	switch ((u64)reg->address) {
> > -	case 0x0:
> > +	case CPC_FFH_CTR_CORE:
> >   		ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val);
> >   		break;
> > -	case 0x1:
> > +	case CPC_FFH_CTR_CONST:
> >   		ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val);
> >   		break;
> >   	}
> > -	if (!ret) {
> > -		*val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
> > -				    reg->bit_offset);
> > -		*val >>= reg->bit_offset;
> > -	}
> > +	if (!ret)
> > +		*val = cpc_ffh_extract_bits(reg, *val);
> >   	return ret;
> >   }
> 
> 
> So, more a nitpik that only applies if this set gets respun, but:
> 
> I don't think this FFH counter logic belongs in the arm64 topology.c file,
> its not really topology related.

I agree. Those bits should be moved.
That said, I think it would be best to land this first,
as it addresses a rather long-standing issue.
We can do the cleanup later. If needed, I'd pick it up soon'ish.

---
BR
Beata
> 
> 
> 
> 
> 

Re: [PATCH v2 0/2] CPPC: reduce FFH feedback-counter sampling skew on arm64

[Beata Michalska]

Looks good to me, though I guess you might need an ack from Rafael (?)

Thank you for the patches.

---
BR
Beata
On Wed, Jul 08, 2026 at 04:28:16PM +0800, Pengjie Zhang wrote:
> The legacy CPPC feedback-counter path reads the delivered and reference
> performance counters separately.
> 
> On arm64 systems using AMU-backed CPPC FFH counters, each FFH read is
> served through a cross-CPU counter read helper. Reading the counters
> separately therefore widens the sampling window between them and can
> skew the delivered/reference ratio used by cpuinfo_cur_freq. Under heavy
> load, the skew is observable as transient values that may exceed the
> platform maximum, as discussed in [1] and [2].
> 
> This series adds a small generic hook for architectures that can obtain
> both FFH feedback counters in one operation, while preserving the
> existing per-register read path as the fallback.
> 
> Patch 1 adds the generic CPPC hook and uses it from cppc_get_perf_ctrs().
> Patch 2 implements the hook on arm64 by sampling both AMU counters in a
> single operation on the target CPU.
> 
> For detailed test results and data demonstrating the observable skew and
> the improvements brought by this series, please refer to [3] and [4].
> 
> [1] https://lore.kernel.org/all/20231025093847.3740104-4-zengheng4@huawei.com/
> [2] https://lore.kernel.org/all/20231212072617.14756-1-lihuisong@huawei.com/
> [3] https://lore.kernel.org/all/443104e2-ba6e-454e-8469-909f35817a99@huawei.com/
> [4] https://lore.kernel.org/all/317d33d5-8279-4aa8-84b7-6ae1976636ac@huawei.com/
> 
> Tested-by: Sumit Gupta <sumitg@nvidia.com>
> Reviewed-by: Sumit Gupta <sumitg@nvidia.com>
> Tested-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> Reviewed-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
> ---
> Changes in v2:
> - Simplified the CPPC generic layer fallback logic to prevent pointless single-read retries.
> - Added upfront register validation in the arm64 hook to avoid unnecessary IPI overhead.
> - Explicitly flipped the -EOPNOTSUPP error to -ENODEV in the arm64 hook when AMU is unsupported, cleanly bypassing redundant CPPC generic fallbacks.
> - Addressed other kernel-doc and naming feedbacks from Beata.
> - Added Reviewed-by and Tested-by tags from Vanshidhar and Sumit
> - Link to v1: https://lore.kernel.org/all/20260410094145.4132082-1-zhangpengjie2@huawei.com/
> 
> Pengjie Zhang (2):
>   ACPI: CPPC: add paired FFH feedback-counter read hook
>   arm64: topology: read CPPC FFH feedback counters in one operation
> 
>  arch/arm64/kernel/topology.c | 92 ++++++++++++++++++++++++++++++++----
>  drivers/acpi/cppc_acpi.c     | 50 ++++++++++++++++++--
>  include/acpi/cppc_acpi.h     |  7 +++
>  3 files changed, 136 insertions(+), 13 deletions(-)
> 
> -- 
> 2.33.0
>