Messages in this thread | | | Date | Thu, 17 Mar 2022 17:07:01 +0100 | Subject | Re: [PATCH v1 2/3] cpufreq: CPPC: Add per_cpu efficiency_class | From | Pierre Gondois <> |
| |
On 3/17/22 16:13, Marc Zyngier wrote: > 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()?
From what I understand, this could be made EXPORT_SYMBOL_GPL(). The only reason was that the other symbol exportation in the file wasn't restricted to 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?
This is effectively an extra check. This could be removed.
> >> + >> + 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++; > }
This is effectively much more readable. Thanks for the code snippet.
Regards, Pierre
> > > Thanks, > > M.
| |