lkml.org 
[lkml]   [2022]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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...

\
 
 \ /
  Last update: 2022-03-17 16:15    [W:0.078 / U:1.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site