[PATCH v3 0/5] arm64: Dereference CPU operations indirectly

[PATCH v3 0/5] arm64: Dereference CPU operations indirectly

[Gavin Shan]

The subject perhaps can't precisely indicate what this series does, but
keep it same as before for consistency.

In current implementation, an array (@cpu_ops[NR_CPUS]) is maintained
to dereference the CPU operations. 2KB memory are consumed when NR_CPUS
is configured to 256. It's too much than what I expected. This series
reworks the implementation to dereference the CPU operations indirectly
by using the index to the CPU operations array, so that less memory (4
bytes) will be consumed for the same purpose. The optimization bases on
the assumption: these CPU operations aren't dereferenced in hot path.
Also, we have only one valid CPU enablement method for all CPUs.

PATCH[1/5] isn't too much relevant, to declare ACPI parking protocol only
when CONFIG_ARM64_ACPI_PARKING_PROTOCOL has been enabled. PATCH[2/5]
renames cpu_read_ops() to init_cpu_ops(), which is obviously more precise
because it's initializing the CPU operations. PATCH[3/5] introduces
get_cpu_ops(), preparing for dereferencing CPU operations indirectly.
PATCH[4/5] removes the CPU operations deferencing array and replaces it
with an 4-bytes variable with the assumption: all CPUs should have same
enablement method. PATCH[5/5] removes the argument of get_cpu_ops() as
it's useless.

Changelog
=========
v3:
   * Assume all CPUs have same enablement method. With this, the used
     memory is further squeezed from 64 bytes to 4 bytes (Lorenzo Pieralisi)
   * Add PATCH[5/5] to remove argument of get_cpu_ops()  (Gavin Shan)
v2:
   * Pack 4 CPUs' indexes into one byte. 64 bytes are consumed in order
     to get the CPU operations                            (Robin Murphy)
   * Use ARRAY_SIZE() to iterate @cpu_ops[]               (Robin Murphy)
   * Make index-0 valid                                   (Robin Murphy)

Gavin Shan (5):
  arm64: Declare ACPI parking protocol CPU operation if needed
  arm64: Rename cpu_read_ops() to init_cpu_ops()
  arm64: Introduce get_cpu_ops() helper function
  arm64: Remove CPU operations dereferencing array
  arm64: Remove argument @cpu of get_cpu_ops()

 arch/arm64/include/asm/cpu_ops.h |  8 ++--
 arch/arm64/kernel/cpu_ops.c      | 69 +++++++++++++++++++-------------
 arch/arm64/kernel/cpuidle.c      | 10 ++---
 arch/arm64/kernel/setup.c        |  8 ++--
 arch/arm64/kernel/smp.c          | 60 ++++++++++++++++++---------
 5 files changed, 95 insertions(+), 60 deletions(-)

-- 
2.23.0


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

[PATCH v3 1/5] arm64: Declare ACPI parking protocol CPU operation if needed

[Gavin Shan]

It's obvious we needn't declare the corresponding CPU operation when
CONFIG_ARM64_ACPI_PARKING_PROTOCOL is disabled.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/kernel/cpu_ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index 7e07072757af..2082cfb1be86 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -15,7 +15,9 @@
 #include <asm/smp_plat.h>
 
 extern const struct cpu_operations smp_spin_table_ops;
+#ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
 extern const struct cpu_operations acpi_parking_protocol_ops;
+#endif
 extern const struct cpu_operations cpu_psci_ops;
 
 const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
-- 
2.23.0


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

[PATCH v3 2/5] arm64: Rename cpu_read_ops() to init_cpu_ops()

[Gavin Shan]

This renames cpu_read_ops() to init_cpu_ops() as the function it's only
called in initialization phase. Also, we will introduce get_cpu_ops() in
the subsequent patches, to retireve the CPU operation by the given CPU
index. The usage of cpu_read_ops() and get_cpu_ops() are difficult to be
distinguish from their names.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/include/asm/cpu_ops.h | 6 +++---
 arch/arm64/kernel/cpu_ops.c      | 2 +-
 arch/arm64/kernel/setup.c        | 2 +-
 arch/arm64/kernel/smp.c          | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index 86aabf1e0199..baa13b5db2ca 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -56,11 +56,11 @@ struct cpu_operations {
 };
 
 extern const struct cpu_operations *cpu_ops[NR_CPUS];
-int __init cpu_read_ops(int cpu);
+int __init init_cpu_ops(int cpu);
 
-static inline void __init cpu_read_bootcpu_ops(void)
+static inline void __init init_bootcpu_ops(void)
 {
-	cpu_read_ops(0);
+	init_cpu_ops(0);
 }
 
 #endif /* ifndef __ASM_CPU_OPS_H */
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index 2082cfb1be86..a6c3c816b618 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -96,7 +96,7 @@ static const char *__init cpu_read_enable_method(int cpu)
 /*
  * Read a cpu's enable method and record it in cpu_ops.
  */
-int __init cpu_read_ops(int cpu)
+int __init init_cpu_ops(int cpu)
 {
 	const char *enable_method = cpu_read_enable_method(cpu);
 
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index a34890bf309f..f66bd260cce8 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -344,7 +344,7 @@ void __init setup_arch(char **cmdline_p)
 	else
 		psci_acpi_init();
 
-	cpu_read_bootcpu_ops();
+	init_bootcpu_ops();
 	smp_init_cpus();
 	smp_build_mpidr_hash();
 
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index d4ed9a19d8fe..6f8477d7f3be 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -488,7 +488,7 @@ static bool __init is_mpidr_duplicate(unsigned int cpu, u64 hwid)
  */
 static int __init smp_cpu_setup(int cpu)
 {
-	if (cpu_read_ops(cpu))
+	if (init_cpu_ops(cpu))
 		return -ENODEV;
 
 	if (cpu_ops[cpu]->cpu_init(cpu))
-- 
2.23.0


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

[PATCH v3 3/5] arm64: Introduce get_cpu_ops() helper function

[Gavin Shan]

This introduces get_cpu_ops() to return the CPU operations according to
the given CPU index. For now, it simply returns the @cpu_ops[cpu] as
before. So it shouldn't introduce any functional changes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/include/asm/cpu_ops.h |  2 +-
 arch/arm64/kernel/cpu_ops.c      |  7 +++-
 arch/arm64/kernel/cpuidle.c      |  9 ++---
 arch/arm64/kernel/setup.c        |  6 ++--
 arch/arm64/kernel/smp.c          | 57 ++++++++++++++++++++++----------
 5 files changed, 55 insertions(+), 26 deletions(-)

diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index baa13b5db2ca..d28e8f37d3b4 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -55,8 +55,8 @@ struct cpu_operations {
 #endif
 };
 
-extern const struct cpu_operations *cpu_ops[NR_CPUS];
 int __init init_cpu_ops(int cpu);
+extern const struct cpu_operations *get_cpu_ops(int cpu);
 
 static inline void __init init_bootcpu_ops(void)
 {
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index a6c3c816b618..e133011f64b5 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -20,7 +20,7 @@ extern const struct cpu_operations acpi_parking_protocol_ops;
 #endif
 extern const struct cpu_operations cpu_psci_ops;
 
-const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
+static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
 
 static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
 	&smp_spin_table_ops,
@@ -111,3 +111,8 @@ int __init init_cpu_ops(int cpu)
 
 	return 0;
 }
+
+const struct cpu_operations *get_cpu_ops(int cpu)
+{
+	return cpu_ops[cpu];
+}
diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index e4d6af2fdec7..b512b5503f6e 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -18,11 +18,11 @@
 
 int arm_cpuidle_init(unsigned int cpu)
 {
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 	int ret = -EOPNOTSUPP;
 
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_suspend &&
-			cpu_ops[cpu]->cpu_init_idle)
-		ret = cpu_ops[cpu]->cpu_init_idle(cpu);
+	if (ops && ops->cpu_suspend && ops->cpu_init_idle)
+		ret = ops->cpu_init_idle(cpu);
 
 	return ret;
 }
@@ -37,8 +37,9 @@ int arm_cpuidle_init(unsigned int cpu)
 int arm_cpuidle_suspend(int index)
 {
 	int cpu = smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 
-	return cpu_ops[cpu]->cpu_suspend(index);
+	return ops->cpu_suspend(index);
 }
 
 #ifdef CONFIG_ACPI
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index f66bd260cce8..3fd2c11c09fc 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -371,8 +371,10 @@ void __init setup_arch(char **cmdline_p)
 static inline bool cpu_can_disable(unsigned int cpu)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_can_disable)
-		return cpu_ops[cpu]->cpu_can_disable(cpu);
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
+	if (ops && ops->cpu_can_disable)
+		return ops->cpu_can_disable(cpu);
 #endif
 	return false;
 }
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 6f8477d7f3be..5e1af1a3c521 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -93,8 +93,10 @@ static inline int op_cpu_kill(unsigned int cpu)
  */
 static int boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	if (cpu_ops[cpu]->cpu_boot)
-		return cpu_ops[cpu]->cpu_boot(cpu);
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
+	if (ops->cpu_boot)
+		return ops->cpu_boot(cpu);
 
 	return -EOPNOTSUPP;
 }
@@ -196,6 +198,7 @@ asmlinkage notrace void secondary_start_kernel(void)
 {
 	u64 mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
 	struct mm_struct *mm = &init_mm;
+	const struct cpu_operations *ops;
 	unsigned int cpu;
 
 	cpu = task_cpu(current);
@@ -227,8 +230,9 @@ asmlinkage notrace void secondary_start_kernel(void)
 	 */
 	check_local_cpu_capabilities();
 
-	if (cpu_ops[cpu]->cpu_postboot)
-		cpu_ops[cpu]->cpu_postboot();
+	ops = get_cpu_ops(cpu);
+	if (ops->cpu_postboot)
+		ops->cpu_postboot();
 
 	/*
 	 * Log the CPU info before it is marked online and might get read.
@@ -266,19 +270,21 @@ asmlinkage notrace void secondary_start_kernel(void)
 #ifdef CONFIG_HOTPLUG_CPU
 static int op_cpu_disable(unsigned int cpu)
 {
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
 	/*
 	 * If we don't have a cpu_die method, abort before we reach the point
 	 * of no return. CPU0 may not have an cpu_ops, so test for it.
 	 */
-	if (!cpu_ops[cpu] || !cpu_ops[cpu]->cpu_die)
+	if (!ops || !ops->cpu_die)
 		return -EOPNOTSUPP;
 
 	/*
 	 * We may need to abort a hot unplug for some other mechanism-specific
 	 * reason.
 	 */
-	if (cpu_ops[cpu]->cpu_disable)
-		return cpu_ops[cpu]->cpu_disable(cpu);
+	if (ops->cpu_disable)
+		return ops->cpu_disable(cpu);
 
 	return 0;
 }
@@ -314,15 +320,17 @@ int __cpu_disable(void)
 
 static int op_cpu_kill(unsigned int cpu)
 {
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
+
 	/*
 	 * If we have no means of synchronising with the dying CPU, then assume
 	 * that it is really dead. We can only wait for an arbitrary length of
 	 * time and hope that it's dead, so let's skip the wait and just hope.
 	 */
-	if (!cpu_ops[cpu]->cpu_kill)
+	if (!ops->cpu_kill)
 		return 0;
 
-	return cpu_ops[cpu]->cpu_kill(cpu);
+	return ops->cpu_kill(cpu);
 }
 
 /*
@@ -357,6 +365,7 @@ void __cpu_die(unsigned int cpu)
 void cpu_die(void)
 {
 	unsigned int cpu = smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 
 	idle_task_exit();
 
@@ -370,7 +379,7 @@ void cpu_die(void)
 	 * mechanism must perform all required cache maintenance to ensure that
 	 * no dirty lines are lost in the process of shutting down the CPU.
 	 */
-	cpu_ops[cpu]->cpu_die(cpu);
+	ops->cpu_die(cpu);
 
 	BUG();
 }
@@ -383,6 +392,7 @@ void cpu_die(void)
 void cpu_die_early(void)
 {
 	int cpu = smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(cpu);
 
 	pr_crit("CPU%d: will not boot\n", cpu);
 
@@ -392,8 +402,8 @@ void cpu_die_early(void)
 #ifdef CONFIG_HOTPLUG_CPU
 	update_cpu_boot_status(CPU_KILL_ME);
 	/* Check if we can park ourselves */
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
+	if (ops && ops->cpu_die)
+		ops->cpu_die(cpu);
 #endif
 	update_cpu_boot_status(CPU_STUCK_IN_KERNEL);
 
@@ -488,10 +498,13 @@ static bool __init is_mpidr_duplicate(unsigned int cpu, u64 hwid)
  */
 static int __init smp_cpu_setup(int cpu)
 {
+	const struct cpu_operations *ops;
+
 	if (init_cpu_ops(cpu))
 		return -ENODEV;
 
-	if (cpu_ops[cpu]->cpu_init(cpu))
+	ops = get_cpu_ops(cpu);
+	if (ops->cpu_init(cpu))
 		return -ENODEV;
 
 	set_cpu_possible(cpu, true);
@@ -714,6 +727,7 @@ void __init smp_init_cpus(void)
 
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
+	const struct cpu_operations *ops;
 	int err;
 	unsigned int cpu;
 	unsigned int this_cpu;
@@ -744,10 +758,11 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 		if (cpu == smp_processor_id())
 			continue;
 
-		if (!cpu_ops[cpu])
+		ops = get_cpu_ops(cpu);
+		if (!ops)
 			continue;
 
-		err = cpu_ops[cpu]->cpu_prepare(cpu);
+		err = ops->cpu_prepare(cpu);
 		if (err)
 			continue;
 
@@ -855,6 +870,10 @@ static atomic_t waiting_for_crash_ipi = ATOMIC_INIT(0);
 
 static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
 {
+#ifdef CONFIG_HOTPLUG_CPU
+	const struct cpu_operations *ops;
+#endif
+
 #ifdef CONFIG_KEXEC_CORE
 	crash_save_cpu(regs, cpu);
 
@@ -864,8 +883,9 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
 	sdei_mask_local_cpu();
 
 #ifdef CONFIG_HOTPLUG_CPU
-	if (cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
+	ops = get_cpu_ops(cpu);
+	if (ops->cpu_die)
+		ops->cpu_die(cpu);
 #endif
 
 	/* just in case */
@@ -1044,8 +1064,9 @@ static bool have_cpu_die(void)
 {
 #ifdef CONFIG_HOTPLUG_CPU
 	int any_cpu = raw_smp_processor_id();
+	const struct cpu_operations *ops = get_cpu_ops(any_cpu);
 
-	if (cpu_ops[any_cpu] && cpu_ops[any_cpu]->cpu_die)
+	if (ops && ops->cpu_die)
 		return true;
 #endif
 	return false;
-- 
2.23.0


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

[PATCH v3 4/5] arm64: Remove CPU operations dereferencing array

[Gavin Shan]

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

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];
 }
-- 
2.23.0


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

[PATCH v3 5/5] arm64: Remove argument @cpu of get_cpu_ops()

[Gavin Shan]

The argument @cpu of get_cpu_ops() isn't used, to remove it.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/arm64/include/asm/cpu_ops.h |  2 +-
 arch/arm64/kernel/cpu_ops.c      |  2 +-
 arch/arm64/kernel/cpuidle.c      |  5 ++---
 arch/arm64/kernel/setup.c        |  2 +-
 arch/arm64/kernel/smp.c          | 21 ++++++++++-----------
 5 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index d28e8f37d3b4..1d5c514ca053 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -56,7 +56,7 @@ struct cpu_operations {
 };
 
 int __init init_cpu_ops(int cpu);
-extern const struct cpu_operations *get_cpu_ops(int cpu);
+extern const struct cpu_operations *get_cpu_ops(void);
 
 static inline void __init init_bootcpu_ops(void)
 {
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index f59c087d6284..67b90399fb4b 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -115,7 +115,7 @@ int __init init_cpu_ops(int cpu)
 	return 0;
 }
 
-const struct cpu_operations *get_cpu_ops(int cpu)
+const struct cpu_operations *get_cpu_ops(void)
 {
 	if (cpu_ops_index == INT_MAX)
 		return NULL;
diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index b512b5503f6e..da2db14d2d45 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -18,7 +18,7 @@
 
 int arm_cpuidle_init(unsigned int cpu)
 {
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 	int ret = -EOPNOTSUPP;
 
 	if (ops && ops->cpu_suspend && ops->cpu_init_idle)
@@ -36,8 +36,7 @@ int arm_cpuidle_init(unsigned int cpu)
  */
 int arm_cpuidle_suspend(int index)
 {
-	int cpu = smp_processor_id();
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	return ops->cpu_suspend(index);
 }
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 3fd2c11c09fc..5c68fa2d9d1b 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -371,7 +371,7 @@ void __init setup_arch(char **cmdline_p)
 static inline bool cpu_can_disable(unsigned int cpu)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	if (ops && ops->cpu_can_disable)
 		return ops->cpu_can_disable(cpu);
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 5e1af1a3c521..0180d4163e17 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -93,7 +93,7 @@ static inline int op_cpu_kill(unsigned int cpu)
  */
 static int boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	if (ops->cpu_boot)
 		return ops->cpu_boot(cpu);
@@ -230,7 +230,7 @@ asmlinkage notrace void secondary_start_kernel(void)
 	 */
 	check_local_cpu_capabilities();
 
-	ops = get_cpu_ops(cpu);
+	ops = get_cpu_ops();
 	if (ops->cpu_postboot)
 		ops->cpu_postboot();
 
@@ -270,7 +270,7 @@ asmlinkage notrace void secondary_start_kernel(void)
 #ifdef CONFIG_HOTPLUG_CPU
 static int op_cpu_disable(unsigned int cpu)
 {
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	/*
 	 * If we don't have a cpu_die method, abort before we reach the point
@@ -320,7 +320,7 @@ int __cpu_disable(void)
 
 static int op_cpu_kill(unsigned int cpu)
 {
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	/*
 	 * If we have no means of synchronising with the dying CPU, then assume
@@ -365,7 +365,7 @@ void __cpu_die(unsigned int cpu)
 void cpu_die(void)
 {
 	unsigned int cpu = smp_processor_id();
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	idle_task_exit();
 
@@ -392,7 +392,7 @@ void cpu_die(void)
 void cpu_die_early(void)
 {
 	int cpu = smp_processor_id();
-	const struct cpu_operations *ops = get_cpu_ops(cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	pr_crit("CPU%d: will not boot\n", cpu);
 
@@ -503,7 +503,7 @@ static int __init smp_cpu_setup(int cpu)
 	if (init_cpu_ops(cpu))
 		return -ENODEV;
 
-	ops = get_cpu_ops(cpu);
+	ops = get_cpu_ops();
 	if (ops->cpu_init(cpu))
 		return -ENODEV;
 
@@ -758,7 +758,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 		if (cpu == smp_processor_id())
 			continue;
 
-		ops = get_cpu_ops(cpu);
+		ops = get_cpu_ops();
 		if (!ops)
 			continue;
 
@@ -883,7 +883,7 @@ static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
 	sdei_mask_local_cpu();
 
 #ifdef CONFIG_HOTPLUG_CPU
-	ops = get_cpu_ops(cpu);
+	ops = get_cpu_ops();
 	if (ops->cpu_die)
 		ops->cpu_die(cpu);
 #endif
@@ -1063,8 +1063,7 @@ int setup_profiling_timer(unsigned int multiplier)
 static bool have_cpu_die(void)
 {
 #ifdef CONFIG_HOTPLUG_CPU
-	int any_cpu = raw_smp_processor_id();
-	const struct cpu_operations *ops = get_cpu_ops(any_cpu);
+	const struct cpu_operations *ops = get_cpu_ops();
 
 	if (ops && ops->cpu_die)
 		return true;
-- 
2.23.0


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

Re: [PATCH v3 0/5] arm64: Dereference CPU operations indirectly

[Gavin Shan]

On 2/12/20 11:43 AM, Gavin Shan wrote:
> The subject perhaps can't precisely indicate what this series does, but
> keep it same as before for consistency.
> 
> In current implementation, an array (@cpu_ops[NR_CPUS]) is maintained
> to dereference the CPU operations. 2KB memory are consumed when NR_CPUS
> is configured to 256. It's too much than what I expected. This series
> reworks the implementation to dereference the CPU operations indirectly
> by using the index to the CPU operations array, so that less memory (4
> bytes) will be consumed for the same purpose. The optimization bases on
> the assumption: these CPU operations aren't dereferenced in hot path.
> Also, we have only one valid CPU enablement method for all CPUs.
> 
> PATCH[1/5] isn't too much relevant, to declare ACPI parking protocol only
> when CONFIG_ARM64_ACPI_PARKING_PROTOCOL has been enabled. PATCH[2/5]
> renames cpu_read_ops() to init_cpu_ops(), which is obviously more precise
> because it's initializing the CPU operations. PATCH[3/5] introduces
> get_cpu_ops(), preparing for dereferencing CPU operations indirectly.
> PATCH[4/5] removes the CPU operations deferencing array and replaces it
> with an 4-bytes variable with the assumption: all CPUs should have same
> enablement method. PATCH[5/5] removes the argument of get_cpu_ops() as
> it's useless.
> 
> Changelog
> =========
> v3:
>     * Assume all CPUs have same enablement method. With this, the used
>       memory is further squeezed from 64 bytes to 4 bytes (Lorenzo Pieralisi)
>     * Add PATCH[5/5] to remove argument of get_cpu_ops()  (Gavin Shan)
> v2:
>     * Pack 4 CPUs' indexes into one byte. 64 bytes are consumed in order
>       to get the CPU operations                            (Robin Murphy)
>     * Use ARRAY_SIZE() to iterate @cpu_ops[]               (Robin Murphy)
>     * Make index-0 valid                                   (Robin Murphy)
> 

Lorenzo/Robin, could you please give it quick scan when you have time?
Please let me know if you have more comments :)

Thanks,
Gavin

> Gavin Shan (5):
>    arm64: Declare ACPI parking protocol CPU operation if needed
>    arm64: Rename cpu_read_ops() to init_cpu_ops()
>    arm64: Introduce get_cpu_ops() helper function
>    arm64: Remove CPU operations dereferencing array
>    arm64: Remove argument @cpu of get_cpu_ops()
> 
>   arch/arm64/include/asm/cpu_ops.h |  8 ++--
>   arch/arm64/kernel/cpu_ops.c      | 69 +++++++++++++++++++-------------
>   arch/arm64/kernel/cpuidle.c      | 10 ++---
>   arch/arm64/kernel/setup.c        |  8 ++--
>   arch/arm64/kernel/smp.c          | 60 ++++++++++++++++++---------
>   5 files changed, 95 insertions(+), 60 deletions(-)
> 


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

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

[Lorenzo Pieralisi]

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

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

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

[Gavin Shan]

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


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