Re: [PATCH v6 3/4] arm64: Provide an AMU-based version of arch_freq_get_on_cpu

[Jie Zhan]

On 26/08/2024 15:23, Beata Michalska wrote:
> On Wed, Aug 14, 2024 at 02:46:16PM +0800, Jie Zhan wrote:
> > Hi Beata,
> Hi Jie,
> > On 03/06/2024 16:21, Beata Michalska wrote:
> > > 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 current 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 | 110 +++++++++++++++++++++++++++++++----
> > >    1 file changed, 100 insertions(+), 10 deletions(-)
> > ...
> > > +
> > > +#define AMU_SAMPLE_EXP_MS	20
> > > +
> > > +unsigned int arch_freq_get_on_cpu(int cpu)
> > > +{
> > > +	struct amu_cntr_sample *amu_sample;
> > > +	unsigned int start_cpu = cpu;
> > > +	unsigned long last_update;
> > > +	unsigned int freq = 0;
> > > +	u64 scale;
> > > +
> > > +	if (!amu_fie_cpu_supported(cpu) || !arch_scale_freq_ref(cpu))
> > > +		return 0;
> > > +
> > > +retry:
> > > +	amu_sample = per_cpu_ptr(&cpu_amu_samples, cpu);
> > > +
> > > +	last_update = amu_sample->last_update;
> > > +
> > > +	/*
> > > +	 * For those CPUs that are in full dynticks mode,
> > > +	 * and those that have not seen tick for a while
> > > +	 * try an alternative source for the counters (and thus freq scale),
> > > +	 * if available, for given policy:
> > > +	 * this boils down to identifying an active cpu within the same freq
> > > +	 * domain, if any.
> > > +	 */
> > > +	if (!housekeeping_cpu(cpu, HK_TYPE_TICK) ||
> > > +	    time_is_before_jiffies(last_update + msecs_to_jiffies(AMU_SAMPLE_EXP_MS))) {
> > One question here.
> >
> > The 2nd condition, providing the addtional code in patch 4, would be:
> > (!idle_cpu(cpu) && time_is_before_jiffies(last_update +
> > msecs_to_jiffies(AMU_SAMPLE_EXP_MS)))
> > That means trying another cpu in the same freq domain if this cpu is running
> > and not having a tick recently.
> >
> > In this case, if it fails to find an alternative cpu in the following code,
> > can it just jump to the calculation
> > part and return an 'old' frequency rather than return 0?
> > The freq here won't be older than the freq when the cpu went idle last time
> > -- yet the freq before last idle
> > is returned if the cpu were idle (patch 4).
> To be fair, I will be dropping the idle_cpu check from this condition so that
> we do keep the cap on the validity of the scale factor: meaning if the cpu
> remains idle for longer than assumed 20ms we will either look for alternative
> or return 0. I'm not entirely convinced returning somewhat stale information on
> the freq for CPUs that remain idle for a while will be useful.
>
> One note here: as per discussion in [1] this functionality will be moved to new
> sysfs attribute so it will no longer be used via scaling_cur_freq.
>
>
> > > +		struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> > > +		int ref_cpu = cpu;
> > > +
> > > +		if (!policy)
> > > +			goto leave;
> > > +
> > > +		if (!policy_is_shared(policy)) {
> > > +			cpufreq_cpu_put(policy);
> > > +			goto leave;
> > > +		}
> > > +
> > > +		do {
> > > +			ref_cpu = cpumask_next_wrap(ref_cpu, policy->cpus,
> > > +						    start_cpu, false);
> > start_cpu is only used here. looks like we can s/start_cpu/cpu/ and remove
> > its definition above?
> It is indeed but it is needed for the cpumask_next_wrap to know when to stop
> the iteration after wrapping.
>
> ---
> [1] https://lore.kernel.org/all/20240603081331.3829278-1-beata.michalska@xxxxxxx/
> ---
>
> BR
> Beata
Sure, thanks. Looking forward to the next version.

Jie