| Message ID | 20220629205852.4172212-1-nathanl@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] powerpc/smp: poll cpu_callin_map more aggressively in __cpu_up() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
Nathan Lynch <nathanl@linux.ibm.com> writes: > > Without the msleep(1) in the hotplug path, the time it takes to online a > CPU on a P9 PowerVM LPAR goes from roughly 30ms to 4ms or less when > exercised via thaw_secondary_cpus(). I need to reword this part. More testing shows that the time for _cpu_up() to complete increases with the number of CPUs online, with or without this change. The patch eliminates the msleep() penalty (roughly 20ms) but does not provide a consistent order-of-magnitude improvement, unfortunately. That is, even with this change, the time to online creeps back up into the tens of milliseconds after a few dozen CPUs: # dmesg --reltime | grep -A88 'Enabling non' [ +5.000006] Enabling non-boot CPUs ... [ +0.000894] CPU1 is up [ +0.000613] CPU2 is up [ +0.000697] CPU3 is up [ +0.000894] CPU4 is up [ +0.000956] CPU5 is up [ +0.001110] CPU6 is up [ +0.001497] CPU7 is up [ +0.001606] CPU8 is up [ +0.001841] CPU9 is up [ +0.001998] CPU10 is up [ +0.002229] CPU11 is up [ +0.002488] CPU12 is up [ +0.002618] CPU13 is up [ +0.002767] CPU14 is up [ +0.002931] CPU15 is up [ +0.003283] CPU16 is up [ +0.003220] CPU17 is up [ +0.003427] CPU18 is up [ +0.003648] CPU19 is up [ +0.003870] CPU20 is up [ +0.004044] CPU21 is up [ +0.004246] CPU22 is up [ +0.004420] CPU23 is up [ +0.008634] CPU24 is up [ +0.006166] CPU25 is up [ +0.006373] CPU26 is up [ +0.006603] CPU27 is up [ +0.006913] CPU28 is up [ +0.007031] CPU29 is up [ +0.007287] CPU30 is up [ +0.007570] CPU31 is up [ +0.008670] CPU32 is up [ +0.007793] CPU33 is up [ +0.008104] CPU34 is up [ +0.008440] CPU35 is up [ +0.008358] CPU36 is up [ +0.008386] CPU37 is up [ +0.008696] CPU38 is up [ +0.009104] CPU39 is up [ +0.009676] CPU40 is up [ +0.009687] CPU41 is up [ +0.009761] CPU42 is up [ +0.010089] CPU43 is up [ +0.010362] CPU44 is up [ +0.010683] CPU45 is up [ +0.011054] CPU46 is up [ +0.011399] CPU47 is up [ +0.012013] CPU48 is up [ +0.011597] CPU49 is up [ +0.011791] CPU50 is up [ +0.012115] CPU51 is up [ +0.012415] CPU52 is up [ +0.012954] CPU53 is up [ +0.013131] CPU54 is up [ +0.013244] CPU55 is up [ +0.013775] CPU56 is up [ +0.013599] CPU57 is up [ +0.013867] CPU58 is up [ +0.014255] CPU59 is up [ +0.014563] CPU60 is up [ +0.014904] CPU61 is up [ +0.015125] CPU62 is up [ +0.015360] CPU63 is up [ +0.015923] CPU64 is up [ +0.015721] CPU65 is up [ +0.016026] CPU66 is up [ +0.016395] CPU67 is up [ +0.016811] CPU68 is up [ +0.017045] CPU69 is up [ +0.017270] CPU70 is up [ +0.017613] CPU71 is up [ +0.018072] CPU72 is up [ +0.017913] CPU73 is up [ +0.018184] CPU74 is up [ +0.018519] CPU75 is up [ +0.018929] CPU76 is up [ +0.019179] CPU77 is up [ +0.019470] CPU78 is up [ +0.019766] CPU79 is up [ +0.020331] CPU80 is up [ +0.020244] CPU81 is up [ +0.020464] CPU82 is up [ +0.020876] CPU83 is up [ +0.021229] CPU84 is up [ +0.021542] CPU85 is up [ +0.021903] CPU86 is up [ +0.022062] CPU87 is up
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index bcefab484ea6..c8431074d590 100644 --- a/arch/powerpc/kernel/smp.c +++ b/arch/powerpc/kernel/smp.c @@ -23,6 +23,7 @@ #include <linux/delay.h> #include <linux/init.h> #include <linux/spinlock.h> +#include <linux/jiffies.h> #include <linux/cache.h> #include <linux/err.h> #include <linux/device.h> @@ -1268,7 +1269,8 @@ static void cpu_idle_thread_init(unsigned int cpu, struct task_struct *idle) int __cpu_up(unsigned int cpu, struct task_struct *tidle) { - int rc, c; + unsigned long deadline; + int rc; /* * Don't allow secondary threads to come online if inhibited @@ -1313,23 +1315,10 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle) } /* - * wait to see if the cpu made a callin (is actually up). - * use this value that I found through experimentation. - * -- Cort + * Give the remote CPU five seconds to enter the kernel. */ - if (system_state < SYSTEM_RUNNING) - for (c = 50000; c && !cpu_callin_map[cpu]; c--) - udelay(100); -#ifdef CONFIG_HOTPLUG_CPU - else - /* - * CPUs can take much longer to come up in the - * hotplug case. Wait five seconds. - */ - for (c = 5000; c && !cpu_callin_map[cpu]; c--) - msleep(1); -#endif - + deadline = jiffies + msecs_to_jiffies(5000); + spin_until_cond(cpu_callin_map[cpu] != 0 || time_after(jiffies, deadline)); if (!cpu_callin_map[cpu]) { printk(KERN_ERR "Processor %u is stuck.\n", cpu); return -ENOENT;
It is not necessary to delay or sleep between polls of cpu_callin_map when waiting for a kicked CPU to come up. We can use spin_until_cond(), combining the boot and hotplug paths while preserving the intended timeout. Without the msleep(1) in the hotplug path, the time it takes to online a CPU on a P9 PowerVM LPAR goes from roughly 30ms to 4ms or less when exercised via thaw_secondary_cpus(). Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> --- Notes: Changes since v1: * Do not poll indefinitely; restore the original 5sec timeout "Benchmark" method, on a LPAR with 24 CPUs: $ echo processors > /sys/power/pm_test $ echo mem > /sys/power/state $ dmesg --reltime | grep -A23 'Enabling non-boot CPUs' Before: [ +5.000003] Enabling non-boot CPUs ... [ +0.047537] CPU1 is up [ +0.030177] CPU2 is up [ +0.030082] CPU3 is up [ +0.030146] CPU4 is up [ +0.030114] CPU5 is up [ +0.030108] CPU6 is up [ +0.030134] CPU7 is up [ +0.040272] CPU8 is up [ +0.030352] CPU9 is up [ +0.030169] CPU10 is up [ +0.040224] CPU11 is up [ +0.030186] CPU12 is up [ +0.030173] CPU13 is up [ +0.030181] CPU14 is up [ +0.030179] CPU15 is up [ +0.030236] CPU16 is up [ +0.030012] CPU17 is up [ +0.030188] CPU18 is up [ +0.030233] CPU19 is up [ +0.030182] CPU20 is up [ +0.030160] CPU21 is up [ +0.030226] CPU22 is up [ +0.030125] CPU23 is up After: [ +5.000004] Enabling non-boot CPUs ... [ +0.000956] CPU1 is up [ +0.000568] CPU2 is up [ +0.000682] CPU3 is up [ +0.000836] CPU4 is up [ +0.000914] CPU5 is up [ +0.001054] CPU6 is up [ +0.001184] CPU7 is up [ +0.001533] CPU8 is up [ +0.001779] CPU9 is up [ +0.001935] CPU10 is up [ +0.002151] CPU11 is up [ +0.002358] CPU12 is up [ +0.002597] CPU13 is up [ +0.002713] CPU14 is up [ +0.002891] CPU15 is up [ +0.003209] CPU16 is up [ +0.003149] CPU17 is up [ +0.003337] CPU18 is up [ +0.003552] CPU19 is up [ +0.003765] CPU20 is up [ +0.003922] CPU21 is up [ +0.004104] CPU22 is up [ +0.004266] CPU23 is up arch/powerpc/kernel/smp.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-)