Re: [PATCH v3 10/15] cpufreq/amd-pstate: Move limit updating code

["Gautham R. Shenoy" <gautham.shenoy@amd.com>]

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.