arch/riscv/include/asm/cpufeature.h | 3 ++- arch/riscv/kernel/cpufeature.c | 28 +++++++++++++++++++++++----- arch/riscv/kernel/smpboot.c | 11 ++++++++++- 3 files changed, 35 insertions(+), 7 deletions(-)
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.
Signed-off-by: Evan Green <evan@rivosinc.com>
---
Jisheng, I didn't add your Tested-by tag since the patch evolved from
the one you tested. Hopefully this one brings you the same result.
---
arch/riscv/include/asm/cpufeature.h | 3 ++-
arch/riscv/kernel/cpufeature.c | 28 +++++++++++++++++++++++-----
arch/riscv/kernel/smpboot.c | 11 ++++++++++-
3 files changed, 35 insertions(+), 7 deletions(-)
diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
index d0345bd659c9..19e7817eba10 100644
--- a/arch/riscv/include/asm/cpufeature.h
+++ b/arch/riscv/include/asm/cpufeature.h
@@ -30,6 +30,7 @@ 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);
+extern bool misaligned_speed_measured;
+int check_unaligned_access(void *unused);
#endif
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 1cfbba65d11a..8eb36e1dfb95 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -42,6 +42,9 @@ struct riscv_isainfo hart_isa[NR_CPUS];
/* Performance information */
DEFINE_PER_CPU(long, misaligned_access_speed);
+/* Boot-time in-parallel unaligned access measurement has occurred. */
+bool misaligned_speed_measured;
+
/**
* riscv_isa_extension_base() - Get base extension word
*
@@ -556,8 +559,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 +575,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 +647,29 @@ 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);
+
+ /* Boot-time measurements are complete. */
+ misaligned_speed_measured = true;
return 0;
}
-arch_initcall(check_unaligned_access_boot_cpu);
+arch_initcall(check_unaligned_access_all_cpus);
#ifdef CONFIG_RISCV_ALTERNATIVE
/*
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 1b8da4e40a4d..39322ae20a75 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -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 (misaligned_speed_measured &&
+ (per_cpu(misaligned_access_speed, curr_cpuid) == RISCV_HWPROBE_MISALIGNED_UNKNOWN)) {
+ check_unaligned_access(NULL);
+ }
if (has_vector()) {
if (riscv_v_setup_vsize())
--
2.34.1On Fri, Sep 15, 2023 at 11:49 AM Evan Green <evan@rivosinc.com> wrote: > > 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. > > Signed-off-by: Evan Green <evan@rivosinc.com> Shoot, I saw the other thread [1] where it seems like my use of alloc_pages() in this context is improper? I had thought I was alright, as Documentation/core-api/memory-allocation.rst says: > If the allocation is performed from an atomic context, e.g interrupt > handler, use ``GFP_NOWAIT``. Any tips for reproducing that splat? I have CONFIG_DEBUG_ATOMIC_SLEEP on (it's in the defconfig), and lockdep, and I'm on Conor's linux-6.6.y-rt, but so far I'm not seeing it. -Evan [1] https://lore.kernel.org/linux-riscv/ZUPWc7sY47l34lV+@xhacker/T/#t
On Thu, Nov 02, 2023 at 03:41:58PM -0700, Evan Green wrote: > On Fri, Sep 15, 2023 at 11:49 AM Evan Green <evan@rivosinc.com> wrote: > > > > 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. > > > > Signed-off-by: Evan Green <evan@rivosinc.com> > > Shoot, I saw the other thread [1] where it seems like my use of > alloc_pages() in this context is improper? I had thought I was > alright, as Documentation/core-api/memory-allocation.rst says: > > > If the allocation is performed from an atomic context, e.g interrupt > > handler, use ``GFP_NOWAIT``. > > Any tips for reproducing that splat? I have CONFIG_DEBUG_ATOMIC_SLEEP > on (it's in the defconfig), and lockdep, and I'm on Conor's > linux-6.6.y-rt, but so far I'm not seeing it. It was originally produced in hardware, but I can also see these issues in QEMU's emulation of my hardware (although as you may have seen, I get them both with and without this patch). My qemu incantation was something like: $(qemu) -M microchip-icicle-kit \ -m 3G -smp 5 \ -kernel vmlinux.bin \ -dtb mpfs-icicle.dtb \ -initrd initramfs \ -display none -serial null \ -serial stdio \ -D qemu.log -d unimp Where the kernel was built from the .config in that branch in my repo. Cheers, Conor.
On Fri, Sep 15, 2023 at 11:49:03AM -0700, Evan Green wrote:
> 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.
>
> Signed-off-by: Evan Green <evan@rivosinc.com>
Reported-by: Jisheng Zhang <jszhang@kernel.org>
>
> ---
>
> Jisheng, I didn't add your Tested-by tag since the patch evolved from
> the one you tested. Hopefully this one brings you the same result.
>
> ---
> arch/riscv/include/asm/cpufeature.h | 3 ++-
> arch/riscv/kernel/cpufeature.c | 28 +++++++++++++++++++++++-----
> arch/riscv/kernel/smpboot.c | 11 ++++++++++-
> 3 files changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index d0345bd659c9..19e7817eba10 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -30,6 +30,7 @@ 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);
> +extern bool misaligned_speed_measured;
> +int check_unaligned_access(void *unused);
>
> #endif
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 1cfbba65d11a..8eb36e1dfb95 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -42,6 +42,9 @@ struct riscv_isainfo hart_isa[NR_CPUS];
> /* Performance information */
> DEFINE_PER_CPU(long, misaligned_access_speed);
>
> +/* Boot-time in-parallel unaligned access measurement has occurred. */
> +bool misaligned_speed_measured;
This var can be avoided, see below.
> +
> /**
> * riscv_isa_extension_base() - Get base extension word
> *
> @@ -556,8 +559,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 +575,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 +647,29 @@ 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);
> +
> + /* Boot-time measurements are complete. */
> + misaligned_speed_measured = true;
> return 0;
> }
>
> -arch_initcall(check_unaligned_access_boot_cpu);
> +arch_initcall(check_unaligned_access_all_cpus);
>
> #ifdef CONFIG_RISCV_ALTERNATIVE
> /*
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 1b8da4e40a4d..39322ae20a75 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -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 (misaligned_speed_measured &&
> + (per_cpu(misaligned_access_speed, curr_cpuid) == RISCV_HWPROBE_MISALIGNED_UNKNOWN)) {
I believe this check is for cpu not-booted during boot time but hotplug in
after that, if so I'm not sure whether
misaligned_speed_measured can be replaced with
(system_state == SYSTEM_RUNNING)
then we don't need misaligned_speed_measured at all.
> + check_unaligned_access(NULL);
> + }
>
> if (has_vector()) {
> if (riscv_v_setup_vsize())
> --
> 2.34.1
>
On Fri, Sep 15, 2023 at 11:49:03AM -0700, Evan Green wrote:
> 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.
Yay! I had just recently tested suspend/resume and wanted to report the
probe as an issue, but I hadn't gotten around to it. This patch resolves
the issue, so
Test-by: Andrew Jones <ajones@ventanamicro.com>
>
> Signed-off-by: Evan Green <evan@rivosinc.com>
>
> ---
>
> Jisheng, I didn't add your Tested-by tag since the patch evolved from
> the one you tested. Hopefully this one brings you the same result.
>
> ---
> arch/riscv/include/asm/cpufeature.h | 3 ++-
> arch/riscv/kernel/cpufeature.c | 28 +++++++++++++++++++++++-----
> arch/riscv/kernel/smpboot.c | 11 ++++++++++-
> 3 files changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index d0345bd659c9..19e7817eba10 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -30,6 +30,7 @@ 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);
> +extern bool misaligned_speed_measured;
Do we need this new state or could we just always check the boot cpu's
state to get the same information?
per_cpu(misaligned_access_speed, 0) != RISCV_HWPROBE_MISALIGNED_UNKNOWN
> +int check_unaligned_access(void *unused);
>
> #endif
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 1cfbba65d11a..8eb36e1dfb95 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -42,6 +42,9 @@ struct riscv_isainfo hart_isa[NR_CPUS];
> /* Performance information */
> DEFINE_PER_CPU(long, misaligned_access_speed);
>
> +/* Boot-time in-parallel unaligned access measurement has occurred. */
> +bool misaligned_speed_measured;
> +
> /**
> * riscv_isa_extension_base() - Get base extension word
> *
> @@ -556,8 +559,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 +575,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 +647,29 @@ 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);
> +
> + /* Boot-time measurements are complete. */
> + misaligned_speed_measured = true;
> return 0;
> }
>
> -arch_initcall(check_unaligned_access_boot_cpu);
> +arch_initcall(check_unaligned_access_all_cpus);
>
> #ifdef CONFIG_RISCV_ALTERNATIVE
> /*
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 1b8da4e40a4d..39322ae20a75 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -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 (misaligned_speed_measured &&
> + (per_cpu(misaligned_access_speed, curr_cpuid) == RISCV_HWPROBE_MISALIGNED_UNKNOWN)) {
> + check_unaligned_access(NULL);
> + }
>
> if (has_vector()) {
> if (riscv_v_setup_vsize())
> --
> 2.34.1
>
Besides my reluctance to add another global variable, this looks good to
me.
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Thanks,
drew
Yo Evan, On Fri, Sep 15, 2023 at 11:49:03AM -0700, Evan Green wrote: > 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. > > Signed-off-by: Evan Green <evan@rivosinc.com> > > --- > > Jisheng, I didn't add your Tested-by tag since the patch evolved from > the one you tested. Hopefully this one brings you the same result. Ya know, I think there's scope to add Reported-by:, Closes: and Fixes: tags to this patch, mentioning explicitly that this has regressed boot time for many core systems, so that this can be fixes material. What do you think? > --- > arch/riscv/include/asm/cpufeature.h | 3 ++- > arch/riscv/kernel/cpufeature.c | 28 +++++++++++++++++++++++----- > arch/riscv/kernel/smpboot.c | 11 ++++++++++- > 3 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h > index d0345bd659c9..19e7817eba10 100644 > --- a/arch/riscv/include/asm/cpufeature.h > +++ b/arch/riscv/include/asm/cpufeature.h > @@ -30,6 +30,7 @@ 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); > +extern bool misaligned_speed_measured; > +int check_unaligned_access(void *unused); > > #endif > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index 1cfbba65d11a..8eb36e1dfb95 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -42,6 +42,9 @@ struct riscv_isainfo hart_isa[NR_CPUS]; > /* Performance information */ > DEFINE_PER_CPU(long, misaligned_access_speed); > > +/* Boot-time in-parallel unaligned access measurement has occurred. */ > +bool misaligned_speed_measured; If you did something like s/measured/complete/ I think you could drop the comment. Tis whatever though :) Conor.
© 2016 - 2026 Red Hat, Inc.