Re: [PATCH] arm64: topology: Cleanup init_amu_fie() a bit

[Ionela Voinescu]

Hi,

Sorry, I missed this.

On Friday 11 Dec 2020 at 16:35:55 (+0530), Viresh Kumar wrote:
> On 10-12-20, 10:38, Ionela Voinescu wrote:
> > Basically, that's functions purpose is only to make sure that invariance
> > at the level of the policy is consistent: either all CPUs in a policy
> > support counters and counters will be used for the scale factor, or
> > either none or only some support counters and therefore the default
> > cpufreq implementation will be used (arch_set_freq_scale()) for all CPUs
> > in a policy.
> 
> Why is it important to have this thing at policy level ? If we are
> okay with only one policy having AMUs and not the other one, then what
> about only some CPUs of both the policies having it. Does it break
> anything ?
> 

First of all, in order for a single CPU in a policy to use AMUs for FIE,
when the others do not support AMUs, we'd have to modify
arch_set_freq_scale() which otherwise would uniformly set its own
computed scale factor for all related CPUs.

Beyond this, it's very likely that CPUs in the same policy have the same
uarch and therefore either all or none in the policy would support AMUs.
The check here is just to make sure of that and then ensure that
arch_set_freq_scale() and arch_scale_freq_tick() do not clash with each
other.

Beyond this, it's difficult to say how important it is to have the same
scale for all CPUs in a policy. AMUs would give you a scale based on an
average frequency between ticks, and more importantly it would be based
on the actual core frequency, not on what software requested. Cpufreq
would give you the scale obtained using the frequency that the OS
requested. Therefore, the two scale values could end up being quite
different, and therefore can result in quite different utilization
values for CPUs doing the same work.

An alternative would be to apply the AMU computed scale to all CPUs
although not all CPUs in a policy might support AMUs. But given that
this scenario is unlikely, the added hassle in arch_scale_freq_tick()
which would involve getting information on related CPUs from policies
was not worth it, in my opionion.

Thanks,
Ionela.

> -- 
> viresh