linux-kernel - Re: [PATCH v2 2/2] cpufreq/amd-pstate: Set the initial min_freq to lowest_nonlinear

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <667844b6-fa28-4648-90ba-8eed0c9a3491@amd.com>
Date: Thu, 17 Oct 2024 09:59:59 +0530
From: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
To: "Gautham R. Shenoy" <gautham.shenoy@....com>
Cc: mario.limonciello@....com, perry.yuan@....com, rafael@...nel.org,
 viresh.kumar@...aro.org, linux-pm@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] cpufreq/amd-pstate: Set the initial min_freq to
 lowest_nonlinear_freq

Hello Gautham,

On 10/17/2024 9:26 AM, Gautham R. Shenoy wrote:
> Hello Dhananjay,
> 
> On Wed, Oct 16, 2024 at 02:46:42PM +0000, Dhananjay Ugwekar wrote:
>> According to the AMD architectural programmer's manual volume 2 [1], in
>> section "17.6.4.1 CPPC_CAPABILITY_1" lowest_nonlinear_perf is described
>> as "Reports the most energy efficient performance level (in terms of
>> performance per watt). Above this threshold, lower performance levels
>> generally result in increased energy efficiency. Reducing performance
>> below this threshold does not result in total energy savings for a given
>> computation, although it reduces instantaneous power consumption". So
>> lowest_nonlinear_perf is the most power efficient performance level, and
>> going below that would lead to a worse performance/watt.
>>
>> Also, setting the minimum frequency to lowest_nonlinear_freq (instead of
>> lowest_freq) allows the CPU to idle at a higher frequency which leads
>> to more time being spent in a deeper idle state (as trivial idle tasks
>> are completed sooner). This has shown a power benefit in some systems,
>> in other systems, power consumption has increased but so has the
>> throughput/watt.
>>
>> Modify the initial policy_data->min passed by cpufreq core to
>> lowest_nonlinear_freq, in the ->verify() callback. Also set the
>> qos_request cpudata->req[0] to FREQ_QOS_MIN_DEFAULT_VALUE (i.e. 0), 
>> so that it also gets overridden by the check in verify function.
>>
>> Link: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf [1]
>>
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
>> ---
>>  drivers/cpufreq/amd-pstate.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index fa16d72d6058..117ad5988e8e 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -529,8 +529,20 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>>  
>>  static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>>  {
>> +	struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
>> +	struct amd_cpudata *cpudata = policy->driver_data;
>> +
>> +	if (!policy)
>> +		return -EINVAL;
>> +
>> +	if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE)
>> +		policy_data->min = cpudata->lowest_nonlinear_freq;
> 
> Why not unconditionally set policy->min to lowest_nonlinear_freq ?

That will lead to discarding all of the user's writes to scaling_min_freq and the lowest_nonlinear_freq 
will become the permanent policy->min value. Because, verify() is called in the scaling_min_freq write 
path as well. refresh_frequency_limits() --> cpufreq_set_policy() --> driver->verify().

> 
> 
>> +
>>  	cpufreq_verify_within_cpu_limits(policy_data);
>>  	pr_debug("policy_max =%d, policy_min=%d\n", policy_data->max, policy_data->min);
>> +
>> +	cpufreq_cpu_put(policy);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -996,7 +1008,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>>  		policy->fast_switch_possible = true;
>>  
>>  	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
>> -				   FREQ_QOS_MIN, policy->cpuinfo.min_freq);
>> +				   FREQ_QOS_MIN, FREQ_QOS_MIN_DEFAULT_VALUE);
> 
> 
> This qos request can still be set to cpuinfo.min_freq, no ? Especially
> if you unconditionally initialize policy->min to lowest_nonlinear_freq
> in amd_pstate_policy, no?

As we cant unconditionally init the policy->min above, this needs to be set accordingly for the 
above if condition to be true (i.e. FREQ_QOS_MIN_DEFAULT_VALUE).

Thanks,
Dhananjay

> 
> --
> Thanks and Regards
> gautham.