Re: [PATCH v3 4/5] arm64: Remove CPU operations dereferencing array

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Gavin Shan <gshan@redhat.com>
Cc: mark.rutland@arm.com, catalin.marinas@arm.com,
	robin.murphy@arm.com, sudeep.holla@arm.com, will@kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 4/5] arm64: Remove CPU operations dereferencing array
Date: Tue, 25 Feb 2020 14:13:07 +0000	[thread overview]
Message-ID: <20200225141307.GA8970@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <20200212004351.66576-5-gshan@redhat.com>

On Wed, Feb 12, 2020 at 11:43:50AM +1100, Gavin Shan wrote:
> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB
> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too
> much memory has been used for this. Also, all CPUs must use same CPU
> operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi
> pointed out.

That's not appropriate for a commit log. If you want to refer
to a mailing list discussion add a Link: tag with a lore archive
pointer.

> This introduces variable (@cpu_ops_index) to store the unified CPU
> operations index. The CPU, which has different index, won't be brought
> up. With this, the CPU operations dereferencing array is removed and
> 2KB memory is saved.

I think it is enough fiddling with indexes, if you need to save
memory reduce the cpu_ops array to a pointer and be done with that.

> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  arch/arm64/kernel/cpu_ops.c | 62 ++++++++++++++++++++-----------------
>  1 file changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index e133011f64b5..f59c087d6284 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -4,7 +4,6 @@
>   *
>   * Copyright (C) 2013 ARM Ltd.
>   */
> -
>  #include <linux/acpi.h>
>  #include <linux/cache.h>
>  #include <linux/errno.h>
> @@ -20,39 +19,32 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
>  #endif
>  extern const struct cpu_operations cpu_psci_ops;
>  
> -static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;

+static const struct cpu_operations *cpu_ops __ro_after_init;

> -
> -static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
> +/*
> + * Each element of the index array is shared by 4 CPUs. It means each
> + * CPU index uses 2 bits.
> + */
> +static const struct cpu_operations *const cpu_ops[] = {
>  	&smp_spin_table_ops,
> -	&cpu_psci_ops,
> -	NULL,
> -};
> -
> -static const struct cpu_operations *const acpi_supported_cpu_ops[] __initconst = {
>  #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
>  	&acpi_parking_protocol_ops,
>  #endif
>  	&cpu_psci_ops,
> -	NULL,
>  };
> +static int cpu_ops_index __ro_after_init = INT_MAX;
>  
> -static const struct cpu_operations * __init cpu_get_ops(const char *name)
> +static int __init get_cpu_ops_index(const char *name)
>  {
> -	const struct cpu_operations *const *ops;
> -
> -	ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;
> -
> -	while (*ops) {
> -		if (!strcmp(name, (*ops)->name))
> -			return *ops;
> +	int index;
>  
> -		ops++;
> +	for (index = 0; index < ARRAY_SIZE(cpu_ops); index++) {
> +		if (!strcmp(cpu_ops[index]->name, name))
> +			return index;
>  	}
>  
> -	return NULL;
> +	return -ERANGE;
>  }
>  
> -static const char *__init cpu_read_enable_method(int cpu)
> +static const char *__init get_cpu_method(int cpu)
>  {
>  	const char *enable_method;
>  
> @@ -93,26 +85,40 @@ static const char *__init cpu_read_enable_method(int cpu)
>  
>  	return enable_method;
>  }
> -/*
> - * Read a cpu's enable method and record it in cpu_ops.
> - */
> +
>  int __init init_cpu_ops(int cpu)
>  {
> -	const char *enable_method = cpu_read_enable_method(cpu);
> +	const char *enable_method = get_cpu_method(cpu);
> +	int index;
>  
>  	if (!enable_method)
>  		return -ENODEV;
>  
> -	cpu_ops[cpu] = cpu_get_ops(enable_method);
> -	if (!cpu_ops[cpu]) {
> +	index = get_cpu_ops_index(enable_method);
> +	if (index < 0) {
>  		pr_warn("Unsupported enable-method: %s\n", enable_method);
>  		return -EOPNOTSUPP;
>  	}
>  
> +	/* Update the index directly if it's invalid */
> +	if (cpu_ops_index == INT_MAX) {
> +		cpu_ops_index = index;
> +		return 0;
> +	}
> +
> +	if (index != cpu_ops_index) {
> +		pr_warn("Invalid CPU operations index %d (%d) on CPU %d\n",
> +			index, cpu_ops_index, cpu);
> +		return -EINVAL;
> +	}

There isn't really a need for this index song and dance, a pointer
will do to achieve what you are doing above.

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel