Re: [PATCH 03/14] cpufreq/amd-pstate: Move perf values into a union

[Mario Limonciello <superm1@kernel.org>]

On 2/12/2025 00:31, Dhananjay Ugwekar wrote:
> On 2/12/2025 3:44 AM, Mario Limonciello wrote:
>> On 2/10/2025 07:38, Dhananjay Ugwekar wrote:
>>> On 2/7/2025 3:26 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.
>>>>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
> [Snip]
>>>>      static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>>>>                      unsigned int target_freq, bool fast_switch)
>>>>    {
>>>>        struct cpufreq_freqs freqs;
>>>> -    struct amd_cpudata *cpudata = policy->driver_data;
>>>> +    struct amd_cpudata *cpudata;
>>>> +    union perf_cached perf;
>>>>        u8 des_perf;
>>>>          amd_pstate_update_min_max_limit(policy);
>>>>    +    cpudata = policy->driver_data;
>>>
>>> Any specific reason why we moved this dereferencing after amd_pstate_update_min_max_limit() ?
>>
>> Closer to the first use.
>>
>>>
>>>> +    perf = READ_ONCE(cpudata->perf);
>>>> +
>>>>        freqs.old = policy->cur;
>>>>        freqs.new = target_freq;
>>>>    -    des_perf = freq_to_perf(cpudata, target_freq);
>>>> +    des_perf = freq_to_perf(perf, cpudata->nominal_freq, target_freq);
>>>
>>> Personally I preferred the earlier 2 argument format for the helper functions, as the helper
>>> function handled the common dereferencing part, (i.e. cpudata->perf and cpudata->nominal_freq)
>>
>> Something like this?
>>
>> static inline u8 freq_to_perf(struct amd_cpudata *cpudata, unsigned int freq_val)
>> {
>>      union perf_cached perf = READ_ONCE(cpudata->perf);
>>      u8 perf_val = DIV_ROUND_UP_ULL((u64)freq_val * perf.nominal_perf, cpudata->nominal_freq);
>>
>>      return clamp_t(u8, perf_val, perf.lowest_perf, perf.highest_perf);
>> }
>>
>> As an example in practice of what that turns into with inline code it should be:
>>
>> static void amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
>> {
>>      struct amd_cpudata *cpudata = policy->driver_data;
>>      union perf_cached perf = READ_ONCE(cpudata->perf);
>>      union perf_cached perf2 = READ_ONCE(cpudata->perf);
>>      union perf_cached perf3 = READ_ONCE(cpudata->perf);
>>      u8 val1 = DIV_ROUND_UP_ULL((u64)policy->max * perf2.nominal_perf, cpudata->nominal_freq);
>>      u8 val2 = DIV_ROUND_UP_ULL((u64)policy->min * perf2.nominal_perf, cpudata->nominal_freq);
>>
>>      perf.max_limit_perf = clamp_t(u8, val1, perf2.lowest_perf, perf2.highest_perf);
>>      perf.min_limit_perf = clamp_t(u8, val2, perf3.lowest_perf, perf3.highest_perf);
>> .
>> .
>> .
>>
>> So now that's 3 reads for cpudata->perf in every use.
> 
> Yea, right, its a tradeoff, in clean looking code vs less computations.
> I'll leave it upto you, I'm okay either way.
> 

OK - I think I'll leave it like it is now for the next spin, and let 
Gautham be the tie breaker when he reviews it if he doesn't like it.