Re: [RESEND][PATCH 0/3] ACPI: CPPC: Reduce .target() callback overhead

On 8/3/26 22:05, Christian Loehle wrote:
> cppc-cpufreq reaches cppc_set_perf() from every target callback.  For
> direct SystemMemory controls, that path currently does several steps
> which are unnecessary once the immutable _CPC layout is known:
> 
>   - a full-width write first reads the access unit and merges the value.
>   - every write takes the descriptor's RMW lock, even when its access unit
>     is not shared with another _CPC entry.
>   - cppc_set_perf() evaluates the same three PCC predicates at each phase
>     of the transaction.
> 
> Remove those costs while retaining the existing conservative paths for
> partial fields, overlapping or malformed access units, and PCC controls.
> 
> The series was tested on Arm Power-Orion O6 and AmpereOne systems using
> cppc-cpufreq and schedutil.  An rt-app task pinned to one CPU ran for
> 500 us every 2 ms with uclamp.min=512, for 5000 periods per run.  schedutil
> rate_limit_us was 1000.  cppc_cpufreq_fast_switch() latency was measured
> for 100 ten-second runs.  Each sample is the mean callback latency within
> one run, and the table reports the median and sample standard deviation
> of those samples.
> 
> Orion O6               median       stdev       callbacks
> baseline               5703 ns      307 ns       188017
> complete series        5023 ns      240 ns       188209
> ==> 680 ns (11.9%) reduction
> 
> AmpereOne              median       stdev
> baseline               2090 ns      157 ns
> complete series      1907.5 ns      140 ns
> ==> 182.5 ns (8.7%) reduction
> 
> The cumulative intermediate results on the Orion O6 attribute roughly half
> of the gain to each of the first two patches: avoiding the read reduced
> the median by 284 ns (5.0%), and avoiding the lock reduced it by another
> 336 ns (6.2%).
> Together they account for 620 ns of the 680 ns total reduction.
> 
> With the same arm64 configuration and GCC 11.4, caching the PCC predicate
> also reduces cppc_set_perf() from 1124 to 884 bytes.  The generated
> function has 60 fewer instructions, 17 fewer loads and 20 fewer branches.
> 
> This series is based on the CPPC fixes posted at (already queued):
> https://lore.kernel.org/lkml/[email protected]/
> 
> and the separately posted fix still under review:
> https://lore.kernel.org/lkml/[email protected]/
> 
> PS:
> There's a final optimization that I actually wanted to make but decided to
> split it out for now as it somewhat replicated Sumit's series:
> Skipping redundant perf ctrl writes in cpufreq-cppc if registers are non-PCC
> and !shared (because the values are unchanged, the
> !autonomous-common-case), but that requires the driver to have a more
> complex caching- and atomic-updating machinery in place, similar to
> hotplug. As opposed to this series the optimization would be for the
> microcontroller handling the CPPC requests, which may be shared across
> many CPUs and therefore redundant requests can increase the dvfs
> transition latency.
> That patch will follow once Sumit's is queued:
> https://lore.kernel.org/lkml/[email protected]/
> 
> [RESEND] for the new base-commit specifier below
> 
> Christian Loehle (3):
>   ACPI: CPPC: Avoid unnecessary reads for full-width writes
>   ACPI: CPPC: Avoid locking standalone full-width registers
>   ACPI: CPPC: Evaluate performance-control PCC use once
> 
>  drivers/acpi/cppc_acpi.c | 135 +++++++++++++++++++++++++++++++--------
>  include/acpi/cppc_acpi.h |   5 +-
>  2 files changed, 113 insertions(+), 27 deletions(-)
> 
> 
> base-commit: 075b74841bd0065a3bda3440873c747938e69b68

Nice, so that worked:
https://sashiko.dev/#/patchset/[email protected]
Seems no findings on $SUBJECT but the rest of the comments it has
look legit to me? I'll go take another look tomorrow:

1. Using per-CPU cpc_desc->rmw_lock for SYSTEM_MEMORY CPPC control register that
may be shared.
2. MASK_VAL_WRITE() truncation on 32bit architectures.
3. acpi_cppc_processor_exit() calls kfree(cpc_ptr) unconditionally (UAF with sysfs?)
4. cppc_set_reg_val_in_pcc() calls cpc_write() before down_write(&pcc_ss_data->pcc_lock)
5 & 6 are sanitization of values from FW.