RISC-V: Probe misaligned access speed in parallel

Probing for misaligned access speed takes about 0.06 seconds. On a system with 64 cores, doing this in smp_callin() means it's done serially, extending boot time by 3.8 seconds. That's a lot of boot time.

Instead of measuring each CPU serially, let's do the measurements on all CPUs in parallel. If we disable preemption on all CPUs, the jiffies stop ticking, so we can do this in stages of 1) everybody except core 0, then 2) core 0.

The measurement call in smp_callin() stays around, but is now conditionalized to only run if a new CPU shows up after the round of in-parallel measurements has run. The goal is to have the measurement call not run during boot or suspend/resume, but only on a hotplug addition.

Reported-by Jisheng Zhang <[email protected]>

Closes https://lore.kernel.org/all/mhng-9359993d-6872-4134-83ce-c97debe1cf9a@palmer-ri-x1c9/T/#mae9b8f40016f9df428829d33360144dc5026bcbf open here

Fixes 584ea6564bca ("RISC-V: Probe for unaligned access speed")

Signed-off-by Evan Green <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Tested-by Andrew Jones <[email protected]>

---

Changes in v2:
- Removed new global, used system_state == SYSTEM_RUNNING instead (Jisheng)
- Added tags

 arch/riscv/include/asm/cpufeature.h |  2 +-
 arch/riscv/kernel/cpufeature.c      | 22 +++++++++++++++++-----
 arch/riscv/kernel/smpboot.c         | 11 ++++++++++-
 3 files changed, 28 insertions(+), 7 deletions(-)

arch/riscv/include/asm/cpufeature.h +1 -1

index d0345bd659c9..b139796392d0 100644
@@ -30,6 +30,6 @@ DECLARE_PER_CPU(long, misaligned_access_speed);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
-void check_unaligned_access(int cpu);
+int check_unaligned_access(void *unused);
 
 #endif

arch/riscv/kernel/cpufeature.c +2 -1

index 1cfbba65d11a..40bb854fcb96 100644
@@ -556,8 +556,9 @@ unsigned long riscv_get_elf_hwcap(void)
 	return hwcap;
 }
 
-void check_unaligned_access(int cpu)
+int check_unaligned_access(void *unused)
 {
+	int cpu = smp_processor_id();
 	u64 start_cycles, end_cycles;
 	u64 word_cycles;
 	u64 byte_cycles;

arch/riscv/kernel/cpufeature.c +7 -0

 	}
 
 	/* Make an unaligned destination buffer. */

@@ -643,15 +644,26 @@ void check_unaligned_access(int cpu)
 
 out:
 	__free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
+	return 0;
+}
+
+static void check_unaligned_access_nonboot_cpu(void *param)
+{
+	if (smp_processor_id() != 0)
+		check_unaligned_access(param);
 }
 

arch/riscv/kernel/cpufeature.c +1 -1

 }
 
-arch_initcall(check_unaligned_access_boot_cpu);
+arch_initcall(check_unaligned_access_all_cpus);
 
 #ifdef CONFIG_RISCV_ALTERNATIVE
 /*

arch/riscv/kernel/smpboot.c +10 -1

index 1b8da4e40a4d..a014955b8699 100644
@@ -27,6 +27,7 @@
 #include <linux/sched/mm.h>
 #include <asm/cpu_ops.h>
 #include <asm/cpufeature.h>
+#include <asm/hwprobe.h>
 #include <asm/irq.h>
 #include <asm/mmu_context.h>
 #include <asm/numa.h>

@@ -246,7 +247,15 @@ asmlinkage __visible void smp_callin(void)
 
 	numa_add_cpu(curr_cpuid);
 	set_cpu_online(curr_cpuid, 1);
-	check_unaligned_access(curr_cpuid);
+
+	/*
+	 * Boot-time misaligned access speed measurements are done in parallel
+	 * in an initcall. Only measure here for hotplug.
+	 */
+	if ((system_state == SYSTEM_RUNNING) &&
+	    (per_cpu(misaligned_access_speed, curr_cpuid) == RISCV_HWPROBE_MISALIGNED_UNKNOWN)) {
+		check_unaligned_access(NULL);
+	}
 
 	if (has_vector()) {
 		if (riscv_v_setup_vsize())

Signature · 1 line

2.34.1

Yo,

Quoted 63 lines · Evan Green

Probing for misaligned access speed takes about 0.06 seconds. On a system with 64 cores, doing this in smp_callin() means it's done serially, extending boot time by 3.8 seconds. That's a lot of boot time.

Instead of measuring each CPU serially, let's do the measurements on all CPUs in parallel. If we disable preemption on all CPUs, the jiffies stop ticking, so we can do this in stages of 1) everybody except core 0, then 2) core 0.

The measurement call in smp_callin() stays around, but is now conditionalized to only run if a new CPU shows up after the round of in-parallel measurements has run. The goal is to have the measurement call not run during boot or suspend/resume, but only on a hotplug addition.

Reported-by Jisheng Zhang <[email protected]>

Closes https://lore.kernel.org/all/mhng-9359993d-6872-4134-83ce-c97debe1cf9a@palmer-ri-x1c9/T/#mae9b8f40016f9df428829d33360144dc5026bcbf open here

Fixes 584ea6564bca ("RISC-V: Probe for unaligned access speed")

Signed-off-by Evan Green <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Tested-by Andrew Jones <[email protected]>

---

Changes in v2:
- Removed new global, used system_state == SYSTEM_RUNNING instead (Jisheng)
- Added tags

 arch/riscv/include/asm/cpufeature.h |  2 +-
 arch/riscv/kernel/cpufeature.c      | 22 +++++++++++++++++-----
 arch/riscv/kernel/smpboot.c         | 11 ++++++++++-
 3 files changed, 28 insertions(+), 7 deletions(-)

arch/riscv/include/asm/cpufeature.h +1 -1

index d0345bd659c9..b139796392d0 100644
@@ -30,6 +30,6 @@ DECLARE_PER_CPU(long, misaligned_access_speed);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
-void check_unaligned_access(int cpu);
+int check_unaligned_access(void *unused);
 
 #endif

arch/riscv/kernel/cpufeature.c +2 -1

index 1cfbba65d11a..40bb854fcb96 100644
@@ -556,8 +556,9 @@ unsigned long riscv_get_elf_hwcap(void)
      return hwcap;
 }
 
-void check_unaligned_access(int cpu)
+int check_unaligned_access(void *unused)
 {
+     int cpu = smp_processor_id();
      u64 start_cycles, end_cycles;
      u64 word_cycles;
      u64 byte_cycles;

@@ -571,7 +572,7 @@ void check_unaligned_access(int cpu)
      page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 575

Quoted 4 lines

arch/riscv/kernel/cpufeature.c +1 -1

      if (!page) {
              pr_warn("Can't alloc pages to measure memcpy performance");
-             return;
+             return 0;

Dumb question maybe, but I am limited setup wise at the moment due to a hardware failure which makes checking stuff hard, why the signature change? Requirement for on_each_cpu()?

Quoted 16 lines

arch/riscv/kernel/cpufeature.c +7 -0

      }
 
      /* Make an unaligned destination buffer. */

@@ -643,15 +644,26 @@ void check_unaligned_access(int cpu)
 
 out:
      __free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
+     return 0;
+}
+
+static void check_unaligned_access_nonboot_cpu(void *param)
+{
+     if (smp_processor_id() != 0)
+             check_unaligned_access(param);
 }
 

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 663

Quoted 10 lines

arch/riscv/kernel/cpufeature.c +6 -2

-static int check_unaligned_access_boot_cpu(void)
+static int check_unaligned_access_all_cpus(void)
 {
-     check_unaligned_access(0);
+     /* Check everybody except 0, who stays behind to tend jiffies. */
+     on_each_cpu(check_unaligned_access_nonboot_cpu, NULL, 1);
+
+     /* Check core 0. */
+     smp_call_on_cpu(0, check_unaligned_access, NULL, true);
      return 0;

Why does this function return an int if it can only return 0?

Cheers,
Conor.

Quoted 37 lines

arch/riscv/kernel/cpufeature.c +1 -1

 }
 
-arch_initcall(check_unaligned_access_boot_cpu);
+arch_initcall(check_unaligned_access_all_cpus);
 
 #ifdef CONFIG_RISCV_ALTERNATIVE
 /*

arch/riscv/kernel/smpboot.c +10 -2

index 1b8da4e40a4d..a014955b8699 100644
@@ -27,6 +27,7 @@
 #include <linux/sched/mm.h>
 #include <asm/cpu_ops.h>
 #include <asm/cpufeature.h>
+#include <asm/hwprobe.h>
 #include <asm/irq.h>
 #include <asm/mmu_context.h>
 #include <asm/numa.h>

@@ -246,7 +247,15 @@ asmlinkage __visible void smp_callin(void)
 
      numa_add_cpu(curr_cpuid);
      set_cpu_online(curr_cpuid, 1);
-     check_unaligned_access(curr_cpuid);
+
+     /*
+      * Boot-time misaligned access speed measurements are done in parallel
+      * in an initcall. Only measure here for hotplug.
+      */
+     if ((system_state == SYSTEM_RUNNING) &&
+         (per_cpu(misaligned_access_speed, curr_cpuid) == RISCV_HWPROBE_MISALIGNED_UNKNOWN)) {
+             check_unaligned_access(NULL);
+     }
 
      if (has_vector()) {
              if (riscv_v_setup_vsize())
--

Quoted 2 lines

2.34.1

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 67 lines · Conor Dooley

Yo,

Quoted 63 lines · Evan Green

Probing for misaligned access speed takes about 0.06 seconds. On a system with 64 cores, doing this in smp_callin() means it's done serially, extending boot time by 3.8 seconds. That's a lot of boot time.

Instead of measuring each CPU serially, let's do the measurements on all CPUs in parallel. If we disable preemption on all CPUs, the jiffies stop ticking, so we can do this in stages of 1) everybody except core 0, then 2) core 0.

The measurement call in smp_callin() stays around, but is now conditionalized to only run if a new CPU shows up after the round of in-parallel measurements has run. The goal is to have the measurement call not run during boot or suspend/resume, but only on a hotplug addition.

Reported-by Jisheng Zhang <[email protected]>

Closes https://lore.kernel.org/all/mhng-9359993d-6872-4134-83ce-c97debe1cf9a@palmer-ri-x1c9/T/#mae9b8f40016f9df428829d33360144dc5026bcbf open here

Fixes 584ea6564bca ("RISC-V: Probe for unaligned access speed")

Signed-off-by Evan Green <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Tested-by Andrew Jones <[email protected]>

---

Changes in v2:
- Removed new global, used system_state == SYSTEM_RUNNING instead (Jisheng)
- Added tags

 arch/riscv/include/asm/cpufeature.h |  2 +-
 arch/riscv/kernel/cpufeature.c      | 22 +++++++++++++++++-----
 arch/riscv/kernel/smpboot.c         | 11 ++++++++++-
 3 files changed, 28 insertions(+), 7 deletions(-)

arch/riscv/include/asm/cpufeature.h +1 -1

index d0345bd659c9..b139796392d0 100644
@@ -30,6 +30,6 @@ DECLARE_PER_CPU(long, misaligned_access_speed);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
-void check_unaligned_access(int cpu);
+int check_unaligned_access(void *unused);
 
 #endif

arch/riscv/kernel/cpufeature.c +2 -1

index 1cfbba65d11a..40bb854fcb96 100644
@@ -556,8 +556,9 @@ unsigned long riscv_get_elf_hwcap(void)
      return hwcap;
 }
 
-void check_unaligned_access(int cpu)
+int check_unaligned_access(void *unused)
 {
+     int cpu = smp_processor_id();
      u64 start_cycles, end_cycles;
      u64 word_cycles;
      u64 byte_cycles;

@@ -571,7 +572,7 @@ void check_unaligned_access(int cpu)
      page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));

Quoted 9 lines

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 575

Quoted 4 lines

arch/riscv/kernel/cpufeature.c +1 -1

      if (!page) {
              pr_warn("Can't alloc pages to measure memcpy performance");
-             return;
+             return 0;

Dumb question maybe, but I am limited setup wise at the moment due to a hardware failure which makes checking stuff hard, why the signature change? Requirement for on_each_cpu()?

Requirement for smp_call_on_cpu.

Quoted 17 lines

Quoted 17 lines

arch/riscv/kernel/cpufeature.c +7 -0

      }
 
      /* Make an unaligned destination buffer. */

@@ -643,15 +644,26 @@ void check_unaligned_access(int cpu)
 
 out:
      __free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
+     return 0;
+}
+
+static void check_unaligned_access_nonboot_cpu(void *param)
+{
+     if (smp_processor_id() != 0)
+             check_unaligned_access(param);
 }
 

Quoted 4 lines

Quoted 4 lines

arch/riscv/kernel/cpufeature.c +1 -2

-static int check_unaligned_access_boot_cpu(void)
+static int check_unaligned_access_all_cpus(void)
 {
-     check_unaligned_access(0);

Quoted 9 lines

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 663

Quoted 6 lines

arch/riscv/kernel/cpufeature.c +5 -0

+     /* Check everybody except 0, who stays behind to tend jiffies. */
+     on_each_cpu(check_unaligned_access_nonboot_cpu, NULL, 1);
+
+     /* Check core 0. */
+     smp_call_on_cpu(0, check_unaligned_access, NULL, true);
      return 0;

Why does this function return an int if it can only return 0?

Should we define a check_unaligned_access_boot_cpu to avoid the return type change ?
We can also get rid of the unused parameter in the check_unaligned_access function.

I also noticed on_each_cpu invokes the function within preempt_disable/enable. check_unaligned_access will invoke it again. It's probably harmless but there is no need for non-boot cpus.

We can just have preempt_disable/enable around check_unaligned_access in the check_unaligned_access_boot_cpu function.

Quoted 79 lines · Atish Patra

Quoted 67 lines · Conor Dooley

Yo,

Quoted 63 lines · Evan Green

Probing for misaligned access speed takes about 0.06 seconds. On a system with 64 cores, doing this in smp_callin() means it's done serially, extending boot time by 3.8 seconds. That's a lot of boot time.

Instead of measuring each CPU serially, let's do the measurements on all CPUs in parallel. If we disable preemption on all CPUs, the jiffies stop ticking, so we can do this in stages of 1) everybody except core 0, then 2) core 0.

The measurement call in smp_callin() stays around, but is now conditionalized to only run if a new CPU shows up after the round of in-parallel measurements has run. The goal is to have the measurement call not run during boot or suspend/resume, but only on a hotplug addition.

Reported-by Jisheng Zhang <[email protected]>

Closes https://lore.kernel.org/all/mhng-9359993d-6872-4134-83ce-c97debe1cf9a@palmer-ri-x1c9/T/#mae9b8f40016f9df428829d33360144dc5026bcbf open here

Fixes 584ea6564bca ("RISC-V: Probe for unaligned access speed")

Signed-off-by Evan Green <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Tested-by Andrew Jones <[email protected]>

---

Changes in v2:
- Removed new global, used system_state == SYSTEM_RUNNING instead (Jisheng)
- Added tags

 arch/riscv/include/asm/cpufeature.h |  2 +-
 arch/riscv/kernel/cpufeature.c      | 22 +++++++++++++++++-----
 arch/riscv/kernel/smpboot.c         | 11 ++++++++++-
 3 files changed, 28 insertions(+), 7 deletions(-)

arch/riscv/include/asm/cpufeature.h +1 -1

index d0345bd659c9..b139796392d0 100644
@@ -30,6 +30,6 @@ DECLARE_PER_CPU(long, misaligned_access_speed);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
-void check_unaligned_access(int cpu);
+int check_unaligned_access(void *unused);
 
 #endif

arch/riscv/kernel/cpufeature.c +2 -1

index 1cfbba65d11a..40bb854fcb96 100644
@@ -556,8 +556,9 @@ unsigned long riscv_get_elf_hwcap(void)
      return hwcap;
 }
 
-void check_unaligned_access(int cpu)
+int check_unaligned_access(void *unused)
 {
+     int cpu = smp_processor_id();
      u64 start_cycles, end_cycles;
      u64 word_cycles;
      u64 byte_cycles;

@@ -571,7 +572,7 @@ void check_unaligned_access(int cpu)
      page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));

Quoted 9 lines

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 575

Quoted 4 lines

arch/riscv/kernel/cpufeature.c +1 -1

      if (!page) {
              pr_warn("Can't alloc pages to measure memcpy performance");
-             return;
+             return 0;

Dumb question maybe, but I am limited setup wise at the moment due to a hardware failure which makes checking stuff hard, why the signature change? Requirement for on_each_cpu()?

Quoted 1 line

Requirement for smp_call_on_cpu.

Right.

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

arch/riscv/kernel/cpufeature.c +7 -0

      }
 
      /* Make an unaligned destination buffer. */

@@ -643,15 +644,26 @@ void check_unaligned_access(int cpu)
 
 out:
      __free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
+     return 0;
+}
+
+static void check_unaligned_access_nonboot_cpu(void *param)
+{
+     if (smp_processor_id() != 0)
+             check_unaligned_access(param);
 }
 

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

arch/riscv/kernel/cpufeature.c +1 -2

-static int check_unaligned_access_boot_cpu(void)
+static int check_unaligned_access_all_cpus(void)
 {
-     check_unaligned_access(0);

Quoted 9 lines

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 663

Quoted 6 lines

arch/riscv/kernel/cpufeature.c +5 -0

+     /* Check everybody except 0, who stays behind to tend jiffies. */
+     on_each_cpu(check_unaligned_access_nonboot_cpu, NULL, 1);
+
+     /* Check core 0. */
+     smp_call_on_cpu(0, check_unaligned_access, NULL, true);
      return 0;

Why does this function return an int if it can only return 0?

This is a requirement on the initcall_t function pointer type.

Ahh great, thanks for the explanations!

Quoted 79 lines · Atish Patra

Quoted 67 lines · Conor Dooley

Yo,

Quoted 63 lines · Evan Green

Probing for misaligned access speed takes about 0.06 seconds. On a system with 64 cores, doing this in smp_callin() means it's done serially, extending boot time by 3.8 seconds. That's a lot of boot time.

Instead of measuring each CPU serially, let's do the measurements on all CPUs in parallel. If we disable preemption on all CPUs, the jiffies stop ticking, so we can do this in stages of 1) everybody except core 0, then 2) core 0.

The measurement call in smp_callin() stays around, but is now conditionalized to only run if a new CPU shows up after the round of in-parallel measurements has run. The goal is to have the measurement call not run during boot or suspend/resume, but only on a hotplug addition.

Reported-by Jisheng Zhang <[email protected]>

Closes https://lore.kernel.org/all/mhng-9359993d-6872-4134-83ce-c97debe1cf9a@palmer-ri-x1c9/T/#mae9b8f40016f9df428829d33360144dc5026bcbf open here

Fixes 584ea6564bca ("RISC-V: Probe for unaligned access speed")

Signed-off-by Evan Green <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Tested-by Andrew Jones <[email protected]>

---

Changes in v2:
- Removed new global, used system_state == SYSTEM_RUNNING instead (Jisheng)
- Added tags

 arch/riscv/include/asm/cpufeature.h |  2 +-
 arch/riscv/kernel/cpufeature.c      | 22 +++++++++++++++++-----
 arch/riscv/kernel/smpboot.c         | 11 ++++++++++-
 3 files changed, 28 insertions(+), 7 deletions(-)

arch/riscv/include/asm/cpufeature.h +1 -1

index d0345bd659c9..b139796392d0 100644
@@ -30,6 +30,6 @@ DECLARE_PER_CPU(long, misaligned_access_speed);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
-void check_unaligned_access(int cpu);
+int check_unaligned_access(void *unused);
 
 #endif

arch/riscv/kernel/cpufeature.c +2 -1

index 1cfbba65d11a..40bb854fcb96 100644
@@ -556,8 +556,9 @@ unsigned long riscv_get_elf_hwcap(void)
      return hwcap;
 }
 
-void check_unaligned_access(int cpu)
+int check_unaligned_access(void *unused)
 {
+     int cpu = smp_processor_id();
      u64 start_cycles, end_cycles;
      u64 word_cycles;
      u64 byte_cycles;

@@ -571,7 +572,7 @@ void check_unaligned_access(int cpu)
      page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));

Quoted 9 lines

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 575

Quoted 4 lines

arch/riscv/kernel/cpufeature.c +1 -1

      if (!page) {
              pr_warn("Can't alloc pages to measure memcpy performance");
-             return;
+             return 0;

Dumb question maybe, but I am limited setup wise at the moment due to a hardware failure which makes checking stuff hard, why the signature change? Requirement for on_each_cpu()?

Quoted 1 line

Requirement for smp_call_on_cpu.

Right.

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

arch/riscv/kernel/cpufeature.c +7 -0

      }
 
      /* Make an unaligned destination buffer. */

@@ -643,15 +644,26 @@ void check_unaligned_access(int cpu)
 
 out:
      __free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
+     return 0;
+}
+
+static void check_unaligned_access_nonboot_cpu(void *param)
+{
+     if (smp_processor_id() != 0)
+             check_unaligned_access(param);
 }
 

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

arch/riscv/kernel/cpufeature.c +1 -2

-static int check_unaligned_access_boot_cpu(void)
+static int check_unaligned_access_all_cpus(void)
 {
-     check_unaligned_access(0);

Quoted 9 lines

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 663

Quoted 6 lines

arch/riscv/kernel/cpufeature.c +5 -0

+     /* Check everybody except 0, who stays behind to tend jiffies. */
+     on_each_cpu(check_unaligned_access_nonboot_cpu, NULL, 1);
+
+     /* Check core 0. */
+     smp_call_on_cpu(0, check_unaligned_access, NULL, true);
      return 0;

Why does this function return an int if it can only return 0?

This is a requirement on the initcall_t function pointer type.

Quoted 1 line

Quoted 4 lines

Should we define a check_unaligned_access_boot_cpu to avoid the return type change ?
We can also get rid of the unused parameter in the check_unaligned_access function.

I don't think it matters too much either way. In my mind it sorta shifts the ballast around but doesn't really decrease it.

Quoted 5 lines

I also noticed on_each_cpu invokes the function within preempt_disable/enable. check_unaligned_access will invoke it again. It's probably harmless but there is no need for non-boot cpus.

Quoted 2 lines

We can just have preempt_disable/enable around check_unaligned_access in the check_unaligned_access_boot_cpu function.

It is harmless. I haven't walked fully through smp_call_on_cpu(), but I'd be intuitively quite surprised if preemption were enabled on those callbacks too (as you'd be saying "run my function on this cpu, but no guarantee it will stay there!"). So in theory I should be able to

remove the preempt_disable/enable entirely. As a side note: I added the smp_call_on_cpu() because I couldn't convince myself the initcalls would stay on cpu 0. In practice they always seem to be.

-Evan

Quoted 124 lines · Evan Green

Quoted 79 lines · Atish Patra

Quoted 67 lines · Conor Dooley

Yo,

Quoted 63 lines · Evan Green

Probing for misaligned access speed takes about 0.06 seconds. On a system with 64 cores, doing this in smp_callin() means it's done serially, extending boot time by 3.8 seconds. That's a lot of boot time.

Instead of measuring each CPU serially, let's do the measurements on all CPUs in parallel. If we disable preemption on all CPUs, the jiffies stop ticking, so we can do this in stages of 1) everybody except core 0, then 2) core 0.

The measurement call in smp_callin() stays around, but is now conditionalized to only run if a new CPU shows up after the round of in-parallel measurements has run. The goal is to have the measurement call not run during boot or suspend/resume, but only on a hotplug addition.

Reported-by Jisheng Zhang <[email protected]>

Closes https://lore.kernel.org/all/mhng-9359993d-6872-4134-83ce-c97debe1cf9a@palmer-ri-x1c9/T/#mae9b8f40016f9df428829d33360144dc5026bcbf open here

Fixes 584ea6564bca ("RISC-V: Probe for unaligned access speed")

Signed-off-by Evan Green <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Tested-by Andrew Jones <[email protected]>

---

Changes in v2:
- Removed new global, used system_state == SYSTEM_RUNNING instead (Jisheng)
- Added tags

 arch/riscv/include/asm/cpufeature.h |  2 +-
 arch/riscv/kernel/cpufeature.c      | 22 +++++++++++++++++-----
 arch/riscv/kernel/smpboot.c         | 11 ++++++++++-
 3 files changed, 28 insertions(+), 7 deletions(-)

arch/riscv/include/asm/cpufeature.h +1 -1

index d0345bd659c9..b139796392d0 100644
@@ -30,6 +30,6 @@ DECLARE_PER_CPU(long, misaligned_access_speed);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
-void check_unaligned_access(int cpu);
+int check_unaligned_access(void *unused);
 
 #endif

arch/riscv/kernel/cpufeature.c +2 -1

index 1cfbba65d11a..40bb854fcb96 100644
@@ -556,8 +556,9 @@ unsigned long riscv_get_elf_hwcap(void)
      return hwcap;
 }
 
-void check_unaligned_access(int cpu)
+int check_unaligned_access(void *unused)
 {
+     int cpu = smp_processor_id();
      u64 start_cycles, end_cycles;
      u64 word_cycles;
      u64 byte_cycles;

@@ -571,7 +572,7 @@ void check_unaligned_access(int cpu)
      page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));

Quoted 9 lines

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 575

Quoted 4 lines

arch/riscv/kernel/cpufeature.c +1 -1

      if (!page) {
              pr_warn("Can't alloc pages to measure memcpy performance");
-             return;
+             return 0;

Dumb question maybe, but I am limited setup wise at the moment due to a hardware failure which makes checking stuff hard, why the signature change? Requirement for on_each_cpu()?

Quoted 1 line

Requirement for smp_call_on_cpu.

Right.

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

arch/riscv/kernel/cpufeature.c +7 -0

      }
 
      /* Make an unaligned destination buffer. */

@@ -643,15 +644,26 @@ void check_unaligned_access(int cpu)
 
 out:
      __free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
+     return 0;
+}
+
+static void check_unaligned_access_nonboot_cpu(void *param)
+{
+     if (smp_processor_id() != 0)
+             check_unaligned_access(param);
 }
 

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

arch/riscv/kernel/cpufeature.c +1 -2

-static int check_unaligned_access_boot_cpu(void)
+static int check_unaligned_access_all_cpus(void)
 {
-     check_unaligned_access(0);

Quoted 9 lines

arch/riscv/kernel/cpufeature.c void check_unaligned_access(int cpu) · line 663

Quoted 6 lines

arch/riscv/kernel/cpufeature.c +5 -0

+     /* Check everybody except 0, who stays behind to tend jiffies. */
+     on_each_cpu(check_unaligned_access_nonboot_cpu, NULL, 1);
+
+     /* Check core 0. */
+     smp_call_on_cpu(0, check_unaligned_access, NULL, true);
      return 0;

Why does this function return an int if it can only return 0?

This is a requirement on the initcall_t function pointer type.

Quoted 5 lines

Should we define a check_unaligned_access_boot_cpu to avoid the return type change ?
We can also get rid of the unused parameter in the check_unaligned_access function.

Quoted 3 lines

I don't think it matters too much either way. In my mind it sorta shifts the ballast around but doesn't really decrease it.

Yeah. But it is just a bit cleaner to avoid unused arguments if we can.

Quoted 6 lines

Quoted 6 lines

I also noticed on_each_cpu invokes the function within preempt_disable/enable. check_unaligned_access will invoke it again. It's probably harmless but there is no need for non-boot cpus.

Quoted 7 lines

Quoted 2 lines

We can just have preempt_disable/enable around check_unaligned_access in the check_unaligned_access_boot_cpu function.

It is harmless. I haven't walked fully through smp_call_on_cpu(), but I'd be intuitively quite surprised if preemption were enabled on those callbacks too (as you'd be saying "run my function on this cpu, but no guarantee it will stay there!"). So in theory I should be able to

I didn't see anything explicitly disable/enable preemption on smp_call_on_cpu. As per my understanding, your queued function is guaranteed to run on that cpu as it is queued on the cpu specific wq but it may be preempted by something else.

For probing alignment speed, you just care about running it on that cpu. Correct ?

Quoted 2 lines

For probing alignment speed, you just care about running it on that cpu. Correct ?

For this we care both about not migrating to other CPUs, and also secondarily minimizing disturbances while the test is being run. Usually I equate pre-emption with migration, but in this case I think the worker threads are bound to that CPU. So I'll keep the preempt_disable/enable where it is, since it's harmless for CPUs other than 0, but useful for 0. I also like it for readability as it highlights the critical section (as a reader, "is preemption disabled" would be one of my first questions when studying this).

...

Quoted 11 lines

Quoted 2 lines

For probing alignment speed, you just care about running it on that cpu. Correct ?

For this we care both about not migrating to other CPUs, and also secondarily minimizing disturbances while the test is being run. Usually I equate pre-emption with migration, but in this case I think the worker threads are bound to that CPU. So I'll keep the preempt_disable/enable where it is, since it's harmless for CPUs other than 0, but useful for 0. I also like it for readability as it highlights the critical section (as a reader, "is preemption disabled" would be one of my first questions when studying this).

You need to disable pre-emption to get any kind of meaningful answer.

But why do you need to run the test on more than the boot cpu? If you've a heterogenous mix of cpu any code that looks at the answer is going to behave incorrectly unless it has also disabled pre-emption or is bound to a cpu.

One obvious use of the result is to setup some static branches. But that assumes all cpu are the same.

Quoted 14 lines · David Laight

...

Quoted 11 lines

Quoted 2 lines

For probing alignment speed, you just care about running it on that cpu. Correct ?

For this we care both about not migrating to other CPUs, and also secondarily minimizing disturbances while the test is being run. Usually I equate pre-emption with migration, but in this case I think the worker threads are bound to that CPU. So I'll keep the preempt_disable/enable where it is, since it's harmless for CPUs other than 0, but useful for 0. I also like it for readability as it highlights the critical section (as a reader, "is preemption disabled" would be one of my first questions when studying this).

Quoted 6 lines

You need to disable pre-emption to get any kind of meaningful answer.

But why do you need to run the test on more than the boot cpu? If you've a heterogenous mix of cpu any code that looks at the answer is going to behave incorrectly unless it has also disabled pre-emption or is bound to a cpu.

I don't think it's safe to assume misaligned access speed is the same across all cores. In a big.little combination I can easily imagine the big cores having fast misaligned access and the slow cores not having it (though hopefully the slow cores don't kick it to firmware). Since this info is presented to usermode per-cpu, I'd like it to be correct.

Quoted 1 line

Quoted 2 lines

One obvious use of the result is to setup some static branches. But that assumes all cpu are the same.

Right, this could be used to set up static branches, or in an ifunc selector. This is why we provide pre-computed answers for "all CPUs" in hwprobe. If the situation I describe above did happen, code asking

Probing for misaligned access speed takes about 0.06 seconds. On a system with 64 cores, doing this in smp_callin() means it's done serially, extending boot time by 3.8 seconds. That's a lot of boot time.

Instead of measuring each CPU serially, let's do the measurements on all CPUs in parallel. If we disable preemption on all CPUs, the jiffies stop ticking, so we can do this in stages of 1) everybody except core 0, then 2) core 0.

The measurement call in smp_callin() stays around, but is now conditionalized to only run if a new CPU shows up after the round of in-parallel measurements has run. The goal is to have the measurement call not run during boot or suspend/resume, but only on a hotplug addition.

Reported-by Jisheng Zhang <[email protected]>

Closes https://lore.kernel.org/all/mhng-9359993d-6872-4134-83ce-c97debe1cf9a@palmer-ri-x1c9/T/#mae9b8f40016f9df428829d33360144dc5026bcbf open here

Fixes 584ea6564bca ("RISC-V: Probe for unaligned access speed")

Signed-off-by Evan Green <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Tested-by Andrew Jones <[email protected]>

---

Changes in v2:
- Removed new global, used system_state == SYSTEM_RUNNING instead (Jisheng)
- Added tags

 arch/riscv/include/asm/cpufeature.h |  2 +-
 arch/riscv/kernel/cpufeature.c      | 22 +++++++++++++++++-----
 arch/riscv/kernel/smpboot.c         | 11 ++++++++++-
 3 files changed, 28 insertions(+), 7 deletions(-)

arch/riscv/include/asm/cpufeature.h +1 -1

index d0345bd659c9..b139796392d0 100644
@@ -30,6 +30,6 @@ DECLARE_PER_CPU(long, misaligned_access_speed);
 /* Per-cpu ISA extensions. */
 extern struct riscv_isainfo hart_isa[NR_CPUS];
 
-void check_unaligned_access(int cpu);
+int check_unaligned_access(void *unused);
 
 #endif

arch/riscv/kernel/cpufeature.c +17 -5

index 1cfbba65d11a..40bb854fcb96 100644
@@ -556,8 +556,9 @@ unsigned long riscv_get_elf_hwcap(void)
 	return hwcap;
 }
 
-void check_unaligned_access(int cpu)
+int check_unaligned_access(void *unused)
 {
+	int cpu = smp_processor_id();
 	u64 start_cycles, end_cycles;
 	u64 word_cycles;
 	u64 byte_cycles;

@@ -571,7 +572,7 @@ void check_unaligned_access(int cpu)
 	page = alloc_pages(GFP_NOWAIT, get_order(MISALIGNED_BUFFER_SIZE));
 	if (!page) {
 		pr_warn("Can't alloc pages to measure memcpy performance");
-		return;
+		return 0;
 	}
 
 	/* Make an unaligned destination buffer. */

@@ -643,15 +644,26 @@ void check_unaligned_access(int cpu)
 
 out:
 	__free_pages(page, get_order(MISALIGNED_BUFFER_SIZE));
+	return 0;
+}
+
+static void check_unaligned_access_nonboot_cpu(void *param)
+{
+	if (smp_processor_id() != 0)
+		check_unaligned_access(param);
 }
 
-static int check_unaligned_access_boot_cpu(void)
+static int check_unaligned_access_all_cpus(void)
 {
-	check_unaligned_access(0);
+	/* Check everybody except 0, who stays behind to tend jiffies. */
+	on_each_cpu(check_unaligned_access_nonboot_cpu, NULL, 1);
+
+	/* Check core 0. */
+	smp_call_on_cpu(0, check_unaligned_access, NULL, true);
 	return 0;
 }
 
-arch_initcall(check_unaligned_access_boot_cpu);
+arch_initcall(check_unaligned_access_all_cpus);
 
 #ifdef CONFIG_RISCV_ALTERNATIVE
 /*

arch/riscv/kernel/smpboot.c +10 -1

index 1b8da4e40a4d..a014955b8699 100644
@@ -27,6 +27,7 @@
 #include <linux/sched/mm.h>
 #include <asm/cpu_ops.h>
 #include <asm/cpufeature.h>
+#include <asm/hwprobe.h>
 #include <asm/irq.h>
 #include <asm/mmu_context.h>
 #include <asm/numa.h>

@@ -246,7 +247,15 @@ asmlinkage __visible void smp_callin(void)
 
 	numa_add_cpu(curr_cpuid);
 	set_cpu_online(curr_cpuid, 1);
-	check_unaligned_access(curr_cpuid);
+
+	/*
+	 * Boot-time misaligned access speed measurements are done in parallel
+	 * in an initcall. Only measure here for hotplug.
+	 */
+	if ((system_state == SYSTEM_RUNNING) &&
+	    (per_cpu(misaligned_access_speed, curr_cpuid) == RISCV_HWPROBE_MISALIGNED_UNKNOWN)) {
+		check_unaligned_access(NULL);
+	}
 
 	if (has_vector()) {
 		if (riscv_v_setup_vsize())

Signature · 1 line

2.34.1