| Message ID | 20210514095339.12979-3-ionela.voinescu@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | arch_topology, ACPI: populate cpu capacity from CPPC | expand |
On 14/05/2021 11:53, Ionela Voinescu wrote: [...] > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > index c1179edc0f3b..f710d64f125b 100644 > --- a/drivers/base/arch_topology.c > +++ b/drivers/base/arch_topology.c > @@ -291,6 +291,45 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu) > return !ret; > } > > +#ifdef CONFIG_ACPI_CPPC_LIB > +#include <acpi/cppc_acpi.h> init_cpu_capacity_cppc() shares a lot of functionality with the existing DT/CPUfreq-based approach (topology_parse_cpu_capacity(), register_cpufreq_notifier(), init_cpu_capacity_callback()). It looks like that the different ways of invocation (two steps per cpu vs. one step for all cpus) makes it hard to restructure the code to create more common bits. > +void init_cpu_capacity_cppc(void) > +{ > + struct cppc_perf_caps perf_caps; > + int cpu; > + > + if (likely(acpi_disabled || !acpi_cpc_valid())) likely(acpi_disabled) ? > + return; > + > + raw_capacity = kcalloc(num_possible_cpus(), sizeof(*raw_capacity), > + GFP_KERNEL); > + if (!raw_capacity) > + return; > + > + for_each_possible_cpu(cpu) { > + if (!cppc_get_perf_caps(cpu, &perf_caps)) { > + raw_capacity[cpu] = perf_caps.highest_perf; > + pr_debug("%s: CPU%d cpu_capacity=%u (raw).\n", > + __func__, cpu, raw_capacity[cpu]); There is quite a variety in the layout of the pr_xxx() log messages in this file. Originally the 'cpu_capacity:' was used to indicate that this log is from drivers/base/arch_topology.c. Now the GCC __func__ identifier is used. Maybe this can be aligned better? Especially since the functionality used in the existing DT-driven and now in the new CPPC-driven functionality is quite similar. Debugging is so much easier with consistent log strings. > + } else { > + pr_err("%s: CPU%d missing highest performance.\n", > + __func__, cpu); > + pr_err("%s: fallback to 1024 for all CPUs\n", > + __func__); > + goto exit; > + } > + } > + > + topology_normalize_cpu_scale(); > + schedule_work(&update_topology_flags_work); > + pr_debug("%s: cpu_capacity initialization done\n", __func__); > + > +exit: > + free_raw_capacity(); > +} > +#endif In case a system has CONFIG_ACPI_CPPC_LIB what does this mean for the DT-based approach via `register_cpufreq_notifier()`? Looks like we rely on: 376 static int __init register_cpufreq_notifier(void) ... 385 if (!acpi_disabled || ...) 386 return -EINVAL; to disable the CPUfreq part of the DT/CPUfreq-based approach on an ACPI system. [...]
On 14/05/21 10:53, Ionela Voinescu wrote: > +#ifdef CONFIG_ACPI_CPPC_LIB > +#include <acpi/cppc_acpi.h> > + > +void init_cpu_capacity_cppc(void) > +{ > + struct cppc_perf_caps perf_caps; > + int cpu; > + > + if (likely(acpi_disabled || !acpi_cpc_valid())) > + return; > + > + raw_capacity = kcalloc(num_possible_cpus(), sizeof(*raw_capacity), > + GFP_KERNEL); Per the below loop, the memory shouldn't need to be cleared. > + if (!raw_capacity) > + return; > + > + for_each_possible_cpu(cpu) { > + if (!cppc_get_perf_caps(cpu, &perf_caps)) { > + raw_capacity[cpu] = perf_caps.highest_perf; > + pr_debug("%s: CPU%d cpu_capacity=%u (raw).\n", > + __func__, cpu, raw_capacity[cpu]); > + } else { > + pr_err("%s: CPU%d missing highest performance.\n", > + __func__, cpu); > + pr_err("%s: fallback to 1024 for all CPUs\n", > + __func__); > + goto exit; > + } > + } > + > + topology_normalize_cpu_scale(); > + schedule_work(&update_topology_flags_work); > + pr_debug("%s: cpu_capacity initialization done\n", __func__); > + > +exit: > + free_raw_capacity(); > +} > +#endif
Hi Dietmar, Many thanks for the review! On Friday 14 May 2021 at 18:16:50 (+0200), Dietmar Eggemann wrote: > On 14/05/2021 11:53, Ionela Voinescu wrote: > > [...] > > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c > > index c1179edc0f3b..f710d64f125b 100644 > > --- a/drivers/base/arch_topology.c > > +++ b/drivers/base/arch_topology.c > > @@ -291,6 +291,45 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu) > > return !ret; > > } > > > > +#ifdef CONFIG_ACPI_CPPC_LIB > > +#include <acpi/cppc_acpi.h> > > init_cpu_capacity_cppc() shares a lot of functionality with the existing > DT/CPUfreq-based approach (topology_parse_cpu_capacity(), > register_cpufreq_notifier(), init_cpu_capacity_callback()). It looks > like that the different ways of invocation (two steps per cpu vs. one > step for all cpus) makes it hard to restructure the code to create more > common bits. > Yes, I looked at ways to reuse more of the DT-based functionality, but I did not find a better way. We reuse the normalization functionality and the rebuild of the scheduling domains, but I'm not sure there's room for much more, as the rest is specific to each source of capacity information, DT+cpufreq or CPPC. I also did not want to tie the new CPPC based functionality to cpufreq. While the DT-based path needs cpufreq policies initialized as it needs information on maximum frequency to obtain capacity, this is not needed in the new code. This results in simpler code and ensures support even for systems that do not have a cpufreq driver. > > +void init_cpu_capacity_cppc(void) > > +{ > > + struct cppc_perf_caps perf_caps; > > + int cpu; > > + > > + if (likely(acpi_disabled || !acpi_cpc_valid())) > > likely(acpi_disabled) ? > likely (acpi_disabled || !acpi_cpc_valid()) :) It's "likely" useless, but this function gets called for each CPU from acpi_cppc_processor_probe(), but it only continues with setting the cpu_scale after all possible CPUs have their _CPC objects populated. Therefore it's a lot more likely we return here. > > + return; > > + > > + raw_capacity = kcalloc(num_possible_cpus(), sizeof(*raw_capacity), > > + GFP_KERNEL); > > + if (!raw_capacity) > > + return; > > + > > + for_each_possible_cpu(cpu) { > > + if (!cppc_get_perf_caps(cpu, &perf_caps)) { > > + raw_capacity[cpu] = perf_caps.highest_perf; > > + pr_debug("%s: CPU%d cpu_capacity=%u (raw).\n", > > + __func__, cpu, raw_capacity[cpu]); > > There is quite a variety in the layout of the pr_xxx() log messages in > this file. Originally the 'cpu_capacity:' was used to indicate that this > log is from drivers/base/arch_topology.c. Now the GCC __func__ > identifier is used. Maybe this can be aligned better? Especially since > the functionality used in the existing DT-driven and now in the new > CPPC-driven functionality is quite similar. Debugging is so much easier > with consistent log strings. > Right! My intention was to keep the prints relatively similar, but my wanting to reduce the line length got the better of me. I'll keep the prints consistent. > > > + } else { > > + pr_err("%s: CPU%d missing highest performance.\n", > > + __func__, cpu); > > + pr_err("%s: fallback to 1024 for all CPUs\n", > > + __func__); > > + goto exit; > > + } > > + } > > + > > + topology_normalize_cpu_scale(); > > + schedule_work(&update_topology_flags_work); > > + pr_debug("%s: cpu_capacity initialization done\n", __func__); > > + > > +exit: > > + free_raw_capacity(); > > +} > > +#endif > > In case a system has CONFIG_ACPI_CPPC_LIB what does this mean for the > DT-based approach via `register_cpufreq_notifier()`? > CONFIG_ACPI_CPPC_LIB is enabled by default on arm64. This only ensures that we have the functionality to parse and work with the ACPI _CPC objects and it does not guarantee that ACPI will be used. > Looks like we rely on: > > 376 static int __init register_cpufreq_notifier(void) > ... > 385 if (!acpi_disabled || ...) > 386 return -EINVAL; > > to disable the CPUfreq part of the DT/CPUfreq-based approach on an ACPI > system. > It's both acpi_disabled and raw_capacity that guard the DT path. You need both to not use ACPI (therefore using DT) and to have valid capacity-dmips-mhz in DT for the cpufreq notifier that will eventually populate the cpu_scale variables to be registered. Thank you, Ionela. > [...]
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c index c1179edc0f3b..f710d64f125b 100644 --- a/drivers/base/arch_topology.c +++ b/drivers/base/arch_topology.c @@ -291,6 +291,45 @@ bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu) return !ret; } +#ifdef CONFIG_ACPI_CPPC_LIB +#include <acpi/cppc_acpi.h> + +void init_cpu_capacity_cppc(void) +{ + struct cppc_perf_caps perf_caps; + int cpu; + + if (likely(acpi_disabled || !acpi_cpc_valid())) + return; + + raw_capacity = kcalloc(num_possible_cpus(), sizeof(*raw_capacity), + GFP_KERNEL); + if (!raw_capacity) + return; + + for_each_possible_cpu(cpu) { + if (!cppc_get_perf_caps(cpu, &perf_caps)) { + raw_capacity[cpu] = perf_caps.highest_perf; + pr_debug("%s: CPU%d cpu_capacity=%u (raw).\n", + __func__, cpu, raw_capacity[cpu]); + } else { + pr_err("%s: CPU%d missing highest performance.\n", + __func__, cpu); + pr_err("%s: fallback to 1024 for all CPUs\n", + __func__); + goto exit; + } + } + + topology_normalize_cpu_scale(); + schedule_work(&update_topology_flags_work); + pr_debug("%s: cpu_capacity initialization done\n", __func__); + +exit: + free_raw_capacity(); +} +#endif + #ifdef CONFIG_CPU_FREQ static cpumask_var_t cpus_to_visit; static void parsing_done_workfn(struct work_struct *work); diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h index f180240dc95f..fbd829c3b7f7 100644 --- a/include/linux/arch_topology.h +++ b/include/linux/arch_topology.h @@ -11,6 +11,10 @@ void topology_normalize_cpu_scale(void); int topology_update_cpu_topology(void); +#ifdef CONFIG_ACPI_CPPC_LIB +void init_cpu_capacity_cppc(void); +#endif + struct device_node; bool topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
Define init_cpu_capacity_cppc() to use highest performance values from _CPC objects to obtain and set maximum capacity information for each CPU. acpi_cppc_processor_probe() is a good point at which to trigger the initialization of CPU (u-arch) capacity values, as at this point the highest performance values can be obtained from each CPU's _CPC objects. Architectures can therefore use this functionality through arch_init_invariance_cppc(). The performance scale used by CPPC is a unified scale for all CPUs in the system. Therefore, by obtaining the raw highest performance values from the _CPC objects, and normalizing them on the [0, 1024] capacity scale, used by the task scheduler, we obtain the CPU capacity of each CPU. While an ACPI Notify(0x85) could alert about a change in the highest performance value, which should in turn retrigger the CPU capacity computations, this notification is not currently handled by the ACPI processor driver. When supported, a call to arch_init_invariance_cppc() would perform the update. Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Sudeep Holla <sudeep.holla@arm.com> --- drivers/base/arch_topology.c | 39 +++++++++++++++++++++++++++++++++++ include/linux/arch_topology.h | 4 ++++ 2 files changed, 43 insertions(+)