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: <CABqG17i93frSsRp4wHCPZSNn-yW9sookHvTy6qCExZXKs2D3gA@mail.gmail.com>
Date: Fri, 20 Dec 2024 01:45:13 +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: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.
>
> 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,
> >