[PATCH] arm64: topology: Don't support AMU without cpufreq

[PATCH] arm64: topology: Don't support AMU without cpufreq

[Viresh Kumar]

The commit cd0ed03a8903 ("arm64: use activity monitors for frequency
invariance"), mentions that:

  "if CONFIG_CPU_FREQ is not enabled, the use of counters is
   enabled on all CPUs only if all possible CPUs correctly support
   the necessary counters"

But that's not really true as validate_cpu_freq_invariance_counters()
fails if max_freq_hz is returned as 0 (in case there is no policy for
the CPU). And the AMUs won't be supported in that case.

Make the code reflect this reality.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm64/kernel/topology.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 0801a0f3c156..b7da372819fc 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -187,14 +187,13 @@ static int validate_cpu_freq_invariance_counters(int cpu)
 	return 0;
 }
 
-static inline bool
-enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
+static inline void update_amu_fie_cpus(int cpu, cpumask_var_t valid_cpus)
 {
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
 
 	if (!policy) {
 		pr_debug("CPU%d: No cpufreq policy found.\n", cpu);
-		return false;
+		return;
 	}
 
 	if (cpumask_subset(policy->related_cpus, valid_cpus))
@@ -202,8 +201,6 @@ enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
 			   amu_fie_cpus);
 
 	cpufreq_cpu_put(policy);
-
-	return true;
 }
 
 static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
@@ -212,7 +209,6 @@ static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
 static int __init init_amu_fie(void)
 {
 	cpumask_var_t valid_cpus;
-	bool have_policy = false;
 	int ret = 0;
 	int cpu;
 
@@ -228,18 +224,9 @@ static int __init init_amu_fie(void)
 		if (validate_cpu_freq_invariance_counters(cpu))
 			continue;
 		cpumask_set_cpu(cpu, valid_cpus);
-		have_policy |= enable_policy_freq_counters(cpu, valid_cpus);
+		update_amu_fie_cpus(cpu, valid_cpus);
 	}
 
-	/*
-	 * If we are not restricted by cpufreq policies, we only enable
-	 * the use of the AMU feature for FIE if all CPUs support AMU.
-	 * Otherwise, enable_policy_freq_counters has already enabled
-	 * policy cpus.
-	 */
-	if (!have_policy && cpumask_equal(valid_cpus, cpu_present_mask))
-		cpumask_or(amu_fie_cpus, amu_fie_cpus, valid_cpus);
-
 	if (!cpumask_empty(amu_fie_cpus)) {
 		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
 			cpumask_pr_args(amu_fie_cpus));
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH] arm64: topology: Don't support AMU without cpufreq

[Ionela Voinescu]

Hi Viresh,

On Thursday 09 Jul 2020 at 12:22:45 (+0530), Viresh Kumar wrote:
> The commit cd0ed03a8903 ("arm64: use activity monitors for frequency
> invariance"), mentions that:
> 
>   "if CONFIG_CPU_FREQ is not enabled, the use of counters is
>    enabled on all CPUs only if all possible CPUs correctly support
>    the necessary counters"
> 

Yes, this part of the commit message is wrong. I could go easy on myself
and say that the comment in the code is correct, but it would not make
this part of the commit message less wrong. 

So the "if CONFIG_CPU_FREQ is not enabled" should have been replaced by
"if we are not restricted by cpufreq policies", which is different,
as described below.

Comment in code:
"""
* If we are not restricted by cpufreq policies, we only enable
* the use of the AMU feature for FIE if all CPUs support AMU.
"""

> But that's not really true as validate_cpu_freq_invariance_counters()
> fails if max_freq_hz is returned as 0 (in case there is no policy for
> the CPU). And the AMUs won't be supported in that case.
> 
> Make the code reflect this reality.
> 

It seems to me that validate_cpu_freq_invariance_counters() already does
this filtering. max_freq_hz would need to have a valid value before AMUs
can be marked as supported.

Functionally, what would happen, is the following:

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  arch/arm64/kernel/topology.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 0801a0f3c156..b7da372819fc 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -187,14 +187,13 @@ static int validate_cpu_freq_invariance_counters(int cpu)
>  	return 0;
>  }
>  
> -static inline bool
> -enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
> +static inline void update_amu_fie_cpus(int cpu, cpumask_var_t valid_cpus)
>  {
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>  
>  	if (!policy) {
>  		pr_debug("CPU%d: No cpufreq policy found.\n", cpu);
> -		return false;
> +		return;
>  	}
>  
>  	if (cpumask_subset(policy->related_cpus, valid_cpus))
> @@ -202,8 +201,6 @@ enable_policy_freq_counters(int cpu, cpumask_var_t valid_cpus)
>  			   amu_fie_cpus);
>  
>  	cpufreq_cpu_put(policy);
> -
> -	return true;
>  }
>  
>  static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
> @@ -212,7 +209,6 @@ static DEFINE_STATIC_KEY_FALSE(amu_fie_key);
>  static int __init init_amu_fie(void)
>  {
>  	cpumask_var_t valid_cpus;
> -	bool have_policy = false;
>  	int ret = 0;
>  	int cpu;
>  
> @@ -228,18 +224,9 @@ static int __init init_amu_fie(void)
>  		if (validate_cpu_freq_invariance_counters(cpu))
>  			continue;

If max_freq_hz happens to be 0 (either !CONFIG_CPU_FREQ or
CONFIG_CPU_FREQ=y && !policy, etc),
validate_cpu_freq_invariance_counters(cpu) will return -EINVAL,
so we'll continue with the next CPU, without adding this cpu to
valid_cpus.

Therefore it would not be marked as a valid AMU supporting CPU, so we
won't call enable_policy_freq_counters() for it.

>  		cpumask_set_cpu(cpu, valid_cpus);
> -		have_policy |= enable_policy_freq_counters(cpu, valid_cpus);
> +		update_amu_fie_cpus(cpu, valid_cpus);

I see this as two different pieces of functionality:
 - (1) validate_cpu_freq_invariance_counters(cpu) has the job of validating
   the CPU support, including max_freq_hz.
 - (2) enable_policy_freq_counters() has the job to restrict AMU enablement
   for the CPUs in a policy if all CPUs in the policy support AMUs.

So both of them, separately, support the case of !CONFIG_CPU_FREQ.

>  	}
>  
> -	/*
> -	 * If we are not restricted by cpufreq policies, we only enable
> -	 * the use of the AMU feature for FIE if all CPUs support AMU.
> -	 * Otherwise, enable_policy_freq_counters has already enabled
> -	 * policy cpus.
> -	 */
> -	if (!have_policy && cpumask_equal(valid_cpus, cpu_present_mask))

This is meant to have the following logic: if for some reason we're not
restricted by policies (according to 2), but all AMU validation was
successful (according to 1), there is no reason not to enable fully AMU
enabled frequency invariance.

I agree that this happening is a cornercase and a reason for which
cpufreq_get_hw_max_freq() was made weak. If some platform has entirely
firmware driven frequency control, but it enables CONFIG_CPU_FREQ
(as is the default) and it defines its own cpufreq_get_hw_max_freq(),
it could benefit from AMU use.

So I did believe it was best for these checks to be decoupled, for this
reason, and potential other reasons in the future, involving more
decoupling from cpufreq.

I do have code in progress to clean the overall interaction between
cpufreq and AMUs, started at [1]. Bear with me on this, it is all
connected :).

[1]
https://lore.kernel.org/lkml/20200701090751.7543-1-ionela.voinescu@arm.com/

Regards,
Ionela.

> -		cpumask_or(amu_fie_cpus, amu_fie_cpus, valid_cpus);
> -
>  	if (!cpumask_empty(amu_fie_cpus)) {
>  		pr_info("CPUs[%*pbl]: counters will be used for FIE.",
>  			cpumask_pr_args(amu_fie_cpus));
> -- 
> 2.25.0.rc1.19.g042ed3e048af
> 

Re: [PATCH] arm64: topology: Don't support AMU without cpufreq

[Viresh Kumar]

On 09-07-20, 11:17, Ionela Voinescu wrote:
> On Thursday 09 Jul 2020 at 12:22:45 (+0530), Viresh Kumar wrote:
> >  		cpumask_set_cpu(cpu, valid_cpus);
> > -		have_policy |= enable_policy_freq_counters(cpu, valid_cpus);
> > +		update_amu_fie_cpus(cpu, valid_cpus);
> 
> I see this as two different pieces of functionality:
>  - (1) validate_cpu_freq_invariance_counters(cpu) has the job of validating
>    the CPU support, including max_freq_hz.
>  - (2) enable_policy_freq_counters() has the job to restrict AMU enablement
>    for the CPUs in a policy if all CPUs in the policy support AMUs.
> 
> So both of them, separately, support the case of !CONFIG_CPU_FREQ.
> 
> >  	}
> >  
> > -	/*
> > -	 * If we are not restricted by cpufreq policies, we only enable
> > -	 * the use of the AMU feature for FIE if all CPUs support AMU.
> > -	 * Otherwise, enable_policy_freq_counters has already enabled
> > -	 * policy cpus.
> > -	 */
> > -	if (!have_policy && cpumask_equal(valid_cpus, cpu_present_mask))
> 
> This is meant to have the following logic: if for some reason we're not
> restricted by policies (according to 2), but all AMU validation was
> successful (according to 1), there is no reason not to enable fully AMU
> enabled frequency invariance.
> 
> I agree that this happening is a cornercase and a reason for which
> cpufreq_get_hw_max_freq() was made weak. If some platform has entirely
> firmware driven frequency control, but it enables CONFIG_CPU_FREQ
> (as is the default) and it defines its own cpufreq_get_hw_max_freq(),
> it could benefit from AMU use.
> 
> So I did believe it was best for these checks to be decoupled, for this
> reason, and potential other reasons in the future, involving more
> decoupling from cpufreq.
> 
> I do have code in progress to clean the overall interaction between
> cpufreq and AMUs, started at [1]. Bear with me on this, it is all
> connected :).

Of course I missed few things here.

- I didn't realize that cpufreq_get_hw_max_freq() is defined weak :(

  I understand that we want to support everything that is possible,
  but there is no need to support cases which we may never have
  actually. We have seen code going in the kernel, which no one ever
  ends up using.

  Do we see a case in near future where someone is going to override
  this weak implementation ? If we don't have an actual target for it
  at the moment, then we should probably remove the weak attribute and
  simplify the code.

- I understood earlier that, we don't pick up AMU support unless all
  CPUs of a policy are supported by AMUs, but forgot that later while
  writing the patch. What is the thing with AMUs? Why would some
  platform add it only for some CPUs out of a policy ? Do we have such
  platforms already or in queue ?

Lets discuss more after we have settled on the first point here.

Thanks for review Ionela.

-- 
viresh

Re: [PATCH] arm64: topology: Don't support AMU without cpufreq

[Ionela Voinescu]

Hey,

On Thursday 09 Jul 2020 at 16:10:48 (+0530), Viresh Kumar wrote:
[..]
> > I agree that this happening is a cornercase and a reason for which
> > cpufreq_get_hw_max_freq() was made weak. If some platform has entirely
> > firmware driven frequency control, but it enables CONFIG_CPU_FREQ
> > (as is the default) and it defines its own cpufreq_get_hw_max_freq(),
> > it could benefit from AMU use.
> > 
> > So I did believe it was best for these checks to be decoupled, for this
> > reason, and potential other reasons in the future, involving more
> > decoupling from cpufreq.
> > 
> > I do have code in progress to clean the overall interaction between
> > cpufreq and AMUs, started at [1]. Bear with me on this, it is all
> > connected :).
> 
> Of course I missed few things here.
> 
> - I didn't realize that cpufreq_get_hw_max_freq() is defined weak :(
> 
>   I understand that we want to support everything that is possible,
>   but there is no need to support cases which we may never have
>   actually. We have seen code going in the kernel, which no one ever
>   ends up using.
> 
>   Do we see a case in near future where someone is going to override
>   this weak implementation ? If we don't have an actual target for it
>   at the moment, then we should probably remove the weak attribute and
>   simplify the code.
> 

I saw this case during FVP testing, although I acknowledge the 'virtual'
part of that platform [1]. But allowing this does enable AMU testing on
an AEM FVP.

While I completely understand the reasoning behind avoiding to introduce
large changes for small corner-case gains, the arguments for this
support was:
 - (1) AMUs are a new feature and it will take some time until we see the
   real usecases. That's always the case with early support for a
   feature - we want to add it early to enable its use and testing, but
   it will take some time to establish the true usecases.
 - (2) It literally needed 2 lines of code + the weak cpufreq function
   to support this.

> - I understood earlier that, we don't pick up AMU support unless all
>   CPUs of a policy are supported by AMUs, but forgot that later while
>   writing the patch. What is the thing with AMUs? Why would some
>   platform add it only for some CPUs out of a policy ? Do we have such
>   platforms already or in queue ?
> 

Given that I can't guarantee what hardware will or won't do, and given
that AMUs are an optional feature, I controlled the only thing I could:
the software :). By not making assumptions about the hardware, I ensured
that the code does not break the interaction between cpufreq use or AMU
use for frequency invariance.

This will be nicer in the new code as the control will be at CPU level,
rather than policy level.

[1]
https://developer.arm.com/tools-and-software/simulation-models/fixed-virtual-platforms

Regards,
Ionela.

> Lets discuss more after we have settled on the first point here.
> 
> Thanks for review Ionela.
> 
> -- 
> viresh

Re: [PATCH] arm64: topology: Don't support AMU without cpufreq

[Viresh Kumar]

On 09-07-20, 13:46, Ionela Voinescu wrote:
> I saw this case during FVP testing, although I acknowledge the 'virtual'
> part of that platform [1]. But allowing this does enable AMU testing on
> an AEM FVP.

In kernel, we only support things that are in mainline, else we don't
care about them. That's the general rule. And yeah I understand that
this is early support for a new hardware, and so it is better to add
code for things we are sure about.

> While I completely understand the reasoning behind avoiding to introduce
> large changes for small corner-case gains,

I think even that is fine, if there is a problem to be solved it needs
to be solved, big or small doesn't really matter. Just that it needs
to be there in mainline.

> the arguments for this
> support was:
>  - (1) AMUs are a new feature and it will take some time until we see the
>    real usecases. That's always the case with early support for a
>    feature - we want to add it early to enable its use and testing, but
>    it will take some time to establish the true usecases.

Exactly, and so people normally prefer to keep things simple until the
time the needs arises for the same. A patch can be added later, its no
big deal. But it should be added when we need it.

>  - (2) It literally needed 2 lines of code + the weak cpufreq function
>    to support this.

Yeah, small or big doesn't really matter.

> Given that I can't guarantee what hardware will or won't do, and given
> that AMUs are an optional feature, I controlled the only thing I could:
> the software :). By not making assumptions about the hardware, I ensured
> that the code does not break the interaction between cpufreq use or AMU
> use for frequency invariance.
> 
> This will be nicer in the new code as the control will be at CPU level,
> rather than policy level.

I won't try to force you to remove this piece and will leave it for
you to decide.

But, I don't see a future system in mainline which uses AMU but
doesn't have cpufreq for all its CPUs. And so I won't have kept code
for that, even if it is just 2 lines. We can always add it back when
required.

Thanks for the review again Ionela.

-- 
viresh