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

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

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.