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

[Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com>]

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@amd.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@amd.com>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.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