From bba9cbe37475c50ea6477de2d244ea81bc29c012 Mon Sep 17 00:00:00 2001 From: Conrad Meyer Date: Mon, 7 Jan 2019 19:39:31 +0000 Subject: [PATCH] powerpc: Fix regression introduced in r342771 In r342771, I introduced a regression in Power by abusing the platform smp_topo() method as a shortcut for providing the MI information needed for the stated sysctls. The smp_topo() method was already called later by sched_ule (under the name cpu_topo()), and initializes a static array of scheduler topology information. I had skimmed the smp_topo_foo() functions and assumed they were idempotent; empirically, they are not (or at least, detect re-initialization and panic). Do the cleaner thing I should have done in the first place and add a platform method specifically for core- and thread-count probing. Reported by: luporl via jhibbits Reviewed by: luporl X-MFC-With: r342771 Differential Revision: https://reviews.freebsd.org/D18777 --- sys/powerpc/include/platform.h | 1 + sys/powerpc/powernv/platform_powernv.c | 23 +++++++++++++-------- sys/powerpc/powerpc/mp_machdep.c | 3 +-- sys/powerpc/powerpc/platform.c | 6 ++++++ sys/powerpc/powerpc/platform_if.m | 11 ++++++++++ sys/powerpc/ps3/platform_ps3.c | 11 ++++++++-- sys/powerpc/pseries/platform_chrp.c | 28 +++++++++++++++++--------- 7 files changed, 61 insertions(+), 22 deletions(-) diff --git a/sys/powerpc/include/platform.h b/sys/powerpc/include/platform.h index d34c9c2d1e4b..7fa8dfa8d9d6 100644 --- a/sys/powerpc/include/platform.h +++ b/sys/powerpc/include/platform.h @@ -58,6 +58,7 @@ int platform_smp_get_bsp(struct cpuref *); int platform_smp_start_cpu(struct pcpu *); void platform_smp_timebase_sync(u_long tb, int ap); void platform_smp_ap_init(void); +void platform_smp_probe_threads(void); const char *installed_platform(void); void platform_probe_and_attach(void); diff --git a/sys/powerpc/powernv/platform_powernv.c b/sys/powerpc/powernv/platform_powernv.c index 45078dae72a9..afcb3d5b90b8 100644 --- a/sys/powerpc/powernv/platform_powernv.c +++ b/sys/powerpc/powernv/platform_powernv.c @@ -71,6 +71,7 @@ static int powernv_smp_get_bsp(platform_t, struct cpuref *cpuref); static void powernv_smp_ap_init(platform_t); #ifdef SMP static int powernv_smp_start_cpu(platform_t, struct pcpu *cpu); +static void powernv_smp_probe_threads(platform_t); static struct cpu_group *powernv_smp_topo(platform_t plat); #endif static void powernv_reset(platform_t); @@ -89,6 +90,7 @@ static platform_method_t powernv_methods[] = { PLATFORMMETHOD(platform_smp_get_bsp, powernv_smp_get_bsp), #ifdef SMP PLATFORMMETHOD(platform_smp_start_cpu, powernv_smp_start_cpu), + PLATFORMMETHOD(platform_smp_probe_threads, powernv_smp_probe_threads), PLATFORMMETHOD(platform_smp_topo, powernv_smp_topo), #endif @@ -403,8 +405,8 @@ powernv_smp_start_cpu(platform_t plat, struct pcpu *pc) return (0); } -static struct cpu_group * -powernv_smp_topo(platform_t plat) +static void +powernv_smp_probe_threads(platform_t plat) { char buf[8]; phandle_t cpu, dev, root; @@ -436,21 +438,26 @@ powernv_smp_topo(platform_t plat) } smp_threads_per_core = nthreads; + if (mp_ncpus % nthreads == 0) + mp_ncores = mp_ncpus / nthreads; +} - if (mp_ncpus % nthreads != 0) { +static struct cpu_group * +powernv_smp_topo(platform_t plat) +{ + if (mp_ncpus % smp_threads_per_core != 0) { printf("WARNING: Irregular SMP topology. Performance may be " "suboptimal (%d threads, %d on first core)\n", - mp_ncpus, nthreads); + mp_ncpus, smp_threads_per_core); return (smp_topo_none()); } - mp_ncores = mp_ncpus / nthreads; - /* Don't do anything fancier for non-threaded SMP */ - if (nthreads == 1) + if (smp_threads_per_core == 1) return (smp_topo_none()); - return (smp_topo_1level(CG_SHARE_L1, nthreads, CG_FLAG_SMT)); + return (smp_topo_1level(CG_SHARE_L1, smp_threads_per_core, + CG_FLAG_SMT)); } #endif diff --git a/sys/powerpc/powerpc/mp_machdep.c b/sys/powerpc/powerpc/mp_machdep.c index 883e72d59e22..2a7dd91cfba7 100644 --- a/sys/powerpc/powerpc/mp_machdep.c +++ b/sys/powerpc/powerpc/mp_machdep.c @@ -188,8 +188,7 @@ next: } #ifdef SMP - /* Probe mp_ncores and smp_threads_per_core as a side effect. */ - (void)cpu_topo(); + platform_smp_probe_threads(); #endif } diff --git a/sys/powerpc/powerpc/platform.c b/sys/powerpc/powerpc/platform.c index 9a134a0d04dd..252978f47a31 100644 --- a/sys/powerpc/powerpc/platform.c +++ b/sys/powerpc/powerpc/platform.c @@ -242,6 +242,12 @@ platform_smp_ap_init() PLATFORM_SMP_AP_INIT(plat_obj); } +void +platform_smp_probe_threads(void) +{ + PLATFORM_SMP_PROBE_THREADS(plat_obj); +} + #ifdef SMP struct cpu_group * cpu_topo(void) diff --git a/sys/powerpc/powerpc/platform_if.m b/sys/powerpc/powerpc/platform_if.m index 6b5b64689fb1..33da5cc02985 100644 --- a/sys/powerpc/powerpc/platform_if.m +++ b/sys/powerpc/powerpc/platform_if.m @@ -84,6 +84,10 @@ CODE { { return; } + static void platform_null_smp_probe_threads(void) + { + return; + } }; /** @@ -196,6 +200,13 @@ METHOD void smp_ap_init { platform_t _plat; } DEFAULT platform_null_smp_ap_init; +/** + * @brief Probe mp_ncores and smp_threads_per_core for early MI code + */ +METHOD void smp_probe_threads { + platform_t _plat; +} DEFAULT platform_null_smp_probe_threads; + /** * @brief Return SMP topology */ diff --git a/sys/powerpc/ps3/platform_ps3.c b/sys/powerpc/ps3/platform_ps3.c index 88daf823ee78..104179f7eda2 100644 --- a/sys/powerpc/ps3/platform_ps3.c +++ b/sys/powerpc/ps3/platform_ps3.c @@ -70,6 +70,7 @@ static int ps3_smp_first_cpu(platform_t, struct cpuref *cpuref); static int ps3_smp_next_cpu(platform_t, struct cpuref *cpuref); static int ps3_smp_get_bsp(platform_t, struct cpuref *cpuref); static int ps3_smp_start_cpu(platform_t, struct pcpu *cpu); +static void ps3_smp_probe_threads(platform_t); static struct cpu_group *ps3_smp_topo(platform_t); #endif static void ps3_reset(platform_t); @@ -87,6 +88,7 @@ static platform_method_t ps3_methods[] = { PLATFORMMETHOD(platform_smp_next_cpu, ps3_smp_next_cpu), PLATFORMMETHOD(platform_smp_get_bsp, ps3_smp_get_bsp), PLATFORMMETHOD(platform_smp_start_cpu, ps3_smp_start_cpu), + PLATFORMMETHOD(platform_smp_probe_threads, ps3_smp_probe_threads), PLATFORMMETHOD(platform_smp_topo, ps3_smp_topo), #endif @@ -243,11 +245,16 @@ ps3_smp_start_cpu(platform_t plat, struct pcpu *pc) return ((pc->pc_awake) ? 0 : EBUSY); } -static struct cpu_group * -ps3_smp_topo(platform_t plat) +static void +ps3_smp_probe_threads(platform_t plat) { mp_ncores = 1; smp_threads_per_core = 2; +} + +static struct cpu_group * +ps3_smp_topo(platform_t plat) +{ return (smp_topo_1level(CG_SHARE_L1, 2, CG_FLAG_SMT)); } #endif diff --git a/sys/powerpc/pseries/platform_chrp.c b/sys/powerpc/pseries/platform_chrp.c index cd8c55e704d9..a642c6942071 100644 --- a/sys/powerpc/pseries/platform_chrp.c +++ b/sys/powerpc/pseries/platform_chrp.c @@ -78,6 +78,7 @@ static void chrp_smp_ap_init(platform_t); static int chrp_cpuref_init(void); #ifdef SMP static int chrp_smp_start_cpu(platform_t, struct pcpu *cpu); +static void chrp_smp_probe_threads(platform_t plat); static struct cpu_group *chrp_smp_topo(platform_t plat); #endif static void chrp_reset(platform_t); @@ -103,6 +104,7 @@ static platform_method_t chrp_methods[] = { PLATFORMMETHOD(platform_smp_get_bsp, chrp_smp_get_bsp), #ifdef SMP PLATFORMMETHOD(platform_smp_start_cpu, chrp_smp_start_cpu), + PLATFORMMETHOD(platform_smp_probe_threads, chrp_smp_probe_threads), PLATFORMMETHOD(platform_smp_topo, chrp_smp_topo), #endif @@ -499,13 +501,13 @@ chrp_smp_start_cpu(platform_t plat, struct pcpu *pc) return ((pc->pc_awake) ? 0 : EBUSY); } -static struct cpu_group * -chrp_smp_topo(platform_t plat) +static void +chrp_smp_probe_threads(platform_t plat) { struct pcpu *pc, *last_pc; - int i, ncores, ncpus; + int i, ncores; - ncores = ncpus = 0; + ncores = 0; last_pc = NULL; for (i = 0; i <= mp_maxid; i++) { pc = pcpu_find(i); @@ -514,23 +516,29 @@ chrp_smp_topo(platform_t plat) if (last_pc == NULL || pc->pc_hwref != last_pc->pc_hwref) ncores++; last_pc = pc; - ncpus++; } mp_ncores = ncores; + if (mp_ncpus % ncores == 0) + smp_threads_per_core = mp_ncpus / ncores; +} - if (ncpus % ncores != 0) { +static struct cpu_group * +chrp_smp_topo(platform_t plat) +{ + + if (mp_ncpus % mp_ncores != 0) { printf("WARNING: Irregular SMP topology. Performance may be " - "suboptimal (%d CPUS, %d cores)\n", ncpus, ncores); + "suboptimal (%d CPUS, %d cores)\n", mp_ncpus, mp_ncores); return (smp_topo_none()); } /* Don't do anything fancier for non-threaded SMP */ - if (ncpus == ncores) + if (mp_ncpus == mp_ncores) return (smp_topo_none()); - smp_threads_per_core = ncpus / ncores; - return (smp_topo_1level(CG_SHARE_L1, ncpus / ncores, CG_FLAG_SMT)); + return (smp_topo_1level(CG_SHARE_L1, smp_threads_per_core, + CG_FLAG_SMT)); } #endif