| Message ID | 20200212004351.66576-5-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Dereference CPU operations indirectly | expand |
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
On 2/26/20 1:13 AM, Lorenzo Pieralisi wrote: > 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. > Yep, a link tag is absolutely needed here. I will have one in next respin. >> 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. > Yes, the code will be simplified with a pointer, index works either. I will have a pointer in v4. >> 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. >> */ >> - The unnecessary change will be dropped in v4. >> #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; > Ok. >> - >> -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; >> } cpu_get_ops()'s logic will be merged into get_cpu_method() since we're here. It's too simple to have a separate function. And it's only called by get_cpu_method(). >> >> -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. > Sure, the index stuff will be replaced by a pointer in v4 :) Thanks, Gavin
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 *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; + } + return 0; } const struct cpu_operations *get_cpu_ops(int cpu) { - return cpu_ops[cpu]; + if (cpu_ops_index == INT_MAX) + return NULL; + + return cpu_ops[cpu_ops_index]; }
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_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. Signed-off-by: Gavin Shan <gshan@redhat.com> --- arch/arm64/kernel/cpu_ops.c | 62 ++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 28 deletions(-)