Message-ID: <evllzzjufritriziohz22gm5qs44jedpft5lcfjfwnekarqtdx@eg42ik2l7bxu>
Date: Fri, 11 Oct 2024 09:29:44 -0700
From: Vanshidhar Konda <vanshikonda@...amperecomputing.com>
To: Beata Michalska <beata.michalska@....com>
Cc: viresh.kumar@...aro.org, Sumit Gupta <sumitg@...dia.com>, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org, 
	ionela.voinescu@....com, sudeep.holla@....com, will@...nel.org, catalin.marinas@....com, 
	rafael@...nel.org, yang@...amperecomputing.com, lihuisong@...wei.com, 
	zhanjie9@...ilicon.com, linux-tegra <linux-tegra@...r.kernel.org>, 
	Bibek Basu <bbasu@...dia.com>
Subject: Re: [PATCH v7 3/4] arm64: Provide an AMU-based version of
 arch_freq_avg_get_on_cpu

On Thu, Oct 10, 2024 at 01:08:23PM GMT, Beata Michalska wrote:
>On Thu, Oct 03, 2024 at 02:54:22PM -0700, Vanshidhar Konda wrote:
>> 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@....com>
>> > > > > > Signed-off-by: Beata Michalska <beata.michalska@....com>
>> > > > > > ---
>> > > > > >   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.
>>
>That is a fair point but I am not entirely convinced using '0' instead makes
>things any more clearer as this is in no way a valid CPU frequency.
>As long as we document the expected behaviour keeping the interface well
>defined,  both options should be fine I guess.
>

Another option could be to list out the reason as 'idle' or 'no entry' instead of
returning EINVAL or ENOENT. These wouldn't be valid values either but cat on the
sysfs node wouldn't return an error.

Thanks,
Vanshidhar

>@Viresh: what is your opinion on that one ?
>
>---
>BR
>Beata
>> 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
>> > > > > >

Powered by blists - more mailing lists