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

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

Commit Message

Gavin Shan Feb. 26, 2020, 12:23 a.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 CPUs must use same CPU
operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi
pointed out.

This introduces variable (@cpu_ops) to store the unfied CPU operations.
Those CPUs using different operations, which is figured out from device
tree or ACPI table, 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 function for that.

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 | 70 +++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 38 deletions(-)

Comments

Mark Rutland March 17, 2020, 10:56 a.m. UTC | #1
On Wed, Feb 26, 2020 at 11:23:55AM +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.

I suspect there might be some pre PSCIv0.2 platforms where the boot CPU
doesn't have PSCI, but others do. On those platforms, this could be
because CPU0 cannot be hotplugged out, and we must avoid doing so.

Can you check the in-kernel DTs to see if any of those exist?

Other than that, I agree that mandating uniformity is the best approach
here.

>  int __init init_cpu_ops(int cpu)
>  {
> -	const char *enable_method = cpu_read_enable_method(cpu);
> -
> -	if (!enable_method)
> -		return -ENODEV;
> +	const struct cpu_operations *ops = get_cpu_method(cpu);
>  
> -	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 global CPU operations if it's not initialized yet */
> +	if (!cpu_ops) {
> +		cpu_ops = ops;
> +		return 0;
> +	}

As above, I don't think this is quite right. If we're going to mandate
uniformity, we should init the ops from the boot CPU, and then verify
that every other CPU matches that. The initialization of the global ops
should not be conditional.

Thanks,
Mark.
Gavin Shan March 18, 2020, 3:53 a.m. UTC | #2
On 3/17/20 9:56 PM, Mark Rutland wrote:
> On Wed, Feb 26, 2020 at 11:23:55AM +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.
> 
> I suspect there might be some pre PSCIv0.2 platforms where the boot CPU
> doesn't have PSCI, but others do. On those platforms, this could be
> because CPU0 cannot be hotplugged out, and we must avoid doing so.
> 
> Can you check the in-kernel DTs to see if any of those exist?
> 
> Other than that, I agree that mandating uniformity is the best approach
> here.
> 

Thanks for the comments, Mark. There are platforms where the CPU#0 and other
CPUs have different "enable-method" specified. More specificly, CPU#0 doesn't
have "enable-method" while other CPUs have "psci" specified:

    lg/lg1312.dtsi
    lg/lg1313.dtsi
    mediatek/mt2712e.dtsi

In order to support two enable methods, I think we have two options here:
(1) Revert the code to what we had in v2. Two bits consumed by one CPU,
which is taken as index to a CPU operation array. The code can be found in
link [1] (2) Two CPU operation pointers are maintained. One is used to track
the CPU#0's operations and other one is for other cpus.

[1] https://patchwork.kernel.org/patch/11363745/

Please let me know which one is better.

>>   int __init init_cpu_ops(int cpu)
>>   {
>> -	const char *enable_method = cpu_read_enable_method(cpu);
>> -
>> -	if (!enable_method)
>> -		return -ENODEV;
>> +	const struct cpu_operations *ops = get_cpu_method(cpu);
>>   
>> -	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 global CPU operations if it's not initialized yet */
>> +	if (!cpu_ops) {
>> +		cpu_ops = ops;
>> +		return 0;
>> +	}
> 
> As above, I don't think this is quite right. If we're going to mandate
> uniformity, we should init the ops from the boot CPU, and then verify
> that every other CPU matches that. The initialization of the global ops
> should not be conditional.> 
I think you're correct because CPU#0's "enable-method" can be unspecified.
In this case, the CPU#0's operations will be set to same one as other CPUs,
which isn't correct. I will see how to handle this comments when the above
comments is resolved. This might become invalid after the above comments get
resolved.

Thanks,
Gavin
Gavin Shan March 18, 2020, 10:53 p.m. UTC | #3
On 3/18/20 2:53 PM, Gavin Shan wrote:
> On 3/17/20 9:56 PM, Mark Rutland wrote:
>> On Wed, Feb 26, 2020 at 11:23:55AM +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.
>>
>> I suspect there might be some pre PSCIv0.2 platforms where the boot CPU
>> doesn't have PSCI, but others do. On those platforms, this could be
>> because CPU0 cannot be hotplugged out, and we must avoid doing so.
>>
>> Can you check the in-kernel DTs to see if any of those exist?
>>
>> Other than that, I agree that mandating uniformity is the best approach
>> here.
>>
> 
> Thanks for the comments, Mark. There are platforms where the CPU#0 and other
> CPUs have different "enable-method" specified. More specificly, CPU#0 doesn't
> have "enable-method" while other CPUs have "psci" specified:
> 
>     lg/lg1312.dtsi
>     lg/lg1313.dtsi
>     mediatek/mt2712e.dtsi
> 
> In order to support two enable methods, I think we have two options here:
> (1) Revert the code to what we had in v2. Two bits consumed by one CPU,
> which is taken as index to a CPU operation array. The code can be found in
> link [1] (2) Two CPU operation pointers are maintained. One is used to track
> the CPU#0's operations and other one is for other cpus.
> 
> [1] https://patchwork.kernel.org/patch/11363745/
> 
> Please let me know which one is better.
> 

Mark, I plan to post a new revision to have option#2 after thinking
about it: we will have two pointers to track the operations for boot cpu
and the secondary CPUs separately. the consumed memory isn't scaled up to
the configured CPU number. So I think it's better than option#1, which
uses two-bits as index to CPU operation array.

Thanks,
Gavin

>>>   int __init init_cpu_ops(int cpu)
>>>   {
>>> -    const char *enable_method = cpu_read_enable_method(cpu);
>>> -
>>> -    if (!enable_method)
>>> -        return -ENODEV;
>>> +    const struct cpu_operations *ops = get_cpu_method(cpu);
>>> -    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 global CPU operations if it's not initialized yet */
>>> +    if (!cpu_ops) {
>>> +        cpu_ops = ops;
>>> +        return 0;
>>> +    }
>>
>> As above, I don't think this is quite right. If we're going to mandate
>> uniformity, we should init the ops from the boot CPU, and then verify
>> that every other CPU matches that. The initialization of the global ops
>> should not be conditional.> 
> I think you're correct because CPU#0's "enable-method" can be unspecified.
> In this case, the CPU#0's operations will be set to same one as other CPUs,
> which isn't correct. I will see how to handle this comments when the above
> comments is resolved. This might become invalid after the above comments get
> resolved.
> 
> Thanks,
> Gavin
>

Patch
diff mbox series

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index e133011f64b5..af73ca502b95 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -20,41 +20,19 @@  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 *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 +69,38 @@  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);
-
-	if (!enable_method)
-		return -ENODEV;
+	const struct cpu_operations *ops = get_cpu_method(cpu);
 
-	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 global CPU operations if it's not initialized yet */
+	if (!cpu_ops) {
+		cpu_ops = ops;
+		return 0;
+	}
+
+	/* We should have unified CPU operations */
+	if (ops != cpu_ops) {
+		pr_warn("Invalid CPU operations %s (%s) on CPU %d\n",
+			ops->name, cpu_ops->name, cpu);
+		return -EINVAL;
 	}
 
 	return 0;
@@ -114,5 +108,5 @@  int __init init_cpu_ops(int cpu)
 
 const struct cpu_operations *get_cpu_ops(int cpu)
 {
-	return cpu_ops[cpu];
+	return cpu_ops;
 }