Re: [PATCH v7 3/4] arm64: Provide an AMU-based version of arch_freq_avg_get_on_cpu
From: Vanshidhar Konda
Date: Thu Oct 03 2024 - 17:54:38 EST
On Thu, Oct 03, 2024 at 11:39:54PM GMT, Beata Michalska wrote:
On Thu, Sep 26, 2024 at 04:21:14PM -0700, Vanshidhar Konda wrote:
On Thu, Sep 26, 2024 at 12:34:01PM GMT, Beata Michalska wrote:
> On Tue, Sep 17, 2024 at 05:41:09PM +0530, Sumit Gupta wrote:
> > Hi Beata,
> Hi Sumit,
> >
> > Thank you for the patches.
> Thank you for having a look at those.
> >
> > On 13/09/24 18:59, Beata Michalska wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > With the Frequency Invariance Engine (FIE) being already wired up with
> > > sched tick and making use of relevant (core counter and constant
> > > counter) AMU counters, getting the average frequency for a given CPU,
> > > can be achieved by utilizing the frequency scale factor which reflects
> > > an average CPU frequency for the last tick period length.
> > >
> > > The solution is partially based on APERF/MPERF implementation of
> > > arch_freq_get_on_cpu.
> > >
> > > Suggested-by: Ionela Voinescu <ionela.voinescu@xxxxxxx>
> > > Signed-off-by: Beata Michalska <beata.michalska@xxxxxxx>
> > > ---
> > > arch/arm64/kernel/topology.c | 109 +++++++++++++++++++++++++++++++----
> > > 1 file changed, 99 insertions(+), 10 deletions(-)
> > >
--- snip ----
> >
> > ..
> > freq_comput:
> > scale = arch_scale_freq_capacity(cpu);
> > freq = scale * arch_scale_freq_ref(cpu);
> > ----
> >
> This boils down to the question what that function, and the information it
> provides, represent really. The 'unknown' here simply says the CPU has been idle
> for a while and as such the frequency data is a bit stale and it does not
> represent the average freq the CPU is actually running at anymore, which is
> the intention here really. Or, that the given CPU is a non-housekeeping one.
> Either way I believe this is a useful information, instead of providing
> stale data with no indication on whether the frequency is really the 'current'
> one or not.
>
> If that is somehow undesirable we can discuss this further, though I'd rather
> avoid exposing an interface where the feedback provided is open to
> interpretation at all times.
Would it make sense to identify that the frequency reporting is unknown due to
cpu being idle vs some other issue like being a non-housekeeping CPU? Would
returning a value of 0 make it easier for tools to represent that the CPU is
currently idle?
That is an option.
Another one would be to return an error for those cases. This would make it
easier to distinguish between valid frequency &/| idle CPU vs tickless CPU
(EINVAL vs ENOENT) ?
That seems like a good idea but I suspect it would be confusing to the end user.
If a user runs `cat /sys/devices/system/cpu/cpu2/cpuinfo_avg_freq` they would
get an error in some cases or get a number in some other iterations.
Thanks,
Vanshidhar
---
BR
Beata
Thanks,
Vanshidhar
>
> ---
> Best Regards
> Beata
> > Thank you,
> > Sumit Gupta
> >
> > P.S. Will be on afk for next 2 weeks with no access to email. Please expect
> > a delay in response.
> >
> > > + cpu = ref_cpu;
> > > + goto retry;
> > > + }
> > > + /*
> > > + * Reversed computation to the one used to determine
> > > + * the arch_freq_scale value
> > > + * (see amu_scale_freq_tick for details)
> > > + */
> > > + scale = arch_scale_freq_capacity(cpu);
> > > + freq = scale * arch_scale_freq_ref(cpu);
> > > + freq >>= SCHED_CAPACITY_SHIFT;
> > > + return freq;
> > > +}
> > > +
> >
> > > static void amu_fie_setup(const struct cpumask *cpus)
> > > {
> > > int cpu;
> > > --
> > > 2.25.1
> > >