Re: [PATCH v2 2/2] cpufreq: Wire-up arch-flavored freq info into cpufreq_verify_current_freq

[Ionela Voinescu]

Hi Sumit,

On Friday 01 Dec 2023 at 18:32:10 (+0530), Sumit Gupta wrote:
> Hi Ionela,
> 
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -1756,7 +1756,8 @@ static unsigned int cpufreq_verify_current_freq(struct cpufreq_policy *policy, b
> > >   {
> > >        unsigned int new_freq;
> > > 
> > > -     new_freq = cpufreq_driver->get(policy->cpu);
> > > +     new_freq = arch_freq_get_on_cpu(policy->cpu);
> > > +     new_freq = new_freq ?: cpufreq_driver->get(policy->cpu);
> > 
> > Given that arch_freq_get_on_cpu() is an average frequency, it does not
> > seem right to me to trigger the sync & update process of
> > cpufreq_verify_current_freq() based on it.
> > 
> > cpufreq_verify_current_freq() will at least modify the internal state of
> > the policy and send PRE and POST notifications, if not do a full frequency
> > update, based on this average frequency, which is likely different from
> > the current frequency, even beyond the 1MHz threshold.
> > 
> > While I believe it's okay to return this average frequency in
> > cpuinfo_cur_freq, I don't think it should be used as an indication of
> > an accurate current frequency, which is what
> > cpufreq_verify_current_freq() expects.
> > 
> > Sumit, can you give more details on the issue at [1] and why this change
> > fixes it?
> > 
> > [1] https://lore.kernel.org/lkml/6a5710f6-bfbb-5dfd-11cd-0cd02220cee7@xxxxxxxxxx/
> > 
> > Thank you,
> > Ionela.
> > 
> cpufreq_verify_current_freq() also updates 'policy->cur' in POST
> notification if the frequency from hardware has more delta (out of sync).
> 
> As the value from 'cpufreq_driver->get()' is not reliable due to [1],
> calling the 'get' hook can update the 'policy->cur' with a wrong value when
> governor starts in cpufreq_start_governor().
> And if the frequency is never changed after the governor starts during
> boot e.g. when performance governor is set as default, then
> 'scaling_cur_freq' always returns wrong value.
> 
> Instead, the arch_freq_get_on_cpu() API updates 'policy->cur' with a more
> stable freq value.
> 
> [1] https://lore.kernel.org/lkml/20230418113459.12860-7-sumitg@xxxxxxxxxx/

Got it, many thanks! 

As the code is right now in v2, arch_freq_get_on_cpu() is called on
show_scaling_cur_freq(), so the problem you describe would not show up.
policy->cur would still be incorrect, but 'scaling_cur_freq' would
return the value from arch_freq_get_on_cpu().

Would it be enough if arch_freq_get_on_cpu() gets also called from
show_cpuinfo_cur_freq() instead of cpufreq_verify_current_freq()?

Thanks,
Ionela.

> 
> Best regards,
> Sumit Gupta
> 
> > >        if (!new_freq)
> > >                return 0;
> > > 
> > > --
> > > 2.25.1
> > >