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

From: Viresh Kumar <viresh.kumar@linaro.org>
To: Ionela Voinescu <ionela.voinescu@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Vincent Guittot <vincent.guittot@linaro.org>
Subject: Re: [PATCH] arm64: topology: Don't support AMU without cpufreq
Date: Thu, 9 Jul 2020 16:10:48 +0530	[thread overview]
Message-ID: <20200709104048.emwuquj2qkyascb3@vireshk-i7> (raw)
In-Reply-To: <20200709101734.GB5623@arm.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel