Re: [PATCH v5] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Al Stone]

On 08/01/2016 02:31 PM, Viresh Kumar wrote:
> [+ Ashwin's new email id..]
> 
> On 20-07-16, 15:10, Al Stone wrote:
>> When CPPC is being used by ACPI on arm64, user space tools such as
>> cpupower report CPU frequency values from sysfs that are incorrect.
>>
>> What the driver was doing was reporting the values given by ACPI tables
>> in whatever scale was used to provide them.  However, the ACPI spec
>> defines the CPPC values as unitless abstract numbers.  Internal kernel
>> structures such as struct perf_cap, in contrast, expect these values
>> to be in KHz.  When these struct values get reported via sysfs, the
>> user space tools also assume they are in KHz, causing them to report
>> incorrect values (for example, reporting a CPU frequency of 1MHz when
>> it should be 1.8GHz).
>>
>> The downside is that this approach has some assumptions:
>>
>>    (1) It relies on SMBIOS3 being used, *and* that the Max Frequency
>>    value for a processor is set to a non-zero value.
>>
>>    (2) It assumes that all processors run at the same speed, or that
>>    the CPPC values have all been scaled to reflect relative speed.
>>    This patch retrieves the largest CPU Max Frequency from a type 4 DMI
>>    record that it can find.  This may not be an issue, however, as a
>>    sampling of DMI data on x86 and arm64 indicates there is often only
>>    one such record regardless.  Since CPPC is relatively new, it is
>>    unclear if the ACPI ASL will always be written to reflect any sort
>>    of relative performance of processors of differing speeds.
>>
>>    (3) It assumes that performance and frequency both scale linearly.
>>
>> For arm64 servers, this may be sufficient, but it does rely on
>> firmware values being set correctly.  Hence, other approaches will
>> be considered in the future.
>>
>> This has been tested on three arm64 servers, with and without DMI, with
>> and without CPPC support.
>>
>> Changes for v5:
>>     -- Move code to cpufreq/cppc_cpufreq.c from acpi/cppc_acpi.c to keep
>>        frequency-related code together, and keep the CPPC abstract scale
>>        in ACPI (Prashanth Prakash)
>>     -- Fix the scaling to remove the incorrect assumption that frequency
>>        was always a range from zero to max; as a practical matter, it is
>>        not (Prasanth Prakash); this also allowed us to remove an over-
>>        engineered function to do this math.
>>
>> Changes for v4:
>>     -- Replaced magic constants with #defines (Rafael Wysocki)
>>     -- Renamed cppc_unitless_to_khz() to cppc_to_khz() (Rafael Wysocki)
>>     -- Replaced hidden initialization with a clearer form (Rafael Wysocki)
>>     -- Instead of picking up the first Max Speed value from DMI, we will
>>        now get the largest Max Speed; still an approximation, but slightly
>>        less subject to error (Rafael Wysocki)
>>     -- Kconfig for cppc_cpufreq now depends on DMI, instead of selecting
>>        it, in order to make sure DMI is set up properly (Rafael Wysocki)
>>
>> Changes for v3:
>>     -- Added clarifying commentary re short-term vs long-term fix (Alexey
>>        Klimov)
>>     -- Added range checking code to ensure proper arithmetic occurs,
>>        especially no division by zero (Alexey Klimov)
>>
>> Changes for v2:
>>     -- Corrected thinko: needed to have DEPENDS on DMI in Kconfig.arm,
>>        not SELECT DMI (found by build daemon)
>>
>> Signed-off-by: Al Stone <ahs3@xxxxxxxxxx>
>> Signed-off-by: Prashanth Prakash <pprakash@xxxxxxxxxxxxxx>
>> ---
>>  drivers/cpufreq/cppc_cpufreq.c | 53 ++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 8882b8e..6debc18 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -19,10 +19,19 @@
>>  #include <linux/delay.h>
>>  #include <linux/cpu.h>
>>  #include <linux/cpufreq.h>
>> +#include <linux/dmi.h>
>>  #include <linux/vmalloc.h>
>>  
>> +#include <asm/unaligned.h>
>> +
>>  #include <acpi/cppc_acpi.h>
>>  
>> +/* Minimum struct length needed for the DMI processor entry we want */
>> +#define DMI_ENTRY_PROCESSOR_MIN_LENGTH	48
>> +
>> +/* Offest in the DMI processor structure for the max frequency */
>> +#define DMI_PROCESSOR_MAX_SPEED  0x14
>> +
>>  /*
>>   * These structs contain information parsed from per CPU
>>   * ACPI _CPC structures.
>> @@ -32,6 +41,39 @@
>>   */
>>  static struct cpudata **all_cpu_data;
>>  
>> +/* Capture the max KHz from DMI */
>> +static u64 cppc_dmi_max_khz;
>> +
>> +/* Callback function used to retrieve the max frequency from DMI */
>> +static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
>> +{
>> +	const u8 *dmi_data = (const u8 *)dm;
>> +	u16 *mhz = (u16 *)private;
>> +
>> +	if (dm->type == DMI_ENTRY_PROCESSOR &&
>> +	    dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) {
>> +		u16 val = (u16)get_unaligned((const u16 *)
>> +				(dmi_data + DMI_PROCESSOR_MAX_SPEED));
>> +		*mhz = val > *mhz ? val : *mhz;
>> +	}
>> +}
>> +
>> +/* Look up the max frequency in DMI */
>> +static u64 cppc_get_dmi_max_khz(void)
>> +{
>> +	u16 mhz = 0;
>> +
>> +	dmi_walk(cppc_find_dmi_mhz, &mhz);
>> +
>> +	/*
>> +	 * Real stupid fallback value, just in case there is no
>> +	 * actual value set.
>> +	 */
>> +	mhz = mhz ? mhz : 1;
>> +
>> +	return (1000 * mhz);
>> +}
>> +
>>  static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>>  		unsigned int target_freq,
>>  		unsigned int relation)
>> @@ -42,7 +84,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>>  
>>  	cpu = all_cpu_data[policy->cpu];
>>  
>> -	cpu->perf_ctrls.desired_perf = target_freq;
>> +	cpu->perf_ctrls.desired_perf = target_freq * policy->max / cppc_dmi_max_khz;
>>  	freqs.old = policy->cur;
>>  	freqs.new = target_freq;
>>  
>> @@ -94,8 +136,10 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>  		return ret;
>>  	}
>>  
>> -	policy->min = cpu->perf_caps.lowest_perf;
>> -	policy->max = cpu->perf_caps.highest_perf;
>> +	cppc_dmi_max_khz = cppc_get_dmi_max_khz();
>> +
>> +	policy->min = cpu->perf_caps.lowest_perf * cppc_dmi_max_khz / cpu->perf_caps.highest_perf;
>> +	policy->max = cppc_dmi_max_khz;
>>  	policy->cpuinfo.min_freq = policy->min;
>>  	policy->cpuinfo.max_freq = policy->max;
>>  	policy->shared_type = cpu->shared_type;
>> @@ -112,7 +156,8 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>  	cpu->cur_policy = policy;
>>  
>>  	/* Set policy->cur to max now. The governors will adjust later. */
>> -	policy->cur = cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf;
>> +	policy->cur = cppc_dmi_max_khz;
>> +	cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf;
>>  
>>  	ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
>>  	if (ret)
>> -- 
>> 2.7.4
> 

Another gentle ping -- any comments?  Can this get pulled in now?

Thanks.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@xxxxxxxxxx
-----------------------------------