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

[Jie Zhan <zhanjie9@hisilicon.com>]

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@arm.com>
>>> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
>>> ---
>>>    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@arm.com/
> ---
>
> BR
> Beata
>
Sure, thanks. Looking forward to the next version.

Jie