Messages in this thread | | | Date | Thu, 17 Mar 2022 15:13:30 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH v1 2/3] cpufreq: CPPC: Add per_cpu efficiency_class |
| |
On 2022-03-17 13:34, Pierre Gondois wrote: > In ACPI, describing power efficiency of CPUs can be done through the > following arm specific field: > ACPI 6.4, s5.2.12.14 'GIC CPU Interface (GICC) Structure', > 'Processor Power Efficiency Class field': > Describes the relative power efficiency of the associated pro- > cessor. Lower efficiency class numbers are more efficient than > higher ones (e.g. efficiency class 0 should be treated as more > efficient than efficiency class 1). However, absolute values > of this number have no meaning: 2 isn’t necessarily half as > efficient as 1. > > The efficiency_class field is stored in the GicC structure of the > ACPI MADT table and it's currently supported in Linux for arm64 only. > Thus, this new functionality is introduced for arm64 only. > > To allow the cppc_cpufreq driver to know and preprocess the > efficiency_class values of all the CPUs, add a per_cpu efficiency_class > variable to store them. Also add a static efficiency_class_populated > to let the driver know efficiency_class values are usable and register > an artificial Energy Model (EM) based on normalized class values. > > At least 2 different efficiency classes must be present, > otherwise there is no use in creating an Energy Model. > > The efficiency_class values are squeezed in [0:#efficiency_class-1] > while conserving the order. For instance, efficiency classes of: > [111, 212, 250] > will be mapped to: > [0 (was 111), 1 (was 212), 2 (was 250)]. > > Each policy being independently registered in the driver, populating > the per_cpu efficiency_class is done only once at the driver > initialization. This prevents from having each policy re-searching the > efficiency_class values of other CPUs. > > The patch also exports acpi_cpu_get_madt_gicc() to fetch the GicC > structure of the ACPI MADT table for each CPU. > > Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com> > --- > arch/arm64/kernel/smp.c | 1 + > drivers/cpufreq/cppc_cpufreq.c | 55 ++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index 27df5c1e6baa..56637cbea5d6 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -512,6 +512,7 @@ struct acpi_madt_generic_interrupt > *acpi_cpu_get_madt_gicc(int cpu) > { > return &cpu_madt_gicc[cpu]; > } > +EXPORT_SYMBOL(acpi_cpu_get_madt_gicc);
Why not EXPORT_SYMBOL_GPL()?
> > /* > * acpi_map_gic_cpu_interface - parse processor MADT entry > diff --git a/drivers/cpufreq/cppc_cpufreq.c > b/drivers/cpufreq/cppc_cpufreq.c > index 8f950fe72765..a6cd95c3b474 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -422,12 +422,66 @@ static unsigned int > cppc_cpufreq_get_transition_delay_us(unsigned int cpu) > return cppc_get_transition_latency(cpu) / NSEC_PER_USEC; > } > > +static bool efficiency_class_populated; > +static DEFINE_PER_CPU(unsigned int, efficiency_class); > + > +static int populate_efficiency_class(void) > +{ > + unsigned int min = UINT_MAX, max = 0, class; > + struct acpi_madt_generic_interrupt *gicc; > + int cpu; > + > + for_each_possible_cpu(cpu) { > + gicc = acpi_cpu_get_madt_gicc(cpu); > + if (!gicc) > + return -ENODEV;
How can that happen if you made it here using ACPI?
> + > + per_cpu(efficiency_class, cpu) = gicc->efficiency_class; > + min = min_t(unsigned int, min, gicc->efficiency_class); > + max = max_t(unsigned int, max, gicc->efficiency_class); > + }
Why don't you use a temporary bitmap of 256 bits, tracking the classes that are actually being used?
> + > + if (min == max) {
This would become (bitmap_weight(used_classes) <= 1). Then from the same construct you know how many different classes you have. You also have the min, max, and all the values in between.
> + pr_debug("Efficiency classes are all equal (=%d). " > + "No EM registered", max); > + return -EINVAL; > + } > + > + /* > + * Squeeze efficiency class values on [0:#efficiency_class-1]. > + * Values are per spec in [0:255]. > + */ > + for (class = 0; class < 256; class++) { > + unsigned int new_min, curr; > + > + new_min = UINT_MAX; > + for_each_possible_cpu(cpu) { > + curr = per_cpu(efficiency_class, cpu); > + if (curr == min) > + per_cpu(efficiency_class, cpu) = class; > + else if (curr > min) > + new_min = min(new_min, curr); > + } > + > + if (new_min == UINT_MAX) > + break; > + min = new_min; > + }
I find it really hard to reason about this because you are dynamically rewriting the values you keep reevaluating.
How about something like this, which I find more readable:
DECLARE_BITMAP(used_classes, 256) = {}; int class, index, cpu;
for_each_possible_cpu(cpu) { unsigned int ec;
ec = acpi_cpu_get_madt_gicc(cpu)->efficiency_class & 0xff; bitmap_set(ec, &used_classes); }
if (bitmap_weight(&used_classes, 256) <= 1) return;
index = 0;
for_each_set_bit(class, &used_classes, 256) { for_each_possible_cpu(cpu) { if (acpi_cpu_get_madt_gicc(cpu)->efficiency_class == class) per_cpu(efficiency_class, cpu) = index; }
index++; }
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |