[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z2Ofggrpkv/rxlJp@BLRRASHENOY1.amd.com>
Date: Thu, 19 Dec 2024 09:52:26 +0530
From: "Gautham R. Shenoy" <gautham.shenoy@....com>
To: Mario Limonciello <mario.limonciello@....com>
Cc: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>,
Perry Yuan <perry.yuan@....com>, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 10/15] cpufreq/amd-pstate: Move limit updating code
On Tue, Dec 17, 2024 at 01:44:47PM -0600, Mario Limonciello wrote:
[..snip..]
> > > > > >
> > > > > > Please let me know your thoughts on this.
> > > > > >
> > > > > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > > > > > index d7b1de97727a..1ac34e3f1fc5 100644
> > > > > > --- a/drivers/cpufreq/amd-pstate.c
> > > > > > +++ b/drivers/cpufreq/amd-pstate.c
> > > > > > @@ -699,7 +699,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
> > > > > > if (min_perf < lowest_nonlinear_perf)
> > > > > > min_perf = lowest_nonlinear_perf;
> > > > here^^^
> > > > > >
> > > > > > - max_perf = cap_perf;
> > > > > > + max_perf = cpudata->max_limit_perf;
> > > > > > if (max_perf < min_perf)
> > > > > > max_perf = min_perf;
> > > > >
> > > > > With this change I think you can also drop the comparison afterwards, as an optimization right?
> > > >
> > > > Umm I think it is possible that scaling_max_freq is set to a value lower than
> > > > lowest_nonlinear_freq in that case this if condition would be needed (as min_perf
> > > > is being lower bounded at lowest_nonlinear_freq at the location highlighted above).
> > > > I would be okay with keeping this check in.
> > >
> > > Well this feels like a bigger problem actually - why is it forcefully bounded at lowest nonlinear freq? Performance is going to be awful at that level
> >
> > Actually this wont necessarily deteriorate the performance, as we are just restricting
> > the min_perf to not go below lowest_nonlinear level. So we are actually ensuring that
> > the schedutil doesnt select a des_perf below lowest_nonlinear_perf.
> >
> > (hence why commit 5d9a354cf839a ("cpufreq/amd-pstate: Set the initial min_freq to lowest_nonlinear_freq") was done),
>
> Sorry re-reading I didn't get my thought out properly, I meant to say
> performance is going to be bad BELOW that level. We're in total agreement
> here.
>
> > >
> > > but shouldn't we "let" people go below that in passive and guided? We do for active.
> >
> > Yes I agree, we should allow the user to set min limit in the entire frequency range,
> > I thought there would've been some reason for restricting this. But I dont see any
> > reasoning for this in the blamed commit log as well. I think one reason would be that
> > below lowest_nonlinear_freq we dont get real power savings. And schedutil might dip
> > into this lower inefficient range if we dont force bound it.
If I were to venture a guess, the real reason could to be have been to
gain parity with acpi_cpufreq + schedutil where the lowest frequency
would not go below P2. Nonlinear frequency was picked as a lower limit
because it approximated that. It may have been as simple as that so
that the out-of-box behavior with the schedutil governor wasn't
suboptimal.
However after Dhananjay's patches where the min_perf is set to
lowest_non_linear_perf by default, I don't think an additional capping
of min_perf is needed.
--
Thanks and Regards
gautham.
Powered by blists - more mailing lists