linux-kernel - Re: [PATCH v2] cpufreq/amd-pstate: Refactor max frequency calculation

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: <754bdf5a-8047-47c3-bacc-238d2d2a593f@amd.com>
Date: Thu, 19 Dec 2024 15:08:20 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Naresh Solanki <naresh.solanki@...ements.com>
Cc: "Gautham R. Shenoy" <gautham.shenoy@....com>,
 Perry Yuan <perry.yuan@....com>, Huang Rui <ray.huang@....com>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>, linux-pm@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] cpufreq/amd-pstate: Refactor max frequency calculation

On 12/19/2024 14:15, Naresh Solanki wrote:
> Hi Mario,
> 
> On Fri, 20 Dec 2024 at 01:40, Naresh Solanki
> <naresh.solanki@...ements.com> wrote:
>>
>> Hi Mario,
>>
>> On Fri, 20 Dec 2024 at 01:02, Mario Limonciello
>> <mario.limonciello@....com> wrote:
>>>
>>> On 12/19/2024 13:21, Naresh Solanki wrote:
>>>> The previous approach introduced roundoff errors during division when
>>>> calculating the boost ratio. This, in turn, affected the maximum
>>>> frequency calculation, often resulting in reporting lower frequency
>>>> values.
>>>>
>>>> For example, on the Glinda SoC based board with the following
>>>> parameters:
>>>>
>>>> max_perf = 208
>>>> nominal_perf = 100
>>>> nominal_freq = 2600 MHz
>>>>
>>>> The Linux kernel previously calculated the frequency as:
>>>> freq = ((max_perf * 1024 / nominal_perf) * nominal_freq) / 1024
>>>> freq = 5405 MHz  // Integer arithmetic.
>>>>
>>>> With the updated formula:
>>>> freq = (max_perf * nominal_freq) / nominal_perf
>>>> freq = 5408 MHz
>>>>
>>>> This change ensures more accurate frequency calculations by eliminating
>>>> unnecessary shifts and divisions, thereby improving precision.
>>>>
>>>> Signed-off-by: Naresh Solanki <naresh.solanki@...ements.com>
>>>
>>> Thanks, this makes sense to me.
>>>
>>> But looking at it, we should have the same problem with lowest nonlinear
>>> freq as it goes through the same conversion process.  Would you mind
>>> fixing that one too?
>> Sure. Somehow my eyes missed that.
>> Also observed that current calculations yields zero for lowest_nonlinear_freq.
> Sorry I was wrong. it's not zero. Its roundoff version.
> 
>> After fixing that, it reported frequency 2002 & 1404 depending on the core.

Mmm, I wouldn't expect that.  Is your APU heterogenous?  Or is this a 
BIOS bug?

Both with your v3 patch in place and your patch not in place can you 
send me the report generated from:
https://gitlab.freedesktop.org/drm/amd/-/blob/master/scripts/amd-pstate-triage.py

>>
>> On a side note, I'm also observing that the highest_perf is set to 196 which
>> should not have been the case as I do have cores with value 208.
>> Seems like amd_get_boost_ratio_numerator needs some addressing for that.

Ah this is something that is quite confusing about how AMD client CPUs 
behave.  There is a feature called "Preferred cores" that uses the 
highest performance value to indicate rankings of the cores.  This means 
that you can't use the value in this register to calculate the 
frequency, you have to use the pre-defined scaling factor.

The scaling factor is listed in arch/x86/kernel/acpi/cppc.c and the 
correct one is fetched for this calculation.

>>
>> Regards,
>> Naresh
>>>
>>> Gautham, Perry,
>>>
>>> Is there something non-obvious I'm missing about why it was done this
>>> way?  It looks like it's been there since the start.
>>>
>>>>
>>>> Changes in V2:
>>>> 1. Rebase on superm1.git/linux-next branch
>>>> ---
>>>>    drivers/cpufreq/amd-pstate.c | 9 ++++-----
>>>>    1 file changed, 4 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>>>> index d7b1de97727a..02a851f93fd6 100644
>>>> --- a/drivers/cpufreq/amd-pstate.c
>>>> +++ b/drivers/cpufreq/amd-pstate.c
>>>> @@ -908,9 +908,9 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>>>>    {
>>>>        int ret;
>>>>        u32 min_freq, max_freq;
>>>> -     u32 nominal_perf, nominal_freq;
>>>> +     u32 highest_perf, nominal_perf, nominal_freq;
>>>>        u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
>>>> -     u32 boost_ratio, lowest_nonlinear_ratio;
>>>> +     u32 lowest_nonlinear_ratio;
>>>>        struct cppc_perf_caps cppc_perf;
>>>>
>>>>        ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
>>>> @@ -927,10 +927,9 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>>>>        else
>>>>                nominal_freq = cppc_perf.nominal_freq;
>>>>
>>>> +     highest_perf = READ_ONCE(cpudata->highest_perf);
>>>>        nominal_perf = READ_ONCE(cpudata->nominal_perf);
>>>> -
>>>> -     boost_ratio = div_u64(cpudata->highest_perf << SCHED_CAPACITY_SHIFT, nominal_perf);
>>>> -     max_freq = (nominal_freq * boost_ratio >> SCHED_CAPACITY_SHIFT);
>>>> +     max_freq = div_u64((u64)highest_perf * nominal_freq, nominal_perf);
>>>>
>>>>        lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
>>>>        lowest_nonlinear_ratio = div_u64(lowest_nonlinear_perf << SCHED_CAPACITY_SHIFT,
>>>