| Message ID | 20201106125334.21570-4-ionela.voinescu@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: cppc: add FFH support using AMUs | expand |
On Fri, Nov 06, 2020 at 12:53:34PM +0000, Ionela Voinescu wrote: > +static inline > +int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val) > +{ > + if (!cpu_has_amu_feat(cpu)) > + return -EOPNOTSUPP; > + > + smp_call_function_single(cpu, func, val, 1); > + > + return 0; > +} I got lost in the cpufreq call chains. Can this function ever be called with interrupts disabled?
Hi, On Thursday 12 Nov 2020 at 18:00:46 (+0000), Catalin Marinas wrote: > On Fri, Nov 06, 2020 at 12:53:34PM +0000, Ionela Voinescu wrote: > > +static inline > > +int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val) > > +{ > > + if (!cpu_has_amu_feat(cpu)) > > + return -EOPNOTSUPP; > > + > > + smp_call_function_single(cpu, func, val, 1); > > + > > + return 0; > > +} > > I got lost in the cpufreq call chains. Can this function ever be called > with interrupts disabled? > The short answer is: not with the current implementation of its only user, the cppc_cpufreq driver (given the current cpufreq implementation). The long answer is: there is a case where the cpufreq .get function is called with local interrupts disabled - cpufreq_quick_get(), but there are a few "if"s in between this becoming a problem: 1. If cppc_cpufreq ever implements .setpolicy or, 1.1 Current implementation of cpufreq_quick_get() changes. 2. If one of the few users of cpufreq_quick_get() is used: cppc_cpufreq ends up being used as CPU cooling device(+IPA governor) or devfreq/tegra30 is used. In this potential case, smp_call_function_single() will warn us of call with irqs_disable() and if the stars align there could be a potential deadlock if two CPUs try to IPI (get counter reads of) each other. So it could be called with irqs disabled, but a few code changes are needed before that happens. I can bail out of counters_read_on_cpu() if irqs_disabled() to be on the safe side. Thanks for the review, Ionela. > -- > Catalin
On Fri, Nov 13, 2020 at 12:28:46PM +0000, Ionela Voinescu wrote: > On Thursday 12 Nov 2020 at 18:00:46 (+0000), Catalin Marinas wrote: > > On Fri, Nov 06, 2020 at 12:53:34PM +0000, Ionela Voinescu wrote: > > > +static inline > > > +int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val) > > > +{ > > > + if (!cpu_has_amu_feat(cpu)) > > > + return -EOPNOTSUPP; > > > + > > > + smp_call_function_single(cpu, func, val, 1); > > > + > > > + return 0; > > > +} > > > > I got lost in the cpufreq call chains. Can this function ever be called > > with interrupts disabled? > > The short answer is: not with the current implementation of its only > user, the cppc_cpufreq driver (given the current cpufreq implementation). > > The long answer is: there is a case where the cpufreq .get function is > called with local interrupts disabled - cpufreq_quick_get(), but there > are a few "if"s in between this becoming a problem: > > 1. If cppc_cpufreq ever implements .setpolicy or, > 1.1 Current implementation of cpufreq_quick_get() changes. > 2. If one of the few users of cpufreq_quick_get() is used: cppc_cpufreq > ends up being used as CPU cooling device(+IPA governor) or > devfreq/tegra30 is used. > > In this potential case, smp_call_function_single() will warn us of call > with irqs_disable() and if the stars align there could be a potential > deadlock if two CPUs try to IPI (get counter reads of) each other. > > So it could be called with irqs disabled, but a few code changes are > needed before that happens. > > I can bail out of counters_read_on_cpu() if irqs_disabled() to be on the > safe side. Thanks for the explanation. In case we forget two years from now and the core code changes, I think it's safe to bail out early with an error. You can add a patch on top of this series, no need to resend the whole.
On Fri, Nov 06, 2020 at 12:53:34PM +0000, Ionela Voinescu wrote: > If Activity Monitors (AMUs) are present, two of the counters can be used > to implement support for CPPC's (Collaborative Processor Performance > Control) delivered and reference performance monitoring functionality > using FFH (Functional Fixed Hardware). > > Given that counters for a certain CPU can only be read from that CPU, > while FFH operations can be called from any CPU for any of the CPUs, use > smp_call_function_single() to provide the requested values. > > Therefore, depending on the register addresses, the following values > are returned: > - 0x0 (DeliveredPerformanceCounterRegister): AMU core counter > - 0x1 (ReferencePerformanceCounterRegister): AMU constant counter > > The use of Activity Monitors is hidden behind the generic > cpu_read_{corecnt,constcnt}() functions. > > Read functionality for these two registers represents the only current > FFH support for CPPC. Read operations for other register values or write > operation for all registers are unsupported. Therefore, keep CPPC's FFH > unsupported if no CPUs have valid AMU frequency counters. For this > purpose, the get_cpu_with_amu_feat() is introduced. > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > --- > arch/arm64/include/asm/cpufeature.h | 3 ++ > arch/arm64/kernel/cpufeature.c | 10 +++++ > arch/arm64/kernel/topology.c | 64 +++++++++++++++++++++++++++++ > 3 files changed, 77 insertions(+) > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index 751bd9d3376b..f5b44ac354dc 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -772,6 +772,9 @@ static inline bool cpu_has_amu_feat(int cpu) > } > #endif > > +/* Get a cpu that supports the Activity Monitors Unit (AMU) */ > +extern int get_cpu_with_amu_feat(void); > + > static inline unsigned int get_vmid_bits(u64 mmfr1) > { > int vmid_bits; > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 1142970e985b..6b08ae74ad0a 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1526,6 +1526,11 @@ bool cpu_has_amu_feat(int cpu) > return cpumask_test_cpu(cpu, &amu_cpus); > } > > +int get_cpu_with_amu_feat(void) > +{ > + return cpumask_any(&amu_cpus); > +} > + > static void cpu_amu_enable(struct arm64_cpu_capabilities const *cap) > { > if (has_cpuid_feature(cap, SCOPE_LOCAL_CPU)) { > @@ -1554,6 +1559,11 @@ static bool has_amu(const struct arm64_cpu_capabilities *cap, > > return true; > } > +#else > +int get_cpu_with_amu_feat(void) > +{ > + return nr_cpu_ids; > +} > #endif > > #ifdef CONFIG_ARM64_VHE > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > index b8cb16e3a2cc..7c9b6a0ecd6a 100644 > --- a/arch/arm64/kernel/topology.c > +++ b/arch/arm64/kernel/topology.c > @@ -147,6 +147,9 @@ void update_freq_counters_refs(void) > > static inline bool freq_counters_valid(int cpu) > { > + if ((cpu >= nr_cpu_ids) || !cpumask_test_cpu(cpu, cpu_present_mask)) > + return false; > + > if (!cpu_has_amu_feat(cpu)) { > pr_debug("CPU%d: counters are not supported.\n", cpu); > return false; > @@ -323,3 +326,64 @@ void topology_scale_freq_tick(void) > this_cpu_write(arch_core_cycles_prev, core_cnt); > this_cpu_write(arch_const_cycles_prev, const_cnt); > } > + > +#ifdef CONFIG_ACPI_CPPC_LIB > +#include <acpi/cppc_acpi.h> Not sure what arm64 maintainers prefer, but this code has nothing to do with topolopy strictly speaking. I wonder if we can put it in separate file conditionally compiled if CONFIG_ACPI_CPPC_LIB is enabled there by eliminating #ifdef(main reason for raising this point). Either way: Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep
Hi Sudeep, On Friday 13 Nov 2020 at 14:16:58 (+0000), Sudeep Holla wrote: [..] > > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > > index b8cb16e3a2cc..7c9b6a0ecd6a 100644 > > --- a/arch/arm64/kernel/topology.c > > +++ b/arch/arm64/kernel/topology.c > > @@ -147,6 +147,9 @@ void update_freq_counters_refs(void) > > > > static inline bool freq_counters_valid(int cpu) > > { > > + if ((cpu >= nr_cpu_ids) || !cpumask_test_cpu(cpu, cpu_present_mask)) > > + return false; > > + > > if (!cpu_has_amu_feat(cpu)) { > > pr_debug("CPU%d: counters are not supported.\n", cpu); > > return false; > > @@ -323,3 +326,64 @@ void topology_scale_freq_tick(void) > > this_cpu_write(arch_core_cycles_prev, core_cnt); > > this_cpu_write(arch_const_cycles_prev, const_cnt); > > } > > + > > +#ifdef CONFIG_ACPI_CPPC_LIB > > +#include <acpi/cppc_acpi.h> > > Not sure what arm64 maintainers prefer, but this code has nothing to do > with topolopy strictly speaking. I wonder if we can put it in separate Yes, you are correct. I am/was wondering the same for all the counters/AMU related functions, but given they were only used for topology_scale_freq_tick() *until now*, it was okay to keep them in topology.c. But I might soon have at least one additional (to FIE and FFH) small usecase for them in the implementation of arch_freq_get_on_cpu(), so all these functions might be better off in a separate file as well. Side note: I don't think frequency invariance is strictly speaking related to topology either. Nether are other functions in the arch_topology driver. It's likely we got used to placing all arch function implementation in either the arch_topology driver or the <arch>/kernel/topology.c. > file conditionally compiled if CONFIG_ACPI_CPPC_LIB is enabled there > by eliminating #ifdef(main reason for raising this point). > I'm happy to split either one(FFH) or both(FFH and counters) in separate files. Given the above, let me know if/how you guys prefer this done. > Either way: > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> > Thank you for the reviews, Ionela. > -- > Regards, > Sudeep
On Fri, Nov 13, 2020 at 04:37:12PM +0000, Ionela Voinescu wrote: > On Friday 13 Nov 2020 at 14:16:58 (+0000), Sudeep Holla wrote: > [..] > > > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c > > > index b8cb16e3a2cc..7c9b6a0ecd6a 100644 > > > --- a/arch/arm64/kernel/topology.c > > > +++ b/arch/arm64/kernel/topology.c > > > @@ -147,6 +147,9 @@ void update_freq_counters_refs(void) > > > > > > static inline bool freq_counters_valid(int cpu) > > > { > > > + if ((cpu >= nr_cpu_ids) || !cpumask_test_cpu(cpu, cpu_present_mask)) > > > + return false; > > > + > > > if (!cpu_has_amu_feat(cpu)) { > > > pr_debug("CPU%d: counters are not supported.\n", cpu); > > > return false; > > > @@ -323,3 +326,64 @@ void topology_scale_freq_tick(void) > > > this_cpu_write(arch_core_cycles_prev, core_cnt); > > > this_cpu_write(arch_const_cycles_prev, const_cnt); > > > } > > > + > > > +#ifdef CONFIG_ACPI_CPPC_LIB > > > +#include <acpi/cppc_acpi.h> > > > > Not sure what arm64 maintainers prefer, but this code has nothing to do > > with topolopy strictly speaking. I wonder if we can put it in separate > > Yes, you are correct. I am/was wondering the same for all the > counters/AMU related functions, but given they were only used for > topology_scale_freq_tick() *until now*, it was okay to keep them in > topology.c. > > But I might soon have at least one additional (to FIE and FFH) small > usecase for them in the implementation of arch_freq_get_on_cpu(), so all > these functions might be better off in a separate file as well. > > Side note: I don't think frequency invariance is strictly speaking > related to topology either. Nether are other functions in the > arch_topology driver. It's likely we got used to placing all > arch function implementation in either the arch_topology driver or the > <arch>/kernel/topology.c. Yeah, it looks like these topology files became a dumping ground for whatever power related ;). I'm ok with these patches as they are for now but it would be good to do some refactoring on top and maybe move them to an amu.c file (it's not urgent, it can be for 5.12 or when you plan to add more stuff next). I don't have an opinion for arch_topology.c, so far it doesn't seem to have any AMU stuff in it.
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 751bd9d3376b..f5b44ac354dc 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -772,6 +772,9 @@ static inline bool cpu_has_amu_feat(int cpu) } #endif +/* Get a cpu that supports the Activity Monitors Unit (AMU) */ +extern int get_cpu_with_amu_feat(void); + static inline unsigned int get_vmid_bits(u64 mmfr1) { int vmid_bits; diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 1142970e985b..6b08ae74ad0a 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1526,6 +1526,11 @@ bool cpu_has_amu_feat(int cpu) return cpumask_test_cpu(cpu, &amu_cpus); } +int get_cpu_with_amu_feat(void) +{ + return cpumask_any(&amu_cpus); +} + static void cpu_amu_enable(struct arm64_cpu_capabilities const *cap) { if (has_cpuid_feature(cap, SCOPE_LOCAL_CPU)) { @@ -1554,6 +1559,11 @@ static bool has_amu(const struct arm64_cpu_capabilities *cap, return true; } +#else +int get_cpu_with_amu_feat(void) +{ + return nr_cpu_ids; +} #endif #ifdef CONFIG_ARM64_VHE diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c index b8cb16e3a2cc..7c9b6a0ecd6a 100644 --- a/arch/arm64/kernel/topology.c +++ b/arch/arm64/kernel/topology.c @@ -147,6 +147,9 @@ void update_freq_counters_refs(void) static inline bool freq_counters_valid(int cpu) { + if ((cpu >= nr_cpu_ids) || !cpumask_test_cpu(cpu, cpu_present_mask)) + return false; + if (!cpu_has_amu_feat(cpu)) { pr_debug("CPU%d: counters are not supported.\n", cpu); return false; @@ -323,3 +326,64 @@ void topology_scale_freq_tick(void) this_cpu_write(arch_core_cycles_prev, core_cnt); this_cpu_write(arch_const_cycles_prev, const_cnt); } + +#ifdef CONFIG_ACPI_CPPC_LIB +#include <acpi/cppc_acpi.h> + +static void cpu_read_corecnt(void *val) +{ + *(u64 *)val = read_corecnt(); +} + +static void cpu_read_constcnt(void *val) +{ + *(u64 *)val = read_constcnt(); +} + +static inline +int counters_read_on_cpu(int cpu, smp_call_func_t func, u64 *val) +{ + if (!cpu_has_amu_feat(cpu)) + return -EOPNOTSUPP; + + smp_call_function_single(cpu, func, val, 1); + + return 0; +} + +/* + * Refer to drivers/acpi/cppc_acpi.c for the description of the functions + * below. + */ +bool cpc_ffh_supported(void) +{ + return freq_counters_valid(get_cpu_with_amu_feat()); +} + +int cpc_read_ffh(int cpu, struct cpc_reg *reg, u64 *val) +{ + int ret = -EOPNOTSUPP; + + switch ((u64)reg->address) { + case 0x0: + ret = counters_read_on_cpu(cpu, cpu_read_corecnt, val); + break; + case 0x1: + ret = counters_read_on_cpu(cpu, cpu_read_constcnt, val); + break; + } + + if (!ret) { + *val &= GENMASK_ULL(reg->bit_offset + reg->bit_width - 1, + reg->bit_offset); + *val >>= reg->bit_offset; + } + + return ret; +} + +int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val) +{ + return -EOPNOTSUPP; +} +#endif /* CONFIG_ACPI_CPPC_LIB */
If Activity Monitors (AMUs) are present, two of the counters can be used to implement support for CPPC's (Collaborative Processor Performance Control) delivered and reference performance monitoring functionality using FFH (Functional Fixed Hardware). Given that counters for a certain CPU can only be read from that CPU, while FFH operations can be called from any CPU for any of the CPUs, use smp_call_function_single() to provide the requested values. Therefore, depending on the register addresses, the following values are returned: - 0x0 (DeliveredPerformanceCounterRegister): AMU core counter - 0x1 (ReferencePerformanceCounterRegister): AMU constant counter The use of Activity Monitors is hidden behind the generic cpu_read_{corecnt,constcnt}() functions. Read functionality for these two registers represents the only current FFH support for CPPC. Read operations for other register values or write operation for all registers are unsupported. Therefore, keep CPPC's FFH unsupported if no CPUs have valid AMU frequency counters. For this purpose, the get_cpu_with_amu_feat() is introduced. Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> --- arch/arm64/include/asm/cpufeature.h | 3 ++ arch/arm64/kernel/cpufeature.c | 10 +++++ arch/arm64/kernel/topology.c | 64 +++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+)