linux-kernel - Re: [PATCH v3 04/18] cpufreq/amd-pstate: Move perf values into a union

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] [day] [month] [year] [list]

Message-ID: <556c0bc3-2c52-4464-997c-4ac40b62fafa@amd.com>
Date: Tue, 25 Feb 2025 09:58:43 +0530
From: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
To: Mario Limonciello <superm1@...nel.org>,
 "Gautham R . Shenoy" <gautham.shenoy@....com>,
 Perry Yuan <perry.yuan@....com>
Cc: "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
 <linux-kernel@...r.kernel.org>,
 "open list:CPU FREQUENCY SCALING FRAMEWORK" <linux-pm@...r.kernel.org>,
 Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v3 04/18] cpufreq/amd-pstate: Move perf values into a
 union

On 2/25/2025 5:59 AM, Mario Limonciello wrote:
> On 2/19/2025 04:57, Dhananjay Ugwekar wrote:
>> On 2/18/2025 3:36 AM, Mario Limonciello wrote:
>>> From: Mario Limonciello <mario.limonciello@....com>
>>>
>>> By storing perf values in a union all the writes and reads can
>>> be done atomically, removing the need for some concurrency protections.
>>>
>>> While making this change, also drop the cached frequency values,
>>> using inline helpers to calculate them on demand from perf value.
>>>
>>> Reviewed-by: Gautham R. Shenoy <gautham.shenoy@....com>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>> ---
>>> v3:
>>>   * Pick up tag
>>> v2:
>>>   * cache perf variable in unit tests
>>>   * Drop unnecessary check from amd_pstate_update_min_max_limit()
>>>   * Consistency with READ_ONCE()
>>>   * Drop unneeded policy checks
>>>   * add kdoc
>>> ---
>>>   drivers/cpufreq/amd-pstate-ut.c |  18 +--
>>>   drivers/cpufreq/amd-pstate.c    | 195 ++++++++++++++++++--------------
>>>   drivers/cpufreq/amd-pstate.h    |  49 +++++---
>>>   3 files changed, 151 insertions(+), 111 deletions(-)
>>>
[Snip]
>>> + *          calculated wrongly. we take the fixed value as the highest_perf.
>>> + * @nominal_perf: the maximum sustained performance level of the processor,
>>> + *          assuming ideal operating conditions
>>> + * @lowest_nonlinear_perf: the lowest performance level at which nonlinear power
>>> + *               savings are achieved
>>> + * @lowest_perf: the absolute lowest performance level of the processor
>>> + * @min_limit_perf: Cached value of the performance corresponding to policy->min
>>> + * @max_limit_perf: Cached value of the performance corresponding to policy->max
>>> + */
>>> +union perf_cached {
>>> +    struct {
>>> +        u8    highest_perf;
>>> +        u8    nominal_perf;
>>> +        u8    lowest_nonlinear_perf;
>>> +        u8    lowest_perf;
>>> +        u8    min_limit_perf;
>>> +        u8    max_limit_perf;
>>
>> Just a thought, how about adding the "u8 desired_perf" (last requested) and "u8 prefcore_ranking"
>> in this. We can pursue it as a separate patch if you want.
>>
>> I think there is value in adding desired_perf atleast, so that when we are caching the
>> min, max limits in the perf_cached variable, desired perf level is also atomically
>> updated into the shared cpudata structure.
> 
> Can you see if there is any performance advantage to caching these?
> If there is, can you please do a follow up to my v5 series?

There might not be a performance advantage, but I thought it will tie up 
the entire perf updation (min, max, des) into one atomic write to perf_cached.
But the min, max and des_perf updation is divided into different functions 
currently. So it may not work as I'm imagining.

> 
> It's going to mean another write in amd_pstate_update() potentially.

Yea, right, I'll investigate and see if it is worth doing.

> 
>>
>> Thanks,
>> Dhananjay