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

[Gavin Shan <gshan@redhat.com>]

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


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