* [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop @ 2016-05-12 10:13 Gaurav Jindal (Gaurav Jindal) 2016-05-18 11:47 ` Peter Zijlstra 2016-06-03 10:48 ` [tip:sched/core] sched/idle: Optimize the generic idle loop tip-bot for Gaurav Jindal (Gaurav Jindal) 0 siblings, 2 replies; 6+ messages in thread From: Gaurav Jindal (Gaurav Jindal) @ 2016-05-12 10:13 UTC (permalink / raw) To: peterz, mingo; +Cc: linux-kernel, Sanjeev Yadav (Sanjeev Kumar Yadav) Currently, smp_processor_id() is used to fetch the current cpu in cpu_idle_loop. Everytime the idle thread runs, it fetches current cpu using smp_processor_id(). For idle thread which is per cpu, current cpu is constant and cannot change at runtime. So moving the smp_processor_id() before the loop saves execution cycles/time in loop. With the patch, assembly code(on x-86 and ARM64) to be executed in loop is reduced. X-86 architecture: Before patch(execution in loop): 148: 0f ae e8 lfence 14b: 65 8b 04 25 00 00 00 00 mov %gs:0x0,%eax 152: 00 153: 89 c0 mov %eax,%eax 155: 49 0f a3 04 24 bt %rax,(%r12) After patch(execution in loop): 150: 0f ae e8 lfence 153: 4d 0f a3 34 24 bt %r14,(%r12) For ARM64: Before patch(execution in loop): 168: d5033d9f dsb ld 16c: b9405661 ldr w1,[x19,#84] 170: 1100fc20 add w0,w1,#0x3f 174: 6b1f003f cmp w1,wzr 178: 1a81b000 csel w0,w0,w1,lt 17c: 130c7000 asr w0,w0,#6 180: 937d7c00 sbfiz x0,x0,#3,#32 184: f8606aa0 ldr x0,[x21,x0] 188: 9ac12401 lsr x1,x0,x1 18c: 36000e61 tbz w1,#0,358 After patch(execution in loop): 1a8: d50339df dsb ld 1ac: f8776ac0 ldr x0,[x22,x23] ab0: ea18001f tst x0,x24 1b4: 54000ea0 b.eq 388 Further observance on ARM64 for 4 seconds shows that cpu_idle_loop is called 8672 times. Shifting the code will save instructions executed in loop and eventually time as well. Signed-off-by: Gaurav Jindal<gaurav.jindal@spreadtrum.com> Reviewed-by: Sanjeev Yadav<sanjeev.yadav@spreadtrum.com> --- diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c index 1214f0a..82698e5 100644 --- a/kernel/sched/idle.c +++ b/kernel/sched/idle.c @@ -185,6 +185,8 @@ exit_idle: */ static void cpu_idle_loop(void) { + int cpu_id; + cpu_id = smp_processor_id(); while (1) { /* * If the arch has a polling bit, we maintain an invariant: @@ -202,7 +204,7 @@ static void cpu_idle_loop(void) check_pgt_cache(); rmb(); - if (cpu_is_offline(smp_processor_id())) + if (cpu_is_offline(cpu_id)) arch_cpu_idle_dead(); local_irq_disable(); -- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop 2016-05-12 10:13 [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop Gaurav Jindal (Gaurav Jindal) @ 2016-05-18 11:47 ` Peter Zijlstra 2016-05-18 12:30 ` Peter Zijlstra 2016-06-03 10:48 ` [tip:sched/core] sched/idle: Optimize the generic idle loop tip-bot for Gaurav Jindal (Gaurav Jindal) 1 sibling, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2016-05-18 11:47 UTC (permalink / raw) To: Gaurav Jindal (Gaurav Jindal) Cc: mingo, linux-kernel, Sanjeev Yadav (Sanjeev Kumar Yadav), Thomas Gleixner Much better; but the below does not in fact apply. > --- > > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c > index 1214f0a..82698e5 100644 > --- a/kernel/sched/idle.c > +++ b/kernel/sched/idle.c > @@ -185,6 +185,8 @@ exit_idle: > */ > static void cpu_idle_loop(void) > { > + int cpu_id; > > + cpu_id = smp_processor_id(); This hunk assumes there's an empty line between the '{' and 'while (1)', this isn't actually there. > while (1) { > /* > * If the arch has a polling bit, we maintain an invariant: > @@ -202,7 +204,7 @@ static void cpu_idle_loop(void) > check_pgt_cache(); > rmb(); > > - if (cpu_is_offline(smp_processor_id())) > + if (cpu_is_offline(cpu_id)) > arch_cpu_idle_dead(); And this hunk fails to apply because the recent cpu hotplug work added another call here. > > local_irq_disable(); I've taken your patch and modified it; see below. Another optimization you could look at is removing that rmb(); I don't actually think its needed, but you'd need to find why it was added and then check it was in fact needed back then, and then check if it is in fact still needed now. Its a bit of a trek through git history, but lfence / dsb ld are expensive instructions. --- Subject: sched,idle: Optimize idle loop From: "Gaurav Jindal (Gaurav Jindal)" <Gaurav.Jindal@spreadtrum.com> Date: Thu, 12 May 2016 10:13:33 +0000 Currently, smp_processor_id() is used to fetch the current CPU in cpu_idle_loop. Every time the idle thread runs, it fetches current CPU using smp_processor_id(). Since the idle thread is per CPU, the current CPU is constant, so we can lift the load out of the loop, saving execution cycles/time in the loop. x86-64: Before patch (execution in loop): 148: 0f ae e8 lfence 14b: 65 8b 04 25 00 00 00 00 mov %gs:0x0,%eax 152: 00 153: 89 c0 mov %eax,%eax 155: 49 0f a3 04 24 bt %rax,(%r12) After patch (execution in loop): 150: 0f ae e8 lfence 153: 4d 0f a3 34 24 bt %r14,(%r12) ARM64: Before patch (execution in loop): 168: d5033d9f dsb ld 16c: b9405661 ldr w1,[x19,#84] 170: 1100fc20 add w0,w1,#0x3f 174: 6b1f003f cmp w1,wzr 178: 1a81b000 csel w0,w0,w1,lt 17c: 130c7000 asr w0,w0,#6 180: 937d7c00 sbfiz x0,x0,#3,#32 184: f8606aa0 ldr x0,[x21,x0] 188: 9ac12401 lsr x1,x0,x1 18c: 36000e61 tbz w1,#0,358 After patch (execution in loop): 1a8: d50339df dsb ld 1ac: f8776ac0 ldr x0,[x22,x23] ab0: ea18001f tst x0,x24 1b4: 54000ea0 b.eq 388 Further observance on ARM64 for 4 seconds shows that cpu_idle_loop is called 8672 times. Shifting the code will save instructions executed in loop and eventually time as well. Cc: "mingo@redhat.com" <mingo@redhat.com> Signed-off-by: Gaurav Jindal <gaurav.jindal@spreadtrum.com> Reviewed-by: Sanjeev Yadav <sanjeev.yadav@spreadtrum.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Link: http://lkml.kernel.org/r/20160512101330.GA488@gauravjindalubtnb.del.spreadtrum.com --- kernel/sched/idle.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) --- a/kernel/sched/idle.c +++ b/kernel/sched/idle.c @@ -201,6 +201,8 @@ static void cpuidle_idle_call(void) */ static void cpu_idle_loop(void) { + int cpu = smp_processor_id(); + while (1) { /* * If the arch has a polling bit, we maintain an invariant: @@ -219,7 +221,7 @@ static void cpu_idle_loop(void) check_pgt_cache(); rmb(); - if (cpu_is_offline(smp_processor_id())) { + if (cpu_is_offline(cpu)) { cpuhp_report_idle_dead(); arch_cpu_idle_dead(); } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop 2016-05-18 11:47 ` Peter Zijlstra @ 2016-05-18 12:30 ` Peter Zijlstra 2016-05-18 12:40 ` Peter Zijlstra 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2016-05-18 12:30 UTC (permalink / raw) To: Gaurav Jindal (Gaurav Jindal) Cc: mingo, linux-kernel, Sanjeev Yadav (Sanjeev Kumar Yadav), Thomas Gleixner On Wed, May 18, 2016 at 01:47:21PM +0200, Peter Zijlstra wrote: > > Another optimization you could look at is removing that rmb(); I don't > actually think its needed, but you'd need to find why it was added and > then check it was in fact needed back then, and then check if it is in > fact still needed now. > > Its a bit of a trek through git history, but lfence / dsb ld are > expensive instructions. > OK; so I was curious ... :-) the rmb() was introduced by the below commit (found in tglx/history.git). which at the time ordered the cpu_idle_state vs pm_idle loads. --- commit f2f1b44c75c478f541d87234c98606d7686c4209 Author: Zwane Mwaikambo <zwane@arm.linux.org.uk> Date: Tue Jan 4 05:35:06 2005 -0800 [PATCH] Remove RCU abuse in cpu_idle() Introduce cpu_idle_wait() on architectures requiring modification of pm_idle from modules, this will ensure that all processors have updated their cached values of pm_idle upon exit. This patch is to address the bug report at http://bugme.osdl.org/show_bug.cgi?id=1716 and replaces the current code fix which is in violation of normal RCU usage as pointed out by Stephen, Dipankar and Paul. Signed-off-by: Zwane Mwaikambo <zwane@arm.linux.org.uk> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org> diff --git a/arch/i386/kernel/apm.c b/arch/i386/kernel/apm.c index 904217f5361e..47712b5d5814 100644 --- a/arch/i386/kernel/apm.c +++ b/arch/i386/kernel/apm.c @@ -2369,7 +2369,7 @@ static void __exit apm_exit(void) * (pm_idle), Wait for all processors to update cached/local * copies of pm_idle before proceeding. */ - synchronize_kernel(); + cpu_idle_wait(); } if (((apm_info.bios.flags & APM_BIOS_DISENGAGED) == 0) && (apm_info.connection_version > 0x0100)) { diff --git a/arch/i386/kernel/process.c b/arch/i386/kernel/process.c index 7efce8e4c376..7b79a9b6a04f 100644 --- a/arch/i386/kernel/process.c +++ b/arch/i386/kernel/process.c @@ -72,6 +72,7 @@ unsigned long thread_saved_pc(struct task_struct *tsk) * Powermanagement idle function, if any.. */ void (*pm_idle)(void); +static cpumask_t cpu_idle_map; void disable_hlt(void) { @@ -93,7 +94,7 @@ EXPORT_SYMBOL(enable_hlt); */ void default_idle(void) { - if (!hlt_counter && current_cpu_data.hlt_works_ok) { + if (!hlt_counter && boot_cpu_data.hlt_works_ok) { local_irq_disable(); if (!need_resched()) safe_halt(); @@ -144,29 +145,44 @@ static void poll_idle (void) */ void cpu_idle (void) { + int cpu = smp_processor_id(); + /* endless idle loop with no priority at all */ while (1) { while (!need_resched()) { void (*idle)(void); - /* - * Mark this as an RCU critical section so that - * synchronize_kernel() in the unload path waits - * for our completion. - */ - rcu_read_lock(); + + if (cpu_isset(cpu, cpu_idle_map)) + cpu_clear(cpu, cpu_idle_map); + rmb(); idle = pm_idle; if (!idle) idle = default_idle; - irq_stat[smp_processor_id()].idle_timestamp = jiffies; + irq_stat[cpu].idle_timestamp = jiffies; idle(); - rcu_read_unlock(); } schedule(); } } +void cpu_idle_wait(void) +{ + int cpu; + cpumask_t map; + + for_each_online_cpu(cpu) + cpu_set(cpu, cpu_idle_map); + + wmb(); + do { + ssleep(1); + cpus_and(map, cpu_idle_map, cpu_online_map); + } while (!cpus_empty(map)); +} +EXPORT_SYMBOL_GPL(cpu_idle_wait); + /* * This uses new MONITOR/MWAIT instructions on P4 processors with PNI, * which can obviate IPI to trigger checking of need_resched. diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c index 078075f7be3f..f96bfb005268 100644 --- a/arch/ia64/kernel/process.c +++ b/arch/ia64/kernel/process.c @@ -25,6 +25,7 @@ #include <linux/unistd.h> #include <linux/efi.h> #include <linux/interrupt.h> +#include <linux/delay.h> #include <asm/cpu.h> #include <asm/delay.h> @@ -46,6 +47,7 @@ #include "sigframe.h" void (*ia64_mark_idle)(int); +static cpumask_t cpu_idle_map; unsigned long boot_option_idle_override = 0; EXPORT_SYMBOL(boot_option_idle_override); @@ -225,10 +227,28 @@ static inline void play_dead(void) } #endif /* CONFIG_HOTPLUG_CPU */ + +void cpu_idle_wait(void) +{ + int cpu; + cpumask_t map; + + for_each_online_cpu(cpu) + cpu_set(cpu, cpu_idle_map); + + wmb(); + do { + ssleep(1); + cpus_and(map, cpu_idle_map, cpu_online_map); + } while (!cpus_empty(map)); +} +EXPORT_SYMBOL_GPL(cpu_idle_wait); + void __attribute__((noreturn)) cpu_idle (void *unused) { void (*mark_idle)(int) = ia64_mark_idle; + int cpu = smp_processor_id(); /* endless idle loop with no priority at all */ while (1) { @@ -241,17 +261,14 @@ cpu_idle (void *unused) if (mark_idle) (*mark_idle)(1); - /* - * Mark this as an RCU critical section so that - * synchronize_kernel() in the unload path waits - * for our completion. - */ - rcu_read_lock(); + + if (cpu_isset(cpu, cpu_idle_map)) + cpu_clear(cpu, cpu_idle_map); + rmb(); idle = pm_idle; if (!idle) idle = default_idle; (*idle)(); - rcu_read_unlock(); } if (mark_idle) diff --git a/arch/x86_64/kernel/process.c b/arch/x86_64/kernel/process.c index 1ce2e1f9a316..129a20d3888c 100644 --- a/arch/x86_64/kernel/process.c +++ b/arch/x86_64/kernel/process.c @@ -61,6 +61,7 @@ EXPORT_SYMBOL(boot_option_idle_override); * Powermanagement idle function, if any.. */ void (*pm_idle)(void); +static cpumask_t cpu_idle_map; void disable_hlt(void) { @@ -123,6 +124,23 @@ static void poll_idle (void) } } + +void cpu_idle_wait(void) +{ + int cpu; + cpumask_t map; + + for_each_online_cpu(cpu) + cpu_set(cpu, cpu_idle_map); + + wmb(); + do { + ssleep(1); + cpus_and(map, cpu_idle_map, cpu_online_map); + } while (!cpus_empty(map)); +} +EXPORT_SYMBOL_GPL(cpu_idle_wait); + /* * The idle thread. There's no useful work to be * done, so just try to conserve power and have a @@ -131,21 +149,20 @@ static void poll_idle (void) */ void cpu_idle (void) { + int cpu = smp_processor_id(); + /* endless idle loop with no priority at all */ while (1) { while (!need_resched()) { void (*idle)(void); - /* - * Mark this as an RCU critical section so that - * synchronize_kernel() in the unload path waits - * for our completion. - */ - rcu_read_lock(); + + if (cpu_isset(cpu, cpu_idle_map)) + cpu_clear(cpu, cpu_idle_map); + rmb(); idle = pm_idle; if (!idle) idle = default_idle; idle(); - rcu_read_unlock(); } schedule(); } diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index d9e4433177d3..47eb7463c009 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -988,7 +988,7 @@ int acpi_processor_power_exit(struct acpi_processor *pr, struct acpi_device *dev * (pm_idle), Wait for all processors to update cached/local * copies of pm_idle before proceeding. */ - synchronize_kernel(); + cpu_idle_wait(); } return_VALUE(0); diff --git a/include/asm-i386/system.h b/include/asm-i386/system.h index 9bb0f63ff64e..c705fa77b138 100644 --- a/include/asm-i386/system.h +++ b/include/asm-i386/system.h @@ -466,5 +466,6 @@ void disable_hlt(void); void enable_hlt(void); extern int es7000_plat; +void cpu_idle_wait(void); #endif diff --git a/include/asm-ia64/system.h b/include/asm-ia64/system.h index fdefa16fabc4..f28b920e9089 100644 --- a/include/asm-ia64/system.h +++ b/include/asm-ia64/system.h @@ -284,6 +284,7 @@ do { \ #define ia64_platform_is(x) (strcmp(x, platform_name) == 0) +void cpu_idle_wait(void); #endif /* __KERNEL__ */ #endif /* __ASSEMBLY__ */ diff --git a/include/asm-x86_64/system.h b/include/asm-x86_64/system.h index 55ac71fd4a60..05acc62c2b61 100644 --- a/include/asm-x86_64/system.h +++ b/include/asm-x86_64/system.h @@ -326,6 +326,8 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old, /* For spinlocks etc */ #define local_irq_save(x) do { warn_if_not_ulong(x); __asm__ __volatile__("# local_irq_save \n\t pushfq ; popq %0 ; cli":"=g" (x): /* no input */ :"memory"); } while (0) +void cpu_idle_wait(void); + /* * disable hlt during certain critical i/o operations */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop 2016-05-18 12:30 ` Peter Zijlstra @ 2016-05-18 12:40 ` Peter Zijlstra 2016-05-19 13:32 ` Gaurav Jindal (Gaurav Jindal) 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2016-05-18 12:40 UTC (permalink / raw) To: Gaurav Jindal (Gaurav Jindal) Cc: mingo, linux-kernel, Sanjeev Yadav (Sanjeev Kumar Yadav), Thomas Gleixner On Wed, May 18, 2016 at 02:30:44PM +0200, Peter Zijlstra wrote: > void cpu_idle (void) > { > + int cpu = smp_processor_id(); > + > /* endless idle loop with no priority at all */ > while (1) { > while (!need_resched()) { > void (*idle)(void); > - /* > - * Mark this as an RCU critical section so that > - * synchronize_kernel() in the unload path waits > - * for our completion. > - */ > - rcu_read_lock(); > + > + if (cpu_isset(cpu, cpu_idle_map)) > + cpu_clear(cpu, cpu_idle_map); > + rmb(); > idle = pm_idle; > > if (!idle) > idle = default_idle; > > - irq_stat[smp_processor_id()].idle_timestamp = jiffies; > + irq_stat[cpu].idle_timestamp = jiffies; > idle(); > - rcu_read_unlock(); > } > schedule(); > } > } > > +void cpu_idle_wait(void) > +{ > + int cpu; > + cpumask_t map; > + > + for_each_online_cpu(cpu) > + cpu_set(cpu, cpu_idle_map); > + > + wmb(); > + do { > + ssleep(1); > + cpus_and(map, cpu_idle_map, cpu_online_map); > + } while (!cpus_empty(map)); > +} > +EXPORT_SYMBOL_GPL(cpu_idle_wait); Which then got 'wrecked' by the below commit. That commit removes the cpu_idle_state, and thereby removes the need for the rmb(), since you cannot 'order' one load. All the idle loop needs to guarantee (and in today's code that's non-obvious) is that it _must_ reload all values on every loop. --- commit 783e391b7b5b273cd20856d8f6f4878da8ec31b3 Author: Venki Pallipadi <venkatesh.pallipadi@intel.com> Date: Thu Apr 10 09:49:58 2008 -0700 x86: Simplify cpu_idle_wait This patch also resolves hangs on boot: http://lkml.org/lkml/2008/2/23/263 http://bugzilla.kernel.org/show_bug.cgi?id=10093 The bug was causing once-in-few-reboots 10-15 sec wait during boot on certain laptops. Earlier commit 40d6a146629b98d8e322b6f9332b182c7cbff3df added smp_call_function in cpu_idle_wait() to kick cpus that are in tickless idle. Looking at cpu_idle_wait code at that time, code seemed to be over-engineered for a case which is rarely used (while changing idle handler). Below is a simplified version of cpu_idle_wait, which just makes a dummy smp_call_function to all cpus, to make them come out of old idle handler and start using the new idle handler. It eliminates code in the idle loop to handle cpu_idle_wait. Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c index be3c7a299f02..43930e73f657 100644 --- a/arch/x86/kernel/process_32.c +++ b/arch/x86/kernel/process_32.c @@ -82,7 +82,6 @@ unsigned long thread_saved_pc(struct task_struct *tsk) */ void (*pm_idle)(void); EXPORT_SYMBOL(pm_idle); -static DEFINE_PER_CPU(unsigned int, cpu_idle_state); void disable_hlt(void) { @@ -190,9 +189,6 @@ void cpu_idle(void) while (!need_resched()) { void (*idle)(void); - if (__get_cpu_var(cpu_idle_state)) - __get_cpu_var(cpu_idle_state) = 0; - check_pgt_cache(); rmb(); idle = pm_idle; @@ -220,40 +216,19 @@ static void do_nothing(void *unused) { } +/* + * cpu_idle_wait - Used to ensure that all the CPUs discard old value of + * pm_idle and update to new pm_idle value. Required while changing pm_idle + * handler on SMP systems. + * + * Caller must have changed pm_idle to the new value before the call. Old + * pm_idle value will not be used by any CPU after the return of this function. + */ void cpu_idle_wait(void) { - unsigned int cpu, this_cpu = get_cpu(); - cpumask_t map, tmp = current->cpus_allowed; - - set_cpus_allowed(current, cpumask_of_cpu(this_cpu)); - put_cpu(); - - cpus_clear(map); - for_each_online_cpu(cpu) { - per_cpu(cpu_idle_state, cpu) = 1; - cpu_set(cpu, map); - } - - __get_cpu_var(cpu_idle_state) = 0; - - wmb(); - do { - ssleep(1); - for_each_online_cpu(cpu) { - if (cpu_isset(cpu, map) && !per_cpu(cpu_idle_state, cpu)) - cpu_clear(cpu, map); - } - cpus_and(map, map, cpu_online_map); - /* - * We waited 1 sec, if a CPU still did not call idle - * it may be because it is in idle and not waking up - * because it has nothing to do. - * Give all the remaining CPUS a kick. - */ - smp_call_function_mask(map, do_nothing, NULL, 0); - } while (!cpus_empty(map)); - - set_cpus_allowed(current, tmp); + smp_mb(); + /* kick all the CPUs so that they exit out of pm_idle */ + smp_call_function(do_nothing, NULL, 0, 1); } EXPORT_SYMBOL_GPL(cpu_idle_wait); diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c index 3baf9b9f4c87..46c4c546b499 100644 --- a/arch/x86/kernel/process_64.c +++ b/arch/x86/kernel/process_64.c @@ -63,7 +63,6 @@ EXPORT_SYMBOL(boot_option_idle_override); */ void (*pm_idle)(void); EXPORT_SYMBOL(pm_idle); -static DEFINE_PER_CPU(unsigned int, cpu_idle_state); static ATOMIC_NOTIFIER_HEAD(idle_notifier); @@ -173,9 +172,6 @@ void cpu_idle(void) while (!need_resched()) { void (*idle)(void); - if (__get_cpu_var(cpu_idle_state)) - __get_cpu_var(cpu_idle_state) = 0; - rmb(); idle = pm_idle; if (!idle) @@ -207,40 +203,19 @@ static void do_nothing(void *unused) { } +/* + * cpu_idle_wait - Used to ensure that all the CPUs discard old value of + * pm_idle and update to new pm_idle value. Required while changing pm_idle + * handler on SMP systems. + * + * Caller must have changed pm_idle to the new value before the call. Old + * pm_idle value will not be used by any CPU after the return of this function. + */ void cpu_idle_wait(void) { - unsigned int cpu, this_cpu = get_cpu(); - cpumask_t map, tmp = current->cpus_allowed; - - set_cpus_allowed(current, cpumask_of_cpu(this_cpu)); - put_cpu(); - - cpus_clear(map); - for_each_online_cpu(cpu) { - per_cpu(cpu_idle_state, cpu) = 1; - cpu_set(cpu, map); - } - - __get_cpu_var(cpu_idle_state) = 0; - - wmb(); - do { - ssleep(1); - for_each_online_cpu(cpu) { - if (cpu_isset(cpu, map) && !per_cpu(cpu_idle_state, cpu)) - cpu_clear(cpu, map); - } - cpus_and(map, map, cpu_online_map); - /* - * We waited 1 sec, if a CPU still did not call idle - * it may be because it is in idle and not waking up - * because it has nothing to do. - * Give all the remaining CPUS a kick. - */ - smp_call_function_mask(map, do_nothing, 0, 0); - } while (!cpus_empty(map)); - - set_cpus_allowed(current, tmp); + smp_mb(); + /* kick all the CPUs so that they exit out of pm_idle */ + smp_call_function(do_nothing, NULL, 0, 1); } EXPORT_SYMBOL_GPL(cpu_idle_wait); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop 2016-05-18 12:40 ` Peter Zijlstra @ 2016-05-19 13:32 ` Gaurav Jindal (Gaurav Jindal) 0 siblings, 0 replies; 6+ messages in thread From: Gaurav Jindal (Gaurav Jindal) @ 2016-05-19 13:32 UTC (permalink / raw) To: Peter Zijlstra Cc: mingo, linux-kernel, Sanjeev Yadav (Sanjeev Kumar Yadav), Thomas Gleixner Hi Peterz Thanks a lot for the overwhelming response :) I will look into the change history to have a pervasive understanding of the framework. On Wed, May 18, 2016 at 02:40:52PM +0200, Peter Zijlstra wrote: > On Wed, May 18, 2016 at 02:30:44PM +0200, Peter Zijlstra wrote: > > void cpu_idle (void) > > { > > + int cpu = smp_processor_id(); > > + > > /* endless idle loop with no priority at all */ > > while (1) { > > while (!need_resched()) { > > void (*idle)(void); > > - /* > > - * Mark this as an RCU critical section so that > > - * synchronize_kernel() in the unload path waits > > - * for our completion. > > - */ > > - rcu_read_lock(); > > + > > + if (cpu_isset(cpu, cpu_idle_map)) > > + cpu_clear(cpu, cpu_idle_map); > > + rmb(); > > idle = pm_idle; > > > > if (!idle) > > idle = default_idle; > > > > - irq_stat[smp_processor_id()].idle_timestamp = jiffies; > > + irq_stat[cpu].idle_timestamp = jiffies; > > idle(); > > - rcu_read_unlock(); > > } > > schedule(); > > } > > } > > > > +void cpu_idle_wait(void) > > +{ > > + int cpu; > > + cpumask_t map; > > + > > + for_each_online_cpu(cpu) > > + cpu_set(cpu, cpu_idle_map); > > + > > + wmb(); > > + do { > > + ssleep(1); > > + cpus_and(map, cpu_idle_map, cpu_online_map); > > + } while (!cpus_empty(map)); > > +} > > +EXPORT_SYMBOL_GPL(cpu_idle_wait); > > > Which then got 'wrecked' by the below commit. > > That commit removes the cpu_idle_state, and thereby removes the need for > the rmb(), since you cannot 'order' one load. > > All the idle loop needs to guarantee (and in today's code that's > non-obvious) is that it _must_ reload all values on every loop. > > > --- > commit 783e391b7b5b273cd20856d8f6f4878da8ec31b3 > Author: Venki Pallipadi <venkatesh.pallipadi@intel.com> > Date: Thu Apr 10 09:49:58 2008 -0700 > > x86: Simplify cpu_idle_wait > > This patch also resolves hangs on boot: > http://lkml.org/lkml/2008/2/23/263 > http://bugzilla.kernel.org/show_bug.cgi?id=10093 > > The bug was causing once-in-few-reboots 10-15 sec wait during boot on > certain laptops. > > Earlier commit 40d6a146629b98d8e322b6f9332b182c7cbff3df added > smp_call_function in cpu_idle_wait() to kick cpus that are in tickless > idle. Looking at cpu_idle_wait code at that time, code seemed to be > over-engineered for a case which is rarely used (while changing idle > handler). > > Below is a simplified version of cpu_idle_wait, which just makes a dummy > smp_call_function to all cpus, to make them come out of old idle handler > and start using the new idle handler. It eliminates code in the idle > loop to handle cpu_idle_wait. > > Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c > index be3c7a299f02..43930e73f657 100644 > --- a/arch/x86/kernel/process_32.c > +++ b/arch/x86/kernel/process_32.c > @@ -82,7 +82,6 @@ unsigned long thread_saved_pc(struct task_struct *tsk) > */ > void (*pm_idle)(void); > EXPORT_SYMBOL(pm_idle); > -static DEFINE_PER_CPU(unsigned int, cpu_idle_state); > > void disable_hlt(void) > { > @@ -190,9 +189,6 @@ void cpu_idle(void) > while (!need_resched()) { > void (*idle)(void); > > - if (__get_cpu_var(cpu_idle_state)) > - __get_cpu_var(cpu_idle_state) = 0; > - > check_pgt_cache(); > rmb(); > idle = pm_idle; > @@ -220,40 +216,19 @@ static void do_nothing(void *unused) > { > } > > +/* > + * cpu_idle_wait - Used to ensure that all the CPUs discard old value of > + * pm_idle and update to new pm_idle value. Required while changing pm_idle > + * handler on SMP systems. > + * > + * Caller must have changed pm_idle to the new value before the call. Old > + * pm_idle value will not be used by any CPU after the return of this function. > + */ > void cpu_idle_wait(void) > { > - unsigned int cpu, this_cpu = get_cpu(); > - cpumask_t map, tmp = current->cpus_allowed; > - > - set_cpus_allowed(current, cpumask_of_cpu(this_cpu)); > - put_cpu(); > - > - cpus_clear(map); > - for_each_online_cpu(cpu) { > - per_cpu(cpu_idle_state, cpu) = 1; > - cpu_set(cpu, map); > - } > - > - __get_cpu_var(cpu_idle_state) = 0; > - > - wmb(); > - do { > - ssleep(1); > - for_each_online_cpu(cpu) { > - if (cpu_isset(cpu, map) && !per_cpu(cpu_idle_state, cpu)) > - cpu_clear(cpu, map); > - } > - cpus_and(map, map, cpu_online_map); > - /* > - * We waited 1 sec, if a CPU still did not call idle > - * it may be because it is in idle and not waking up > - * because it has nothing to do. > - * Give all the remaining CPUS a kick. > - */ > - smp_call_function_mask(map, do_nothing, NULL, 0); > - } while (!cpus_empty(map)); > - > - set_cpus_allowed(current, tmp); > + smp_mb(); > + /* kick all the CPUs so that they exit out of pm_idle */ > + smp_call_function(do_nothing, NULL, 0, 1); > } > EXPORT_SYMBOL_GPL(cpu_idle_wait); > > diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c > index 3baf9b9f4c87..46c4c546b499 100644 > --- a/arch/x86/kernel/process_64.c > +++ b/arch/x86/kernel/process_64.c > @@ -63,7 +63,6 @@ EXPORT_SYMBOL(boot_option_idle_override); > */ > void (*pm_idle)(void); > EXPORT_SYMBOL(pm_idle); > -static DEFINE_PER_CPU(unsigned int, cpu_idle_state); > > static ATOMIC_NOTIFIER_HEAD(idle_notifier); > > @@ -173,9 +172,6 @@ void cpu_idle(void) > while (!need_resched()) { > void (*idle)(void); > > - if (__get_cpu_var(cpu_idle_state)) > - __get_cpu_var(cpu_idle_state) = 0; > - > rmb(); > idle = pm_idle; > if (!idle) > @@ -207,40 +203,19 @@ static void do_nothing(void *unused) > { > } > > +/* > + * cpu_idle_wait - Used to ensure that all the CPUs discard old value of > + * pm_idle and update to new pm_idle value. Required while changing pm_idle > + * handler on SMP systems. > + * > + * Caller must have changed pm_idle to the new value before the call. Old > + * pm_idle value will not be used by any CPU after the return of this function. > + */ > void cpu_idle_wait(void) > { > - unsigned int cpu, this_cpu = get_cpu(); > - cpumask_t map, tmp = current->cpus_allowed; > - > - set_cpus_allowed(current, cpumask_of_cpu(this_cpu)); > - put_cpu(); > - > - cpus_clear(map); > - for_each_online_cpu(cpu) { > - per_cpu(cpu_idle_state, cpu) = 1; > - cpu_set(cpu, map); > - } > - > - __get_cpu_var(cpu_idle_state) = 0; > - > - wmb(); > - do { > - ssleep(1); > - for_each_online_cpu(cpu) { > - if (cpu_isset(cpu, map) && !per_cpu(cpu_idle_state, cpu)) > - cpu_clear(cpu, map); > - } > - cpus_and(map, map, cpu_online_map); > - /* > - * We waited 1 sec, if a CPU still did not call idle > - * it may be because it is in idle and not waking up > - * because it has nothing to do. > - * Give all the remaining CPUS a kick. > - */ > - smp_call_function_mask(map, do_nothing, 0, 0); > - } while (!cpus_empty(map)); > - > - set_cpus_allowed(current, tmp); > + smp_mb(); > + /* kick all the CPUs so that they exit out of pm_idle */ > + smp_call_function(do_nothing, NULL, 0, 1); > } > EXPORT_SYMBOL_GPL(cpu_idle_wait); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:sched/core] sched/idle: Optimize the generic idle loop 2016-05-12 10:13 [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop Gaurav Jindal (Gaurav Jindal) 2016-05-18 11:47 ` Peter Zijlstra @ 2016-06-03 10:48 ` tip-bot for Gaurav Jindal (Gaurav Jindal) 1 sibling, 0 replies; 6+ messages in thread From: tip-bot for Gaurav Jindal (Gaurav Jindal) @ 2016-06-03 10:48 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, hpa, peterz, gaurav.jindal, sanjeev.yadav, torvalds, efault, tglx, Gaurav.Jindal, mingo Commit-ID: df55f462b905f3b2d40ec3fb865891382a6ebfb1 Gitweb: http://git.kernel.org/tip/df55f462b905f3b2d40ec3fb865891382a6ebfb1 Author: Gaurav Jindal (Gaurav Jindal) <Gaurav.Jindal@spreadtrum.com> AuthorDate: Thu, 12 May 2016 10:13:33 +0000 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Fri, 3 Jun 2016 09:18:56 +0200 sched/idle: Optimize the generic idle loop Currently, smp_processor_id() is used to fetch the current CPU in cpu_idle_loop(). Every time the idle thread runs, it fetches the current CPU using smp_processor_id(). Since the idle thread is per CPU, the current CPU is constant, so we can lift the load out of the loop, saving execution cycles/time in the loop. x86-64: Before patch (execution in loop): 148: 0f ae e8 lfence 14b: 65 8b 04 25 00 00 00 00 mov %gs:0x0,%eax 152: 00 153: 89 c0 mov %eax,%eax 155: 49 0f a3 04 24 bt %rax,(%r12) After patch (execution in loop): 150: 0f ae e8 lfence 153: 4d 0f a3 34 24 bt %r14,(%r12) ARM64: Before patch (execution in loop): 168: d5033d9f dsb ld 16c: b9405661 ldr w1,[x19,#84] 170: 1100fc20 add w0,w1,#0x3f 174: 6b1f003f cmp w1,wzr 178: 1a81b000 csel w0,w0,w1,lt 17c: 130c7000 asr w0,w0,#6 180: 937d7c00 sbfiz x0,x0,#3,#32 184: f8606aa0 ldr x0,[x21,x0] 188: 9ac12401 lsr x1,x0,x1 18c: 36000e61 tbz w1,#0,358 After patch (execution in loop): 1a8: d50339df dsb ld 1ac: f8776ac0 ldr x0,[x22,x23] ab0: ea18001f tst x0,x24 1b4: 54000ea0 b.eq 388 Further observance on ARM64 for 4 seconds shows that cpu_idle_loop is called 8672 times. Shifting the code will save instructions executed in loop and eventually time as well. Signed-off-by: Gaurav Jindal <gaurav.jindal@spreadtrum.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reviewed-by: Sanjeev Yadav <sanjeev.yadav@spreadtrum.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Mike Galbraith <efault@gmx.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/20160512101330.GA488@gauravjindalubtnb.del.spreadtrum.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- kernel/sched/idle.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c index bd12c6c..db4ff7c 100644 --- a/kernel/sched/idle.c +++ b/kernel/sched/idle.c @@ -201,6 +201,8 @@ exit_idle: */ static void cpu_idle_loop(void) { + int cpu = smp_processor_id(); + while (1) { /* * If the arch has a polling bit, we maintain an invariant: @@ -219,7 +221,7 @@ static void cpu_idle_loop(void) check_pgt_cache(); rmb(); - if (cpu_is_offline(smp_processor_id())) { + if (cpu_is_offline(cpu)) { cpuhp_report_idle_dead(); arch_cpu_idle_dead(); } ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-03 10:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-12 10:13 [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop Gaurav Jindal (Gaurav Jindal) 2016-05-18 11:47 ` Peter Zijlstra 2016-05-18 12:30 ` Peter Zijlstra 2016-05-18 12:40 ` Peter Zijlstra 2016-05-19 13:32 ` Gaurav Jindal (Gaurav Jindal) 2016-06-03 10:48 ` [tip:sched/core] sched/idle: Optimize the generic idle loop tip-bot for Gaurav Jindal (Gaurav Jindal)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome