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: <CABqG17hUvdo4ShQzWPWpfNqJFiDCDHmApPnHahtTt83B1Thg9Q@mail.gmail.com>
Date: Fri, 20 Dec 2024 01:40:54 +0530
From: Naresh Solanki <naresh.solanki@...ements.com>
To: Mario Limonciello <mario.limonciello@....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

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.
After fixing that, it reported frequency 2002 & 1404 depending on the core.

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.

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,
>