Re: [PATCH v1 2/3] cpufreq: CPPC: Add per_cpu efficiency_class

[Marc Zyngier <maz@kernel.org>]

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...