Re: [PATCH v6 0/4] Add support for AArch64 AMUv1-based arch_freq_get_on_cpu

[Beata Michalska <beata.michalska@arm.com>]

On Wed, Aug 14, 2024 at 04:05:24PM +0800, Jie Zhan wrote:
> 
> On 11/07/2024 21:59, Beata Michalska wrote:
> > Hi Catalin,
> > 
> > On Mon, Jul 08, 2024 at 06:10:02PM +0100, Catalin Marinas wrote:
> > > Hi Beata,
> > > 
> > > On Mon, Jun 03, 2024 at 09:21:50AM +0100, Beata Michalska wrote:
> > > > Introducing arm64 specific version of arch_freq_get_on_cpu, cashing on
> > > > existing implementation for FIE and AMUv1 support: the frequency scale
> > > > factor, updated on each sched tick, serves as a base for retrieving
> > > > the frequency for a given CPU, representing an average frequency
> > > > reported between the ticks - thus its accuracy is limited.
> > > > 
> > > > The changes have been rather lightly (due to some limitations) tested on
> > > > an FVP model. Note that some small discrepancies have been observed while
> > > > testing (on the model) and this is currently being investigated, though it
> > > > should not have any significant impact on the overall results.
> > > What's the plan with this series? Are you still investigating those
> > > discrepancies or is it good to go?
> > > 
> > Overall it should be good to go with small caveat:
> > as per discussion [1] we might need to provide new sysfs attribute exposing an
> > average frequency instead of plugging new code under existing cpuinfo_cur_freq.
> > This is to avoid messing up with other archs and make a clean distinction on
> > which attribute provides what information.
> > As such, the arch_freq_get_on_cpu implementation provided within this series
> > [PATCH v6 3/4] will most probably be shifted to a new function.
> > 
> > Hopefully will be able to send those changes soon.
> > 
> > ---
> > [1] https://lore.kernel.org/all/ZmrB_DqtmVpvG30l@arm.com/
> > ---
> > BR
> > Beata
> > 
> > > -- 
> > > Catalin
> > > 
> Hi Beata,
Hi Jie,
> 
> I've recently tested this patchset on a Kunpeng system.
> It works as expected when reading scaling_cur_freq.
> The frequency samples are much stabler than what cppc_cpufreq returns.
Thank you for giving it a spin.
(and apologies for late reply)
> 
> A few minor things.
> 
> 1. I observed larger errors on idle cpus than busy cpus, though it's just up
> to 1%.
> Not sure if this comes from the uncertain time interval between the last
> tick and entering idle.
> The shorter averaging interval, the larger error, I supposed.
All right - will look into it.
Just for my benefit: that diff is strictly between arch_freq_avg_get_on_cpu
and cpufreq_driver->get(policy->cpu) ?
> 
> 2. In the current implementation, the resolution of frequency would be:
> max_freq_khz / SCHED_CAPACITY_SCALE
> This looks a bit unnecessary to me.
> 
> It's supposed to get a better resolution if we can do this in
> arch_freq_get_on_cpu():
> 
> freq = delta_cycle_cnt * max_freq_khz / delta_const_cnt
> 
> which may require caching both current and previous sets of counts in the
> per-cpu struct amu_cntr_sample.
> 
arch_freq_get_on_cpu relies on the frequency scale factor to derive the average
frequency. The scale factor is being calculated based on the deltas you have
mentioned and arch_max_freq_scale which uses SCHED_CAPACITY_SCALE*2 factor to
accommodate for rather low reference frequencies. arch_freq_get_on_cpu just does
somewhat reverse computation to that.

---
BR
Beata
> Kind regards,
> Jie
>