| Message ID | 20200318230145.72097-5-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Dereference CPU operations indirectly | expand |
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 >
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 >> >
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; }
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(-)