[Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop

[Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop

[Gaurav Jindal (Gaurav Jindal)]

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();

--

Re: [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop

[Peter Zijlstra]

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();
 			}

Re: [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop

[Peter Zijlstra]

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
  */

Re: [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop

[Peter Zijlstra]

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);
 

Re: [Patch]cpuidle: Save current cpu as local variable instead of calling smp_processor_id() in loop

[Gaurav Jindal (Gaurav Jindal)]

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);
>  

[tip:sched/core] sched/idle: Optimize the generic idle loop

[tip-bot for Gaurav Jindal (Gaurav Jindal)]

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();
 			}