[v5,4/4] arm64: Remove CPU operations dereferencing array
diff mbox series

Message ID 20200318230145.72097-5-gshan@redhat.com
State New
Headers show
Series
  • arm64: Dereference CPU operations indirectly
Related show

Commit Message

Gavin Shan March 18, 2020, 11:01 p.m. UTC
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 secondary CPUs must use
same CPU operations and we shouldn't bring up the broken CPU as Lorenzo
Pieralisi and Mark Rutland pointed out.

This introduces two variables (@{boot,secondary}_cpu_ops) to store the
CPU operations for boot CPU and secondary CPUs separately, which are
figured out from device tree or ACPI table. The secondary CPUs which
have inconsistent operations won't be brought up. With this, the CPU
operations dereferencing array is removed and 2KB memory is saved. Note
the logic of cpu_get_ops() is merged to get_cpu_method() since the logic
is simple enough and no need to have a separate function for it.

Link: https://lore.kernel.org/linux-arm-kernel/20200211114553.GA21093@e121166-lin.cambridge.arm.com
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/kernel/cpu_ops.c | 77 +++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 38 deletions(-)

Comments

Mark Rutland March 19, 2020, 7:38 p.m. UTC | #1
On Thu, Mar 19, 2020 at 10:01:45AM +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 secondary CPUs must use
> same CPU operations and we shouldn't bring up the broken CPU as Lorenzo
> Pieralisi and Mark Rutland pointed out.
> 
> This introduces two variables (@{boot,secondary}_cpu_ops) to store the
> CPU operations for boot CPU and secondary CPUs separately, which are
> figured out from device tree or ACPI table. The secondary CPUs which
> have inconsistent operations won't be brought up. With this, the CPU
> operations dereferencing array is removed and 2KB memory is saved. Note
> the logic of cpu_get_ops() is merged to get_cpu_method() since the logic
> is simple enough and no need to have a separate function for it.

To be honest, I'm not too keen on this. We've generally tried to bucket
things as either global or per-cpu, and it's odd to go against that.

Is 2K a problem because it forms part of the static Image size? If so,
could we make this a percpu pointer instead, or is there a problem with
that?

Thanks,
Mark.

> 
> Link: https://lore.kernel.org/linux-arm-kernel/20200211114553.GA21093@e121166-lin.cambridge.arm.com
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  arch/arm64/kernel/cpu_ops.c | 77 +++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index e133011f64b5..a0f647d22e36 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -20,41 +20,20 @@ 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 *const dt_supported_cpu_ops[] __initconst = {
> +static const struct cpu_operations *const available_cpu_ops[] __initconst = {
>  	&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 const struct cpu_operations *boot_cpu_ops __ro_after_init;
> +static const struct cpu_operations *secondary_cpu_ops __ro_after_init;
>  
> -static const struct cpu_operations * __init cpu_get_ops(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;
> -
> -		ops++;
> -	}
> -
> -	return NULL;
> -}
> -
> -static const char *__init cpu_read_enable_method(int cpu)
> +static const struct cpu_operations * __init get_cpu_method(int cpu)
>  {
>  	const char *enable_method;
> +	int i;
>  
>  	if (acpi_disabled) {
>  		struct device_node *dn = of_get_cpu_node(cpu, NULL);
> @@ -91,22 +70,44 @@ static const char *__init cpu_read_enable_method(int cpu)
>  		}
>  	}
>  
> -	return enable_method;
> +	if (!enable_method) {
> +		pr_warn("No enable-method found on CPU %d\n", cpu);
> +		return NULL;
> +	}
> +
> +	/* Search in the array with method */
> +	for (i = 0; i < ARRAY_SIZE(available_cpu_ops); i++) {
> +		if (!strcmp(available_cpu_ops[i]->name, enable_method))
> +			return available_cpu_ops[i];
> +	}
> +
> +	return NULL;
>  }
> -/*
> - * 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 struct cpu_operations *ops = get_cpu_method(cpu);
>  
> -	if (!enable_method)
> -		return -ENODEV;
> -
> -	cpu_ops[cpu] = cpu_get_ops(enable_method);
> -	if (!cpu_ops[cpu]) {
> -		pr_warn("Unsupported enable-method: %s\n", enable_method);
> +	if (!ops)
>  		return -EOPNOTSUPP;
> +
> +	/* Update boot CPU operations */
> +	if (!cpu) {
> +		boot_cpu_ops = ops;
> +		return 0;
> +	}
> +
> +	/* Update secondary CPU operations if it's not initialized yet */
> +	if (!secondary_cpu_ops) {
> +		secondary_cpu_ops = ops;
> +		return 0;
> +	}
> +
> +	/* We should have unified secondary CPU operations */
> +	if (ops != secondary_cpu_ops) {
> +		pr_warn("Invalid CPU operations %s (%s) on secondary CPU %d\n",
> +			ops->name, secondary_cpu_ops->name, cpu);
> +		return -EINVAL;
>  	}
>  
>  	return 0;
> @@ -114,5 +115,5 @@ int __init init_cpu_ops(int cpu)
>  
>  const struct cpu_operations *get_cpu_ops(int cpu)
>  {
> -	return cpu_ops[cpu];
> +	return cpu ? secondary_cpu_ops : boot_cpu_ops;
>  }
> -- 
> 2.23.0
>
Gavin Shan March 19, 2020, 10:54 p.m. UTC | #2
On 3/20/20 6:38 AM, Mark Rutland wrote:
> On Thu, Mar 19, 2020 at 10:01:45AM +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 secondary CPUs must use
>> same CPU operations and we shouldn't bring up the broken CPU as Lorenzo
>> Pieralisi and Mark Rutland pointed out.
>>
>> This introduces two variables (@{boot,secondary}_cpu_ops) to store the
>> CPU operations for boot CPU and secondary CPUs separately, which are
>> figured out from device tree or ACPI table. The secondary CPUs which
>> have inconsistent operations won't be brought up. With this, the CPU
>> operations dereferencing array is removed and 2KB memory is saved. Note
>> the logic of cpu_get_ops() is merged to get_cpu_method() since the logic
>> is simple enough and no need to have a separate function for it.
> 
> To be honest, I'm not too keen on this. We've generally tried to bucket
> things as either global or per-cpu, and it's odd to go against that.
> 
> Is 2K a problem because it forms part of the static Image size? If so,
> could we make this a percpu pointer instead, or is there a problem with
> that?
> 

Yes, I agree the usual option is global array or per-cpu. The global array
is increasing the image size, which isn't nice. I don't think the per-cpu
can be used in this case because the per-cpu offset (@__per_cpu_offset[])
isn't initialized yet at that point.

    start_kernel
       setup_arch
          init_bootcpu_ops      # unable to access per-cpu bucket yet
       setup_per_cpu_areas      # per-cpu is initialized

As mentioned early, there are two options if we really want to decrease
the image size (for 2KB): (1) Two CPU operation pointers for boot CPU
and the secondary CPUs separately, which is exactly this patch does.
(2) Use a 2-bits index to the global CPU operation array, 64-bytes are
needed if 256 CPUs are configured.

On the other hand, the cleanup still sounds something to have if we don't
care extra 2KB size introduced to the image:

    * Merge array @dt_supported_cpu_ops[] and @acpi_supported_cpu_ops[]
    * Merge the logic of cpu_get_ops() and cpu_read_enable_method()
    * Comments cleanup.

Please let me know your preference so that I can give it another respin
if needed.

Thanks,
Gavin

>>
>> Link: https://lore.kernel.org/linux-arm-kernel/20200211114553.GA21093@e121166-lin.cambridge.arm.com
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   arch/arm64/kernel/cpu_ops.c | 77 +++++++++++++++++++------------------
>>   1 file changed, 39 insertions(+), 38 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
>> index e133011f64b5..a0f647d22e36 100644
>> --- a/arch/arm64/kernel/cpu_ops.c
>> +++ b/arch/arm64/kernel/cpu_ops.c
>> @@ -20,41 +20,20 @@ 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 *const dt_supported_cpu_ops[] __initconst = {
>> +static const struct cpu_operations *const available_cpu_ops[] __initconst = {
>>   	&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 const struct cpu_operations *boot_cpu_ops __ro_after_init;
>> +static const struct cpu_operations *secondary_cpu_ops __ro_after_init;
>>   
>> -static const struct cpu_operations * __init cpu_get_ops(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;
>> -
>> -		ops++;
>> -	}
>> -
>> -	return NULL;
>> -}
>> -
>> -static const char *__init cpu_read_enable_method(int cpu)
>> +static const struct cpu_operations * __init get_cpu_method(int cpu)
>>   {
>>   	const char *enable_method;
>> +	int i;
>>   
>>   	if (acpi_disabled) {
>>   		struct device_node *dn = of_get_cpu_node(cpu, NULL);
>> @@ -91,22 +70,44 @@ static const char *__init cpu_read_enable_method(int cpu)
>>   		}
>>   	}
>>   
>> -	return enable_method;
>> +	if (!enable_method) {
>> +		pr_warn("No enable-method found on CPU %d\n", cpu);
>> +		return NULL;
>> +	}
>> +
>> +	/* Search in the array with method */
>> +	for (i = 0; i < ARRAY_SIZE(available_cpu_ops); i++) {
>> +		if (!strcmp(available_cpu_ops[i]->name, enable_method))
>> +			return available_cpu_ops[i];
>> +	}
>> +
>> +	return NULL;
>>   }
>> -/*
>> - * 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 struct cpu_operations *ops = get_cpu_method(cpu);
>>   
>> -	if (!enable_method)
>> -		return -ENODEV;
>> -
>> -	cpu_ops[cpu] = cpu_get_ops(enable_method);
>> -	if (!cpu_ops[cpu]) {
>> -		pr_warn("Unsupported enable-method: %s\n", enable_method);
>> +	if (!ops)
>>   		return -EOPNOTSUPP;
>> +
>> +	/* Update boot CPU operations */
>> +	if (!cpu) {
>> +		boot_cpu_ops = ops;
>> +		return 0;
>> +	}
>> +
>> +	/* Update secondary CPU operations if it's not initialized yet */
>> +	if (!secondary_cpu_ops) {
>> +		secondary_cpu_ops = ops;
>> +		return 0;
>> +	}
>> +
>> +	/* We should have unified secondary CPU operations */
>> +	if (ops != secondary_cpu_ops) {
>> +		pr_warn("Invalid CPU operations %s (%s) on secondary CPU %d\n",
>> +			ops->name, secondary_cpu_ops->name, cpu);
>> +		return -EINVAL;
>>   	}
>>   
>>   	return 0;
>> @@ -114,5 +115,5 @@ int __init init_cpu_ops(int cpu)
>>   
>>   const struct cpu_operations *get_cpu_ops(int cpu)
>>   {
>> -	return cpu_ops[cpu];
>> +	return cpu ? secondary_cpu_ops : boot_cpu_ops;
>>   }
>> -- 
>> 2.23.0
>>
>

Patch
diff mbox series

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index e133011f64b5..a0f647d22e36 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -20,41 +20,20 @@  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 *const dt_supported_cpu_ops[] __initconst = {
+static const struct cpu_operations *const available_cpu_ops[] __initconst = {
 	&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 const struct cpu_operations *boot_cpu_ops __ro_after_init;
+static const struct cpu_operations *secondary_cpu_ops __ro_after_init;
 
-static const struct cpu_operations * __init cpu_get_ops(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;
-
-		ops++;
-	}
-
-	return NULL;
-}
-
-static const char *__init cpu_read_enable_method(int cpu)
+static const struct cpu_operations * __init get_cpu_method(int cpu)
 {
 	const char *enable_method;
+	int i;
 
 	if (acpi_disabled) {
 		struct device_node *dn = of_get_cpu_node(cpu, NULL);
@@ -91,22 +70,44 @@  static const char *__init cpu_read_enable_method(int cpu)
 		}
 	}
 
-	return enable_method;
+	if (!enable_method) {
+		pr_warn("No enable-method found on CPU %d\n", cpu);
+		return NULL;
+	}
+
+	/* Search in the array with method */
+	for (i = 0; i < ARRAY_SIZE(available_cpu_ops); i++) {
+		if (!strcmp(available_cpu_ops[i]->name, enable_method))
+			return available_cpu_ops[i];
+	}
+
+	return NULL;
 }
-/*
- * 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 struct cpu_operations *ops = get_cpu_method(cpu);
 
-	if (!enable_method)
-		return -ENODEV;
-
-	cpu_ops[cpu] = cpu_get_ops(enable_method);
-	if (!cpu_ops[cpu]) {
-		pr_warn("Unsupported enable-method: %s\n", enable_method);
+	if (!ops)
 		return -EOPNOTSUPP;
+
+	/* Update boot CPU operations */
+	if (!cpu) {
+		boot_cpu_ops = ops;
+		return 0;
+	}
+
+	/* Update secondary CPU operations if it's not initialized yet */
+	if (!secondary_cpu_ops) {
+		secondary_cpu_ops = ops;
+		return 0;
+	}
+
+	/* We should have unified secondary CPU operations */
+	if (ops != secondary_cpu_ops) {
+		pr_warn("Invalid CPU operations %s (%s) on secondary CPU %d\n",
+			ops->name, secondary_cpu_ops->name, cpu);
+		return -EINVAL;
 	}
 
 	return 0;
@@ -114,5 +115,5 @@  int __init init_cpu_ops(int cpu)
 
 const struct cpu_operations *get_cpu_ops(int cpu)
 {
-	return cpu_ops[cpu];
+	return cpu ? secondary_cpu_ops : boot_cpu_ops;
 }