kick sleeping idle CPUS on cpu_idle_wait

RFC PATCH In review Steven Rostedt 7 participants 17 messages Jan 8, 2008 – Apr 10, 2008 1 message not in the archive x86

lore

Share & tools

lore

Steven Rostedt

author

2008-01-08 02:28 UTC #0 lore raw

RFC PATCH 1/1 kick sleeping idle CPUS on cpu_idle_wait

Sometimes cpu_idle_wait gets stuck because it might miss CPUS that are already in idle, have no tasks waiting to run and have no interrupts going to them. This is common on bootup when switching cpu idle governors.

This patch accounts for CPUs already in idle.

Background:
-----------
I notice this while developing the mcount patches, that every once in a while the system would hang. Looking deeper, the hang was always at boot up when registering init_menu of the cpu_idle menu governor. Talking with Thomas Gliexner, we discovered that one of the CPUS had no timer events scheduled for it and it was in idle (running with NO_HZ). So the CPU would not set the cpu_idle_state bit.

Hitting sysrq-t a few times would eventually route the interrupt to the stuck CPU and the system would continue.

Note, I would have used the PDA isidle but that is set after the cpu_idle_state bit is cleared, and would leave a window open where we may miss being kicked.

hmm, looking closer at this, we still have a small race window between clearing the cpu_idle_state and disabling interrupts (hence the RFC).

CPU0: CPU 1:

  ---------                       ---------
 cpu_idle_wait():                 cpu_idle():
      |                           __cpu_cpu_var(is_idle) = 1;
      |                           if (__get_cpu_var(cpu_idle_state)) /* == 0 */
 per_cpu(cpu_idle_state, 1) = 1;         |
 if (per_cpu(is_idle, 1)) /* == 1 */     |
 smp_call_function(1)                    |

| receives ipi and runs do_nothing.
wait on map == empty idle();
/* waits forever */

So really we need interrupts off for most of this then. One might think that we could simply clear the cpu_idle_state from do_nothing, but I'm assuming that cpu_idle governors can be removed, and this might cause a race that a governor might be used after the module was removed.

One solution I think is that we could keep calling do_nothing to all the bits of map that are still set in the cpu_idle_wait() loop. That would work too.

Thoughts?

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/kernel/process_32.c |   36 ++++++++++++++++++++++++++++++++----
 arch/x86/kernel/process_64.c |   28 ++++++++++++++++++++++++----
 2 files changed, 56 insertions(+), 8 deletions(-)

arch/x86/kernel/process_32.c +32 -4

Index: linus.git/arch/x86/kernel/process_32.c
===================================================================
@@ -83,6 +83,7 @@ unsigned long thread_saved_pc(struct tas
 void (*pm_idle)(void);
 EXPORT_SYMBOL(pm_idle);
 static DEFINE_PER_CPU(unsigned int, cpu_idle_state);
+static DEFINE_PER_CPU(unsigned int, in_idle);
 
 void disable_hlt(void)
 {

@@ -178,6 +179,12 @@ void cpu_idle(void)
 	/* endless idle loop with no priority at all */
 	while (1) {
 		tick_nohz_stop_sched_tick();
+		__get_cpu_var(in_idle) = 1;
+		/*
+		 * Make sure in_idle is seen by others
+		 * before cpu_idle_state is set (and read)
+		 */
+		smp_mb();
 		while (!need_resched()) {
 			void (*idle)(void);

@@ -197,6 +204,7 @@ void cpu_idle(void)
 			__get_cpu_var(irq_stat).idle_timestamp = jiffies;
 			idle();
 		}
+		__get_cpu_var(in_idle) = 0;
 		tick_nohz_restart_sched_tick();
 		preempt_enable_no_resched();
 		schedule();

@@ -204,6 +212,10 @@ void cpu_idle(void)
 	}
 }
 
+static void do_nothing(void *unused)
+{
+}
+
 void cpu_idle_wait(void)
 {
 	unsigned int cpu, this_cpu = get_cpu();

@@ -212,15 +224,31 @@ void cpu_idle_wait(void)
 	set_cpus_allowed(current, cpumask_of_cpu(this_cpu));
 	put_cpu();
 
+	for_each_online_cpu(cpu)
+		per_cpu(cpu_idle_state, cpu) = 1;
+
+	__get_cpu_var(cpu_idle_state) = 0;
+
+	/*
+	 * Make cpu_idle_state viewable by other CPUs
+	 * and be able to read in_idle of other CPUs.
+	 */
+	smp_mb();
+
+	/* Wake up any CPUs that are already in idle */
 	cpus_clear(map);
 	for_each_online_cpu(cpu) {
-		per_cpu(cpu_idle_state, cpu) = 1;
-		cpu_set(cpu, map);
+		if (per_cpu(in_idle, cpu))
+			cpu_set(cpu, map);
 	}
+	smp_call_function_mask(map, do_nothing, 0, 0);
 
-	__get_cpu_var(cpu_idle_state) = 0;
+	cpus_clear(map);
+	for_each_online_cpu(cpu) {
+		if (per_cpu(cpu_idle_state, cpu))
+			cpu_set(cpu, map);
+	}
 
-	wmb();
 	do {
 		ssleep(1);
 		for_each_online_cpu(cpu) {

arch/x86/kernel/process_64.c +24 -4

Index: linus.git/arch/x86/kernel/process_64.c
===================================================================
@@ -65,6 +65,7 @@ EXPORT_SYMBOL(boot_option_idle_override)
 void (*pm_idle)(void);
 EXPORT_SYMBOL(pm_idle);
 static DEFINE_PER_CPU(unsigned int, cpu_idle_state);
+static DEFINE_PER_CPU(unsigned int, in_idle);
 
 static ATOMIC_NOTIFIER_HEAD(idle_notifier);

@@ -135,6 +136,10 @@ static void poll_idle (void)
 	cpu_relax();
 }
 
+static void do_nothing(void *unused)
+{
+}
+
 void cpu_idle_wait(void)
 {
 	unsigned int cpu, this_cpu = get_cpu();

@@ -143,15 +148,30 @@ void cpu_idle_wait(void)
 	set_cpus_allowed(current, cpumask_of_cpu(this_cpu));
 	put_cpu();
 
+	for_each_online_cpu(cpu)
+		per_cpu(cpu_idle_state, cpu) = 1;
+	__get_cpu_var(cpu_idle_state) = 0;
+
+	/*
+	 * Make cpu_idle_state viewable by other CPUs
+	 * and be able to read in_idle of other CPUs.
+	 */
+	smp_mb();
+
+	/* Wake up any CPUs that are already in idle */
 	cpus_clear(map);
 	for_each_online_cpu(cpu) {
-		per_cpu(cpu_idle_state, cpu) = 1;
-		cpu_set(cpu, map);
+		if (per_cpu(in_idle, cpu))
+			cpu_set(cpu, map);
 	}
+	smp_call_function_mask(map, do_nothing, 0, 0);
 
-	__get_cpu_var(cpu_idle_state) = 0;
+	cpus_clear(map);
+	for_each_online_cpu(cpu) {
+		if (per_cpu(cpu_idle_state, cpu))
+			cpu_set(cpu, map);
+	}
 
-	wmb();
 	do {
 		ssleep(1);
 		for_each_online_cpu(cpu) {

Andi Kleen

2008-01-08 03:33 UTC #1 lore raw

Quoted 5 lines · Steven Rostedt

Sometimes cpu_idle_wait gets stuck because it might miss CPUS that are already in idle, have no tasks waiting to run and have no interrupts going to them. This is common on bootup when switching cpu idle governors.

I must admit I never liked that cpu idle wait code anyways. Why again can't normal RCU be used for this? Waiting for two RCU quiescent cycles should be enough, shouldn't it?

-Andi

Steven Rostedt

author

2008-01-09 20:43 UTC #2 lore raw

PATCH 1/1 Kick CPUS that might be sleeping in cpus_idle_wait

This patch is different than the first patch I sent out. This one just sends an IPI to all CPUS that don't check in after 1 sec.

This patch gives those CPUS that don't check in an IPI kick.

Signed-off-by Steven Rostedt <[email protected]>

---
 arch/x86/kernel/process_32.c |   11 +++++++++++
 arch/x86/kernel/process_64.c |   11 +++++++++++
 2 files changed, 22 insertions(+)

arch/x86/kernel/process_32.c +11 -0

Index: linux-compile-i386.git/arch/x86/kernel/process_32.c
===================================================================
@@ -204,6 +204,10 @@ void cpu_idle(void)
 	}
 }
 
+static void do_nothing(void *unused)
+{
+}
+
 void cpu_idle_wait(void)
 {
 	unsigned int cpu, this_cpu = get_cpu();

@@ -228,6 +232,13 @@ void cpu_idle_wait(void)
 				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);

arch/x86/kernel/process_64.c +11 -0

Index: linux-compile-i386.git/arch/x86/kernel/process_64.c
===================================================================
@@ -135,6 +135,10 @@ static void poll_idle (void)
 	cpu_relax();
 }
 
+static void do_nothing(void *unused)
+{
+}
+
 void cpu_idle_wait(void)
 {
 	unsigned int cpu, this_cpu = get_cpu();

@@ -160,6 +164,13 @@ void cpu_idle_wait(void)
 				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);

Steven Rostedt

author

2008-01-09 22:13 UTC #3 lore raw

Re: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait

Quoted 10 lines · Steven Rostedt

This patch is different than the first patch I sent out. This one just sends an IPI to all CPUS that don't check in after 1 sec.
Sometimes cpu_idle_wait gets stuck because it might miss CPUS that are already in idle, have no tasks waiting to run and have no interrupts going to them. This is common on bootup when switching cpu idle governors.
This patch gives those CPUS that don't check in an IPI kick.

FYI, I just hit this hang on 2.6.24-rc6 without any extra patches. So, unless 2.6.24-rc7 did anything to fix this issue, this is a high priority bug (IMHO).

-- Steve

Ingo Molnar

2008-01-10 13:46 UTC #4 lore raw

Re: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait

Quoted 10 lines · Steven Rostedt

Quoted 9 lines · Steven Rostedt
This patch is different than the first patch I sent out. This one just sends an IPI to all CPUS that don't check in after 1 sec.
Sometimes cpu_idle_wait gets stuck because it might miss CPUS that are already in idle, have no tasks waiting to run and have no interrupts going to them. This is common on bootup when switching cpu idle governors.

Quoted 5 lines

Quoted 1 line
This patch gives those CPUS that don't check in an IPI kick.
FYI, I just hit this hang on 2.6.24-rc6 without any extra patches. So, unless 2.6.24-rc7 did anything to fix this issue, this is a high priority bug (IMHO).

i'm wondering why this only triggered now. Is this something new in 2.6.24?

Ingo

Steven Rostedt

author

2008-01-10 14:44 UTC #5 lore raw

Re: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait

Quoted 6 lines · Ingo Molnar

Quoted 3 lines
FYI, I just hit this hang on 2.6.24-rc6 without any extra patches. So, unless 2.6.24-rc7 did anything to fix this issue, this is a high priority bug (IMHO).
i'm wondering why this only triggered now. Is this something new in 2.6.24?

It only triggeres with the switching of the idle governors. And not just one, you need to switch twice. The first loading of a governor does not call cpu_idle_wait, but the second one does. NO_HZ must also be enabled, plus this needs to happen when no events or threads are scheduled to run on a CPU, which limits this to boot up.

Also, this only seems to happen on my 2x2 (4way) and only once in a while.

I'm surprised that I'm the only one so far to report it. I can boot up the 2.6.23 kernel on this box to see if it also hangs sometimes. But, as I said, it may take several hundreds of tries to see it.

-- Steve

Pallipadi, Venkatesh

2008-01-10 17:52 UTC #6 lore raw

RE: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait

Quoted 32 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Steven Rostedt
Sent: Thursday, January 10, 2008 6:44 AM
To: Ingo Molnar
Cc: LKML; Linus Torvalds; Andrew Morton; Thomas Gleixner; Brown, Len; Pallipadi, Venkatesh; Adam Belay; Peter Zijlstra; Andi Kleen
Subject: Re: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait
Quoted 1 line · Ingo Molnar
Quoted 1 line
FYI, I just hit this hang on 2.6.24-rc6 without any extra
patches. So,
Quoted 5 lines
Quoted 2 lines
unless 2.6.24-rc7 did anything to fix this issue, this is a high priority bug (IMHO).
i'm wondering why this only triggered now. Is this something new in 2.6.24?
It only triggeres with the switching of the idle governors. And not just
one, you need to switch twice. The first loading of a governor does not call cpu_idle_wait, but the second one does. NO_HZ must also be enabled,
plus this needs to happen when no events or threads are scheduled to run
on a CPU, which limits this to boot up.

Quoted 7 lines

Also, this only seems to happen on my 2x2 (4way) and only once in a while.
I'm surprised that I'm the only one so far to report it. I can boot up the
2.6.23 kernel on this box to see if it also hangs sometimes. But, as I said, it may take several hundreds of tries to see it.

With 2.6.23, you can try compiling acpi processor.ko as a module and doing insmod and rmmod in a loop. That should call cpu_idle_wait very frequently.

Thanks,
Venki

Steven Rostedt

author

2008-01-10 18:05 UTC #7 lore raw

RE: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait

Quoted 5 lines · Venkatesh

With 2.6.23, you can try compiling acpi processor.ko as a module and doing insmod and rmmod in a loop. That should call cpu_idle_wait very frequently.

The thing is, after the full boot, there's too many things going on to keep a system fully idle. Little services can wake a processor up. The problem I had was that this happens on early bootup where there's not much around to wake the sleeping processor.

I'm not sure if it is too much of a problem after boot up.

-- Steve

Andrew Morton

2008-01-09 23:44 UTC #8 lore raw

Re: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait

In reply to Steven Rostedt

Quoted 2 lines

Subject: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait

s/cpus_/cpu_/

Quoted 11 lines · On Wed, 09 Jan 2008 15:42:10 -0500 Steven Rostedt <[email protected]> wrote:

This patch is different than the first patch I sent out. This one just sends an IPI to all CPUS that don't check in after 1 sec.
Sometimes cpu_idle_wait gets stuck because it might miss CPUS that are already in idle, have no tasks waiting to run and have no interrupts going to them. This is common on bootup when switching cpu idle governors.
This patch gives those CPUS that don't check in an IPI kick.

(wakes up)

Quoted 20 lines

Index: linux-compile-i386.git/arch/x86/kernel/process_32.c
===================================================================

arch/x86/kernel/process_32.c +4 -0

@@ -204,6 +204,10 @@ void cpu_idle(void)
 	}
 }
 
+static void do_nothing(void *unused)
+{
+}
+
 void cpu_idle_wait(void)
 {
 	unsigned int cpu, this_cpu = get_cpu();

@@ -228,6 +232,13 @@ void cpu_idle_wait(void)
 				cpu_clear(cpu, map);
 		}
 		cpus_and(map, map, cpu_online_map);

arch/x86/kernel/process_32.c void cpu_idle_wait(void) · line 244

Quoted 10 lines

arch/x86/kernel/process_32.c +7 -0

+		/*
+		 * 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);

This seems rather hacky. Although it may turn out to be the most efficient fix, dunno.

I'd have thought that the right fix would be to plug the race which you described at the top-of-thread. That might require some redesign, but it sounds like the design is wrong anyway.

Maybe your proposed fix is suitable for a 2.6.24 bandaid..

OK, it's called infrequently, so a few extra IPIs there won't hurt.

btw, it's pretty damn sad that cpu_idle_wait() will always stall for at least one second. That's a huge amount of time and I bet it's thousands of times longer than is actually needed..

Steven Rostedt

author

2008-01-10 00:06 UTC #9 lore raw

Re: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait

Quoted 4 lines · Andrew Morton

Quoted 2 lines
Subject: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait
s/cpus_/cpu_/

I've been up to 4am writing patches. I must be seeing double :-/

Quoted 15 lines

{

arch/x86/kernel/process_32.c +7 -0

 	unsigned int cpu, this_cpu = get_cpu();

@@ -228,6 +232,13 @@ void cpu_idle_wait(void)
 				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));

Quoted 4 lines

Review void cpu_idle_wait(void) · line 244
Quoted 1 line
(quoted diff) +0 -0
 	set_cpus_allowed(current, tmp);
This seems rather hacky. Although it may turn out to be the most efficient fix, dunno.

s/seems/is/

Quoted 6 lines

I'd have thought that the right fix would be to plug the race which you described at the top-of-thread. That might require some redesign, but it sounds like the design is wrong anyway.
Maybe your proposed fix is suitable for a 2.6.24 bandaid..

I was thinking the same thing.

Quoted 6 lines

<looks at cpu_idle_wait()>
<pokes his tongue out at the person who put in a global, exported-to-modules interface and didn't bother documenting it>

Quoted 7 lines

OK, it's called infrequently, so a few extra IPIs there won't hurt.
btw, it's pretty damn sad that cpu_idle_wait() will always stall for at least one second. That's a huge amount of time and I bet it's thousands of times longer than is actually needed..

I didn't like that either. But I was focusing on something else, and I was getting sick and tired of my box hanging on bootup every once in a while (usually when I reboot and walk away, just to come back to find the box hung).

So this was my band-aid, and since it was only happening on the box running with my patches, I thought it may have been something I did. But then it finally hung on a reboot to a vanilla kernel, so I decided to at least send my band-aid out.

If anything, this should get some notice and we can have a proper fix for .25.

-- Steve

Pallipadi, Venkatesh

2008-01-10 00:31 UTC #10 lore raw

RE: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait

In reply to Steven Rostedt

Quoted 15 lines

-----Original Message-----
From: Steven Rostedt [mailto:[email protected]]
Sent: Wednesday, January 09, 2008 12:42 PM
To: LKML
Cc: Linus Torvalds; Andrew Morton; Ingo Molnar; Thomas Gleixner; Brown, Len; Pallipadi, Venkatesh; Adam Belay; Peter Zijlstra; Andi Kleen
Subject: [PATCH] Kick CPUS that might be sleeping in cpus_idle_wait
This patch is different than the first patch I sent out. This one just sends an IPI to all CPUS that don't check in after 1 sec.

Quoted 7 lines

Sometimes cpu_idle_wait gets stuck because it might miss CPUS that are already in idle, have no tasks waiting to run and have no interrupts going to them. This is common on bootup when switching cpu idle governors.
This patch gives those CPUS that don't check in an IPI kick.

I think your RFC patch is the right solution here. As I see it, there is no race with your RFC patch.
As long as you call a dummy smp_call_function on all CPUs, we should be OK. We can get rid of cpu_idle_state
and the current wait forever logic altogether with dummy smp_call_function. And so there wont be any wait forever scenario.

The whole point of cpu_idle_wait() is to make all CPUs come out of idle loop atleast once.
The caller will use cpu_idle_wait something like this.

// Want to change idle handler
- Switch global idle handler to always present default_idle
- call cpu_idle_wait so that all cpus come out of idle for an instant and stop using old idle pointer and start using default idle
- Change the idle handler to a new handler
- optional cpu_idle_wait if you want all cpus to start using the new handler immediately.

May be the below 1s patch is safe bet for .24. But for .25, I would say we just replace all complicated logic by simple dummy smp_call_function and remove cpu_idle_state altogether.

Thanks,
Venki

Quoted 71 lines

Signed-off-by Steven Rostedt <[email protected]>

---

arch/x86/kernel/process_32.c |   11 +++++++++++
arch/x86/kernel/process_64.c |   11 +++++++++++
2 files changed, 22 insertions(+)

Index: linux-compile-i386.git/arch/x86/kernel/process_32.c
===================================================================
--- linux-compile-i386.git.orig/arch/x86/kernel/process_32.c

2008-01-09 14:09:36.000000000 -0500
+++ linux-compile-i386.git/arch/x86/kernel/process_32.c	
2008-01-09 14:09:45.000000000 -0500

arch/x86/kernel/process_32.c +11 -0

@@ -204,6 +204,10 @@ void cpu_idle(void)
 	}
 }
 
+static void do_nothing(void *unused)
+{
+}
+
 void cpu_idle_wait(void)
 {
 	unsigned int cpu, this_cpu = get_cpu();

@@ -228,6 +232,13 @@ void cpu_idle_wait(void)
 				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);

Index: linux-compile-i386.git/arch/x86/kernel/process_64.c
===================================================================
--- linux-compile-i386.git.orig/arch/x86/kernel/process_64.c

2008-01-09 14:09:36.000000000 -0500
+++ linux-compile-i386.git/arch/x86/kernel/process_64.c	
2008-01-09 15:17:20.000000000 -0500

arch/x86/kernel/process_64.c +11 -0

@@ -135,6 +135,10 @@ static void poll_idle (void)
 	cpu_relax();
 }
 
+static void do_nothing(void *unused)
+{
+}
+
 void cpu_idle_wait(void)
 {
 	unsigned int cpu, this_cpu = get_cpu();

@@ -160,6 +164,13 @@ void cpu_idle_wait(void)
 				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);

A message that is not in the archive (sent off-list or not yet received): B5B0CFF685D7DF46A05CF1678CFB42ED20E00653@orsmsx423.amr.corp.intel.com

Venki Pallipadi

2008-02-08 01:06 UTC #12 lore raw

PATCH 1/1 x86: Simplify cpu_idle_wait

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 <[email protected]>

arch/x86/kernel/process_32.c +11 ~~-36~~

Index: linux-2.6.25-rc/arch/x86/kernel/process_32.c
===================================================================
@@ -82,7 +82,6 @@ unsigned long thread_saved_pc(struct tas
  */
 void (*pm_idle)(void);
 EXPORT_SYMBOL(pm_idle);
-static DEFINE_PER_CPU(unsigned int, cpu_idle_state);
 
 void disable_hlt(void)
 {

@@ -181,9 +180,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;

@@ -208,40 +204,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, 0);
 }
 EXPORT_SYMBOL_GPL(cpu_idle_wait);

arch/x86/kernel/process_64.c +11 ~~-37~~

68 lines not on this page · raw message

Andi Kleen

2008-02-08 10:29 UTC #13 lore raw

Re: [PATCH] x86: Simplify cpu_idle_wait

arch/x86/kernel/process_32.c static void do_nothing(void *unused) · line 219

Quoted 5 lines

arch/x86/kernel/process_32.c +3 -1

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

I think the last argument (wait) needs to be 1 to make sure it is synchronous (for 32/64) Otherwise the patch looks great.

-Andi

Venki Pallipadi

2008-02-08 17:24 UTC #14 lore raw

Re: [PATCH] x86: Simplify cpu_idle_wait

Quoted 8 lines · Andi Kleen

Review
Quoted 5 lines
(quoted diff) +3 -1
-	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, 0);
I think the last argument (wait) needs to be 1 to make sure it is synchronous (for 32/64) Otherwise the patch looks great.

Yes. Below is the updated patch

Signed-off-by Venkatesh Pallipadi <[email protected]>

arch/x86/kernel/process_32.c +11 ~~-36~~

Index: linux-2.6.25-rc/arch/x86/kernel/process_32.c
===================================================================
@@ -82,7 +82,6 @@ unsigned long thread_saved_pc(struct tas
  */
 void (*pm_idle)(void);
 EXPORT_SYMBOL(pm_idle);
-static DEFINE_PER_CPU(unsigned int, cpu_idle_state);
 
 void disable_hlt(void)
 {

@@ -181,9 +180,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;

@@ -208,40 +204,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);

arch/x86/kernel/process_64.c +11 ~~-37~~

68 lines not on this page · raw message

Andi Kleen

Acked-by

2008-02-08 18:45 UTC #15 lore raw

Re: [PATCH] x86: Simplify cpu_idle_wait

Quoted 11 lines · Venki Pallipadi

Quoted 8 lines · Andi Kleen
arch/x86/kernel/process_32.c static void do_nothing(void *unused)
Quoted 5 lines
arch/x86/kernel/process_32.c +3 -1
-	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, 0);
I think the last argument (wait) needs to be 1 to make sure it is synchronous (for 32/64) Otherwise the patch looks great.
Yes. Below is the updated patch

Acked-by Andi Kleen <[email protected]>

-Andi

Thomas Gleixner

Applied

2008-02-09 09:35 UTC #16 lore raw

Re: [PATCH] x86: Simplify cpu_idle_wait

In reply to Venki Pallipadi

Quoted 11 lines · Venki Pallipadi

Quoted 8 lines · Andi Kleen
arch/x86/kernel/process_32.c static void do_nothing(void *unused)
Quoted 5 lines
arch/x86/kernel/process_32.c +3 -1
-	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, 0);
I think the last argument (wait) needs to be 1 to make sure it is synchronous (for 32/64) Otherwise the patch looks great.
Yes. Below is the updated patch

Applied. Thanks,

tglx

Quoted 162 lines

Signed-off-by Venkatesh Pallipadi <[email protected]>

Index: linux-2.6.25-rc/arch/x86/kernel/process_32.c
===================================================================

arch/x86/kernel/process_32.c +11 ~~-36~~

@@ -82,7 +82,6 @@ unsigned long thread_saved_pc(struct tas
  */
 void (*pm_idle)(void);
 EXPORT_SYMBOL(pm_idle);
-static DEFINE_PER_CPU(unsigned int, cpu_idle_state);
 
 void disable_hlt(void)
 {

@@ -181,9 +180,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;

@@ -208,40 +204,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);

Index: linux-2.6.25-rc/arch/x86/kernel/process_64.c
===================================================================

arch/x86/kernel/process_64.c +11 ~~-37~~

@@ -64,7 +64,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);

@@ -139,41 +138,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);

@@ -215,9 +192,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;
-
 			tick_nohz_stop_sched_tick();
 
 			rmb();

Venki Pallipadi

2008-04-10 16:55 UTC #17 lore raw

Re: [PATCH] x86: Simplify cpu_idle_wait

Quoted 9 lines · Thomas Gleixner

   On Fri, 8 Feb 2008, Venki Pallipadi wrote:

   > On Fri, Feb 08, 2008 at 11:28:48AM +0100, Andi Kleen wrote:
   > >
   > > > - 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, 0);
   > >

Quoted 8 lines

   > > I think the last argument (wait) needs to be 1 to make sure it is
   > > synchronous (for 32/64) Otherwise the patch looks great.
   >
   > Yes. Below is the updated patch

   Applied. Thanks,

            tglx

thomas/ingo/hpa,

Looks like this patch never made it to mainline.

The earlier patch does not apply cleanly to mainline any more. Below is the updated patch.

Thanks,
Venki

As a bonus, this patch seems to resolve the bug reported here
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.

Signed-off-by Venkatesh Pallipadi <[email protected]>

---
 arch/x86/kernel/process_32.c |   47 ++++++++++---------------------------------
 arch/x86/kernel/process_64.c |   47 ++++++++++---------------------------------
 2 files changed, 22 insertions(+), 72 deletions(-)

arch/x86/kernel/process_32.c +11 ~~-36~~

Index: linux-2.6/arch/x86/kernel/process_32.c
===================================================================
@@ -82,7 +82,6 @@ unsigned long thread_saved_pc(struct tas
  */
 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);

arch/x86/kernel/process_64.c +11 ~~-36~~

67 lines not on this page · raw message