Strange code in cpu_idle()
RFC 1/1 Strange code in cpu_idle()
Hello, Steve,
OK, I believe I found the other end of this:
static void __exit apm_exit(void)
{
int error;
if (set_pm_idle) {
pm_idle = original_pm_idle;
/*
* We are about to unload the current idle thread pm callback
* (pm_idle), Wait for all processors to update cached/local
* copies of pm_idle before proceeding.
*/
synchronize_kernel();
}Unfortunately, the idle loop is a quiescent state, so it is possible for synchronize_kernel() to return before the idle threads have returned. So I don't believe RCU is useful here. One other approach would be to keep a cpu mask, in which apm_exit() sets all bits, and pm_idle() clears its CPU's bit only if it is set. Then apm_exit() could wait for all CPU's bits to clear.
There is probably a better way to do this, but that is what comes to mind immediately.
Thoughts?
Thanx, Paul
----- Forwarded message from "Paul E. McKenney" <[email protected]> -----
Date: Sat, 4 Dec 2004 15:11:49 -0800
From: "Paul E. McKenney" <[email protected]>
To: [email protected], [email protected], [email protected], [email protected],
[email protected]
Cc: [email protected]
Subject: [RFC] Strange code in cpu_idle()
Reply-To: [email protected]
Hello!
Strange code in i386, ia64, and x86-64 cpu_idle():
void cpu_idle (void)
{
/* 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();
idle = pm_idle;
if (!idle)
idle = default_idle;
idle();
rcu_read_unlock();
}
schedule();
}
}Unless idle_cpu() is busted, it seems like the above is, given the code in rcu_check_callbacks():
void rcu_check_callbacks(int cpu, int user)
{
if (user ||
(idle_cpu(cpu) && !in_softirq() &&
hardirq_count() <= (1 << HARDIRQ_SHIFT))) {
rcu_qsctr_inc(cpu);
rcu_bh_qsctr_inc(cpu);
} else if (!in_softirq())
rcu_bh_qsctr_inc(cpu);
tasklet_schedule(&per_cpu(rcu_tasklet, cpu));
}And idle_cpu() is pretty straightforward:
int idle_cpu(int cpu)
{
return cpu_curr(cpu) == cpu_rq(cpu)->idle;
}So I would say that the rcu_read_lock() in cpu_idle() is having no effect, because any timer interrupt from cpu_idle() will mark a quiescent state notwithstanding. What am I missing here?
If I am not missing anything, then the attached patch would be in order here, though there might be some additional work required. (Though I thought that the try_stop_module() stuff took care of all of this these days...)
Note that we really, really do want the idle loop to be an extended quiescent state, otherwise one gets indefinite grace periods and runs out of memory...
Thanx, Paul
arch/i386/kernel/process.c +4 -7
@@ -144,14 +144,12 @@ void cpu_idle (void) {/* endless idle loop with no priority at all */ while (1 ) { +/* +* Note that it is illegal to use RCU read-side +* critical sections within the idle loop. +*/ 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 (); idle = pm_idle;if (!idle)
@@ -159,7 +157,6 @@ void cpu_idle (void) irq_stat[smp_processor_id ()].idle_timestamp = jiffies;idle (); -rcu_read_unlock (); }schedule (); }
arch/ia64/kernel/process.c +4 -7
@@ -230,6 +230,10 @@ cpu_idle (void *unused)/* endless idle loop with no priority at all */ while (1 ) { +/* +* Note that it is illegal to use RCU read-side +* critical sections within the idle loop. +*/ #ifdef CONFIG_SMP if (!need_resched ())min_xtp ();
@@ -239,17 +243,10 @@ 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 (); idle = pm_idle;if (!idle) idle = default_idle; (*idle)(); -rcu_read_unlock (); }if (mark_idle)
arch/x86_64/kernel/process.c +4 -7
@@ -133,19 +133,16 @@ void cpu_idle (void) {/* endless idle loop with no priority at all */ while (1 ) { +/* +* Note that it is illegal to use RCU read-side +* critical sections within the idle loop. +*/ 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 (); idle = pm_idle;if (!idle) idle = default_idle;idle (); -rcu_read_unlock (); }schedule (); }
----- End forwarded message -----
Hi Paul,
Quoted 24 lines · On Sat, 4 Dec 2004 16:45:57 -0800 "Paul E. McKenney" <[email protected]> wrote:
OK, I believe I found the other end of this:
static void
__exitapm_exit(void){ int error; if (set_pm_idle) { pm_idle = original_pm_idle; /* * We are about to unload the current idle thread pm callback * (pm_idle), Wait for all processors to update cached/local * copies of pm_idle before proceeding. */ synchronize_kernel(); }Unfortunately, the idle loop is a quiescent state, so it is possible for
synchronize_kernel()to return before the idle threads have returned. So I don't believe RCU is useful here. One other approach would be to keep a cpu mask, in whichapm_exit()sets all bits, andpm_idle()clears its CPU's bit only if it is set. Thenapm_exit()could wait for all CPU's bits to clear.
Quoted 4 lines
There is probably a better way to do this, but that is what comes to mind immediately.
Thoughts?
None, sorry :-)
I don't even remember that piece of code going in (I certainly didn't write it), though I can see what it was trying to do.
Signature · 3 lines
Cheers, Stephen Rothwell [email protected] http://www.canb.auug.org.au/~sfr/
Quoted 4 lines · On Mon, 6 Dec 2004 11:16:34 +1100 Stephen Rothwell <[email protected]> wrote:
Quoted 2 lines
Thoughts?
None, sorry :-)
Actually, Rusty suggests stop_machine() ...
Signature · 3 lines
Cheers, Stephen Rothwell [email protected] http://www.canb.auug.org.au/~sfr/
Quoted 7 lines · Stephen Rothwell
Quoted 4 lines · On Mon, 6 Dec 2004 11:16:34 +1100 Stephen Rothwell <[email protected]> wrote:
Quoted 2 lines
Thoughts?
None, sorry :-)
Actually, Rusty suggests
stop_machine()...
Hmm i believe there is a window with CONFIG_PREEMPT whereupon we read the pm_idle pointer, get scheduled out to run the stopmachine thread and return with the stale pointer.
Thanks,
Zwane
Quoted 27 lines · Stephen Rothwell
Quoted 27 lines
OK, I believe I found the other end of this:
static void
__exitapm_exit(void){ int error; if (set_pm_idle) { pm_idle = original_pm_idle; /* * We are about to unload the current idle thread pm callback * (pm_idle), Wait for all processors to update cached/local * copies of pm_idle before proceeding. */ synchronize_kernel(); }Unfortunately, the idle loop is a quiescent state, so it is possible for
synchronize_kernel()to return before the idle threads have returned. So I don't believe RCU is useful here. One other approach would be to keep a cpu mask, in whichapm_exit()sets all bits, andpm_idle()clears its CPU's bit only if it is set. Thenapm_exit()could wait for all CPU's bits to clear.There is probably a better way to do this, but that is what comes to mind immediately.
Quoted 6 lines
Quoted 1 line
Thoughts?
None, sorry :-)
I don't even remember that piece of code going in (I certainly didn't write it), though I can see what it was trying to do.
It came in via the below patch. I guess we need to find a different way to fix this problem.
# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/09/17 12:09:12-07:00 [email protected]
# [PATCH] Close race with preempt and modular pm_idle callbacks
#
# The following patch from Shaohua Li fixes a race with preempt enabled when # a module containing a pm_idle callback is unloaded. Cached values in local # variables need to be protected as RCU critical sections so that the # synchronize_kernel() call in the unload path waits for all processors. # There original bugzilla entry can be found at # # Shaohua, i had to make a small change (variable declaration after code in # code block) so that it compiles with geriatric compilers such as the ones # Andrew is attached to ;) # # http://bugzilla.kernel.org/show_bug.cgi?id=1716 # # Signed-off-by: Li Shaohua <[email protected]> # Signed-off-by: Zwane Mwaikambo <[email protected]> # Signed-off-by: Andrew Morton <[email protected]> # Signed-off-by: Linus Torvalds <[email protected]> # # drivers/acpi/processor.c # 2004/09/17 00:07:02-07:00 [email protected] +5 -0 # Close race with preempt and modular pm_idle callbacks # # arch/x86_64/kernel/process.c # 2004/09/17 00:07:02-07:00 [email protected] +13 -4 # Close race with preempt and modular pm_idle callbacks # # arch/ia64/kernel/process.c # 2004/09/17 00:07:02-07:00 [email protected] +12 -4 # Close race with preempt and modular pm_idle callbacks # # arch/i386/kernel/process.c # 2004/09/17 00:07:02-07:00 [email protected] +9 -1 # Close race with preempt and modular pm_idle callbacks # # arch/i386/kernel/apm.c # 2004/09/17 00:07:02-07:00 [email protected] +8 -1 # Close race with preempt and modular pm_idle callbacks # diff -Nru a/arch/i386/kernel/apm.c b/arch/i386/kernel/apm.c --- a/arch/i386/kernel/apm.c 2004-12-05 23:18:08 -08:00 +++ b/arch/i386/kernel/apm.c 2004-12-05 23:18:08 -08:00 @@ -2362,8 +2362,15 @@ { int error;
- if (set_pm_idle)
+ if (set_pm_idle) {
pm_idle = original_pm_idle;
+ /*
+ * We are about to unload the current idle thread pm callback
+ * (pm_idle), Wait for all processors to update cached/local
+ * copies of pm_idle before proceeding.
+ */
+ synchronize_kernel();
+ }
if (((apm_info.bios.flags & APM_BIOS_DISENGAGED) == 0)
&& (apm_info.connection_version > 0x0100)) {
error = apm_engage_power_management(APM_DEVICE_ALL, 0);
arch/i386/kernel/process.c +9 -1
@@ -142,13 +142,21 @@/* endless idle loop with no priority at all */ while (1 ) {while (!need_resched ()) { -void (*idle)(void ) = pm_idle; +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 (); + idle = pm_idle;if (!idle) idle = default_idle; irq_stat[smp_processor_id ()].idle_timestamp = jiffies;idle (); +rcu_read_unlock (); }schedule (); }
arch/ia64/kernel/process.c +12 -4
@@ -228,18 +228,26 @@/* endless idle loop with no priority at all */ while (1 ) { -void (*idle)(void ) = pm_idle; -if (!idle) - idle = default_idle; -#ifdef CONFIG_SMP if (!need_resched ())min_xtp ();#endif while (!need_resched ()) { +void (*idle)(void ); +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 (); + idle = pm_idle; +if (!idle) + idle = default_idle; (*idle)(); +rcu_read_unlock (); }if (mark_idle)
arch/x86_64/kernel/process.c +13 -4
@@ -130,11 +130,20 @@ {/* endless idle loop with no priority at all */ while (1 ) { -void (*idle)(void ) = pm_idle; -if (!idle) - idle = default_idle; -while (!need_resched ()) +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 (); + idle = pm_idle; +if (!idle) + idle = default_idle;idle (); +rcu_read_unlock (); + }schedule (); } }
drivers/acpi/processor.c +5 -0
@@ -2419,6 +2419,11 @@/* Unregister the idle handler when processor #0 is removed. */ if (pr->id ==0 ) { pm_idle = pm_idle_save; +/* +* We are about to unload the current idle thread pm callback +* (pm_idle), Wait for all processors to update cached/local +* copies of pm_idle before proceeding. +*/ synchronize_kernel (); }
Hi,
The original intent to go with synchronize_kernel and RCU
protection was for simplicity's sake, as the alternative implementations at the time looked like major overkill. Now in defense of this method, when entering the idle thread and placing the processor in a holding state (hlt) and an RCU grace period is begun, the processor in the holding state will be unaware of the new RCU grace period until it exits the idle loop callback (pm_idle) anyway, so the rcu_read will block the other processors from making RCU grace period completion as much as the processor holding state. This is true of all current pm_idle callbacks on i386, x86_64 and ia64 with the exception of APM (but i'll conveniently ignore that for now ;). When we do take an interrupt to exit the processor holding state and run through rcu_check_callbacks we will notice that we are in a hard interrupt and will defer marking of the processsor as quiescent. By that point we will have exited the idle thread callback therefore making it safe to use synchronize_kernel to protect removal of the callback.
Thanks,
Zwane "who usually doesn't condone interface abuse" Mwaikambo
Quoted 8 lines · Zwane Mwaikambo
Hi,
The original intent to go withsynchronize_kerneland RCU
protection was for simplicity's sake, as the alternative implementations at the time looked like major overkill. Now in defense of this method, when entering the idle thread and placing the processor in a holding state (hlt) and an RCU grace period is begun, the processor in the holding state will be unaware of the new RCU grace period until it exits the idle loop callback (pm_idle) anyway, so thercu_readwill block the other processors
Quoted 8 lines
from making RCU grace period completion as much as the processor holding state. This is true of all current
pm_idlecallbacks on i386,x86_64and ia64 with the exception of APM (but i'll conveniently ignore that for now ;). When we do take an interrupt to exit the processor holding state and run throughrcu_check_callbackswe will notice that we are in a hard interrupt and will defer marking of the processsor as quiescent. By that point we will have exited the idle thread callback therefore making it safe to usesynchronize_kernelto protect removal of the callback.
I am not going to claim to thoroughly understand the power-management code, but do have an additional question.
What happens if the processor became aware of a new grace period just before entering pm_idle? I could imagine this code simply refusing to power down the processor if there was a pending grace period, but I don't see any sign of this. I could also imagine somehow deferring interrupts until pm_idle exits. I don't see anything that looks like it does this, but don't claim to be any sort of power-management expert.
Thanx, Paul
Quoted 10 lines · Paul E. McKenney
I am not going to claim to thoroughly understand the power-management code, but do have an additional question.
What happens if the processor became aware of a new grace period just before entering
pm_idle? I could imagine this code simply refusing to power down the processor if there was a pending grace period, but I don't see any sign of this. I could also imagine somehow deferring interrupts untilpm_idleexits. I don't see anything that looks like it does this, but don't claim to be any sort of power-management expert.
Are you referring to the synchronize_kernel side? That's basically unload module path so it's ok if it sits there for a bit, but it should only last for as long as the next interrupt, which would be a pretty short perid considering HZ=1000. But the usage still has a race and hence invalid as pointed out by Dipankar
Thanks,
Zwane
Quoted 13 lines · Zwane Mwaikambo
Quoted 10 lines · Paul E. McKenney
I am not going to claim to thoroughly understand the power-management code, but do have an additional question.
What happens if the processor became aware of a new grace period just before entering
pm_idle? I could imagine this code simply refusing to power down the processor if there was a pending grace period, but I don't see any sign of this. I could also imagine somehow deferring interrupts untilpm_idleexits. I don't see anything that looks like it does this, but don't claim to be any sort of power-management expert.
Quoted 5 lines
Are you referring to the
synchronize_kernelside? That's basically unload module path so it's ok if it sits there for a bit, but it should only last for as long as the next interrupt, which would be a pretty short perid considering HZ=1000. But the usage still has a race and hence invalid as pointed out by Dipankar
My bad -- I hadn't read through the entire thread before responding, so was speculating on how it might manage to be correct.
Thanx, Paul
Quoted 2 lines · Paul E. McKenney
My bad -- I hadn't read through the entire thread before responding, so was speculating on how it might manage to be correct.
Ok, i'll come up with a method which doesn't use RCU. Thank you, for pointing out the faults.
Zwane
PATCH 1/1 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.
Tested on EM64T/SMP/x86_64 with modprobe/rmmod loops of ACPI processor/thermal modules.
arch/i386/kernel/apm.c | 2 +- arch/i386/kernel/process.c | 27 ++++++++++++++++++++------- arch/ia64/kernel/process.c | 26 +++++++++++++++++++------- arch/x86_64/kernel/process.c | 27 ++++++++++++++++++++------- drivers/acpi/processor.c | 2 +- include/asm-i386/system.h | 1 + include/asm-ia64/system.h | 1 + include/asm-x86_64/system.h | 2 ++ 8 files changed, 65 insertions(+), 23 deletions(-)
Index: linux-2.6.10-rc2/arch/i386/kernel/apm.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/arch/i386/kernel/apm.c,v retrieving revision 1.1.1.1
arch/i386/kernel/apm.c +1 -1
@@ -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 )) {
Index: linux-2.6.10-rc2/arch/i386/kernel/process.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/arch/i386/kernel/process.c,v retrieving revision 1.1.1.1
arch/i386/kernel/process.c +20 -7
@@ -72,6 +72,7 @@ unsigned long thread_saved_pc(struct tas* Powermanagement idle function, if any.. */ void (*pm_idle)(void ); +static cpumask_t cpu_idle_map;void disable_hlt (void ) {
@@ -142,16 +143,16 @@ 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)
@@ -159,12 +160,24 @@ void cpu_idle (void) irq_stat[smp_processor_id ()].idle_timestamp = jiffies;idle (); -rcu_read_unlock (); }schedule (); } } +void cpu_idle_wait (void ) +{ +int cpu; + +for_each_online_cpu (cpu) +cpu_set (cpu, cpu_idle_map); + +wmb (); +while (cpus_equal (cpu_idle_map, cpu_online_map)) +schedule_timeout (HZ ); +} +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.
Index: linux-2.6.10-rc2/arch/ia64/kernel/process.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/arch/ia64/kernel/process.c,v retrieving revision 1.1.1.1
arch/ia64/kernel/process.c +19 -7
@@ -46,6 +46,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);
@@ -223,10 +224,24 @@ static inline void play_dead(void) }#endif /* CONFIG_HOTPLUG_CPU */ +void cpu_idle_wait (void ) +{ +int cpu; + +for_each_online_cpu (cpu) +cpu_set (cpu, cpu_idle_map); + +wmb (); +while (cpus_equal (cpu_idle_map, cpu_online_map)) +schedule_timeout (HZ ); +} +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 ) {
@@ -239,17 +254,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)
Index: linux-2.6.10-rc2/arch/x86_64/kernel/process.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/arch/x86_64/kernel/process.c,v retrieving revision 1.1.1.1
arch/x86_64/kernel/process.c +20 -7
@@ -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,19 @@ static void poll_idle (void) } } +void cpu_idle_wait (void ) +{ +int cpu; + +for_each_online_cpu (cpu) +cpu_set (cpu, cpu_idle_map); + +wmb (); +while (cpus_equal (cpu_idle_map, cpu_online_map)) +schedule_timeout (HZ ); +} +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 +145,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 (); }
Index: linux-2.6.10-rc2/drivers/acpi/processor.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/drivers/acpi/processor.c,v retrieving revision 1.1.1.1
drivers/acpi/processor.c +1 -1
@@ -2535,7 +2535,7 @@ acpi_processor_remove (* (pm_idle), Wait for all processors to update cached/local * copies of pm_idle before proceeding. */ -synchronize_kernel (); +cpu_idle_wait (); } status = acpi_remove_notify_handler(pr->handle, ACPI_DEVICE_NOTIFY,
Index: linux-2.6.10-rc2/include/asm-i386/system.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/include/asm-i386/system.h,v retrieving revision 1.1.1.1
include/asm-i386/system.h +1 -0
@@ -466,5 +466,6 @@ void disable_hlt(void);void enable_hlt (void );extern int es7000_plat; +void cpu_idle_wait (void );#endif
Index: linux-2.6.10-rc2/include/asm-ia64/system.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/include/asm-ia64/system.h,v retrieving revision 1.1.1.1
include/asm-ia64/system.h +1 -0
@@ -284,6 +284,7 @@ do { \#define ia64_platform_is (x) (strcmp(x, platform_name) == 0) +void cpu_idle_wait (void );#endif /* __KERNEL__ */ #endif /* __ASSEMBLY__ */
Index: linux-2.6.10-rc2/include/asm-x86_64/system.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/include/asm-x86_64/system.h,v retrieving revision 1.1.1.1
include/asm-x86_64/system.h +2 -0
@@ -326,6 +326,8 @@ static inline unsigned long __cmpxchg(vo/* 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] Remove RCU abuse in cpu_idle()
Quoted 6 lines · Zwane Mwaikambo
Introduce
cpu_idle_wait()on architectures requiring modification ofpm_idlefrom modules, this will ensure that all processors have updated their cached values ofpm_idleupon 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.
Aargh, i shouldn't be programming whilst sick :( i'll repost a non broken version shortly.
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 6 lines · Zwane Mwaikambo
Quoted 6 lines
Introduce
cpu_idle_wait()on architectures requiring modification ofpm_idlefrom modules, this will ensure that all processors have updated their cached values ofpm_idleupon 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.
arch/i386/kernel/apm.c | 2 +- arch/i386/kernel/process.c | 30 +++++++++++++++++++++++------- arch/ia64/kernel/process.c | 30 +++++++++++++++++++++++------- arch/x86_64/kernel/process.c | 31 ++++++++++++++++++++++++------- drivers/acpi/processor.c | 2 +- include/asm-i386/system.h | 1 + include/asm-ia64/system.h | 1 + include/asm-x86_64/system.h | 2 ++ 8 files changed, 76 insertions(+), 23 deletions(-)
Index: linux-2.6.10-rc2/arch/i386/kernel/apm.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/arch/i386/kernel/apm.c,v retrieving revision 1.1.1.1
arch/i386/kernel/apm.c +1 -1
@@ -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 )) {
Index: linux-2.6.10-rc2/arch/i386/kernel/process.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/arch/i386/kernel/process.c,v retrieving revision 1.1.1.1
arch/i386/kernel/process.c +23 -7
@@ -72,6 +72,7 @@ unsigned long thread_saved_pc(struct tas* Powermanagement idle function, if any.. */ void (*pm_idle)(void ); +static cpumask_t cpu_idle_map;void disable_hlt (void ) {
@@ -142,16 +143,16 @@ 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)
@@ -159,12 +160,27 @@ void cpu_idle (void) irq_stat[smp_processor_id ()].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 { +schedule_timeout (HZ ); +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.
Index: linux-2.6.10-rc2/arch/ia64/kernel/process.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/arch/ia64/kernel/process.c,v retrieving revision 1.1.1.1
arch/ia64/kernel/process.c +23 -7
@@ -46,6 +46,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);
@@ -223,10 +224,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 { +schedule_timeout (HZ ); +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 ) {
@@ -239,17 +258,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)
Index: linux-2.6.10-rc2/arch/x86_64/kernel/process.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/arch/x86_64/kernel/process.c,v retrieving revision 1.1.1.1
arch/x86_64/kernel/process.c +24 -7
@@ -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 { +schedule_timeout (HZ ); +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 (); }
Index: linux-2.6.10-rc2/drivers/acpi/processor.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/drivers/acpi/processor.c,v retrieving revision 1.1.1.1
drivers/acpi/processor.c +1 -1
@@ -2535,7 +2535,7 @@ acpi_processor_remove (* (pm_idle), Wait for all processors to update cached/local * copies of pm_idle before proceeding. */ -synchronize_kernel (); +cpu_idle_wait (); } status = acpi_remove_notify_handler(pr->handle, ACPI_DEVICE_NOTIFY,
Index: linux-2.6.10-rc2/include/asm-i386/system.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/include/asm-i386/system.h,v retrieving revision 1.1.1.1
include/asm-i386/system.h +1 -0
@@ -466,5 +466,6 @@ void disable_hlt(void);void enable_hlt (void );extern int es7000_plat; +void cpu_idle_wait (void );#endif
Index: linux-2.6.10-rc2/include/asm-ia64/system.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/include/asm-ia64/system.h,v retrieving revision 1.1.1.1
include/asm-ia64/system.h +1 -0
@@ -284,6 +284,7 @@ do { \#define ia64_platform_is (x) (strcmp(x, platform_name) == 0) +void cpu_idle_wait (void );#endif /* __KERNEL__ */ #endif /* __ASSEMBLY__ */
Index: linux-2.6.10-rc2/include/asm-x86_64/system.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.10-rc2/include/asm-x86_64/system.h,v retrieving revision 1.1.1.1
include/asm-x86_64/system.h +2 -0
@@ -326,6 +326,8 @@ static inline unsigned long __cmpxchg(vo/* 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] Remove RCU abuse in cpu_idle()
Quoted 14 lines · Zwane Mwaikambo
Quoted 6 lines · Zwane Mwaikambo
Quoted 6 lines
Introduce
cpu_idle_wait()on architectures requiring modification ofpm_idlefrom modules, this will ensure that all processors have updated their cached values ofpm_idleupon 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....
void
cpu_idle(void)
{
arch/i386/kernel/process.c static void poll_idle (void) · line 148
Quoted 3 lines
arch/i386/kernel/process.c +2
-0+int cpu =smp_processor_id (); +/* endless idle loop with no priority at all */
This gives me scadzillions of "using smp_procesor_id() in preemptible" warnings.
I'll shut that up with _smp_processor_id() but one does wonder what happens if we get preempted and `cpu' refers to some other CPU?
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 11 lines · Andrew Morton
arch/i386/kernel/process.c static void poll_idle (void) · line 148
Quoted 5 lines
void
cpu_idle(void)
{arch/i386/kernel/process.c +2
-0+int cpu =smp_processor_id (); +/* endless idle loop with no priority at all */ This gives me scadzillions of "using
smp_procesor_id()in preemptible" warnings.I'll shut that up with
_smp_processor_id()but one does wonder what happens if we get preempted and `cpu' refers to some other CPU?
The idle thread is special in the sense that it can't get migrated so the cached values of smp_processor_id are fine.
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 3 lines · Zwane Mwaikambo
The idle thread is special in the sense that it can't get migrated so the
cached values ofsmp_processor_idare fine.
duh, knew that.
We can use the cached value throughout, no?
arch/i386/kernel/process.c renamed from arch/i386/kernel/process.c~remove-rcu-abuse-in-cpu_idle-warning-fix +2 -2
@@ -146,7 +146,7 @@ static void poll_idle (void)*/ void cpu_idle (void ) { -int cpu =smp_processor_id (); +int cpu =_smp_processor_id ();/* endless idle loop with no priority at all */ while (1 ) {
@@ -161,7 +161,7 @@ void cpu_idle (void)if (!idle) idle = default_idle; - irq_stat[smp_processor_id ()].idle_timestamp = jiffies; + irq_stat[cpu].idle_timestamp = jiffies;idle (); }schedule ();
_
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 8 lines · Andrew Morton
Quoted 3 lines · Zwane Mwaikambo
The idle thread is special in the sense that it can't get migrated so the
cached values ofsmp_processor_idare fine.duh, knew that.
We can use the cached value throughout, no?
Oh yes, silly me i forgot about those.
Thanks!
Re: [PATCH] Remove RCU abuse in cpu_idle()
In reply to Zwane MwaikamboQuoted 3 lines · Zwane Mwaikambo
Quoted 3 lines
This gives me scadzillions of "using
smp_procesor_id()in preemptible" warnings.
As does the current_cpu_data evaluation in default_idle(), now we've gone and dropped the rcu_read_lock() from around it. That debugging patch is a pain.
I'll switch it to boot_cpu_data.
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 10 lines · Andrew Morton
Quoted 3 lines · Zwane Mwaikambo
Quoted 3 lines
This gives me scadzillions of "using
smp_procesor_id()in preemptible" warnings.As does the
current_cpu_dataevaluation indefault_idle(), now we've gone and dropped thercu_read_lock()from around it. That debugging patch is a pain.I'll switch it to
boot_cpu_data.
I really should have turned on all the debugging knobs, although i might have hesitated before scattering _smp_processor_id in places.
Re: [PATCH] Remove RCU abuse in cpu_idle()
In reply to Zwane MwaikamboQuoted 8 lines · Zwane Mwaikambo
Quoted 6 lines · Zwane Mwaikambo
Quoted 6 lines
Introduce
cpu_idle_wait()on architectures requiring modification ofpm_idlefrom modules, this will ensure that all processors have updated their cached values ofpm_idleupon 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.
<snip>
arch/i386/kernel/process.c void cpu_idle (void) · line 180
Quoted 5 lines
arch/i386/kernel/process.c +5
-0+wmb (); +do { +schedule_timeout (HZ ); +cpus_and (map, cpu_idle_map, cpu_online_map); + }while (!cpus_empty (map));
<snip>
All of these schedule_timeout() calls are broken. They do not set the state before hand and therefore will return early. Since you're not checking for signals and there are no waitqueue events around the code, I'm assuming you can just use ssleep(1) instead of the current schedule_timeout() calls.
Thanks,
Nish
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 5 lines · Nish Aravamudan
All of these
schedule_timeout()calls are broken. They do not set the state before hand and therefore will return early. Since you're not checking for signals and there are no waitqueue events around the code, I'm assuming you can just use ssleep(1) instead of the currentschedule_timeout()calls.
Returning early is fine (and will happen if the other processors are busy), we're spinning on a condition, but yes ssleep() could be used instead.
Thanks,
Zwane
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 8 lines · Zwane Mwaikambo
Quoted 5 lines · Nish Aravamudan
All of these
schedule_timeout()calls are broken. They do not set the state before hand and therefore will return early. Since you're not checking for signals and there are no waitqueue events around the code, I'm assuming you can just use ssleep(1) instead of the currentschedule_timeout()calls.
Quoted 4 lines
Returning early is fine (and will happen if the other processors are busy), we're spinning on a condition, but yes
ssleep()could be used instead.
Hi Zwane,
This thread can possibly be stalled forever if there is a CPU hog running, right?
In which case, you will want to use ssleep rather than a busy loop.
Another alternative may be to use more complex logic to detect that a CPU is not in the idle loop at all. In that case, a simple cpu_relax type spin loop should be OK, because the synchronisation would be achieved very quickly.
Nick
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 2 lines · Nick Piggin
This thread can possibly be stalled forever if there is a CPU hog running, right?
Yep.
Quoted 1 line
In which case, you will want to use ssleep rather than a busy loop.
Well ssleep essentially does the same thing as the schedule_timeout.
Quoted 4 lines
Another alternative may be to use more complex logic to detect that a CPU is not in the idle loop at all. In that case, a simple
cpu_relaxtype spin loop should be OK, because the synchronisation would be achieved very quickly.
I considered checking whether the cpu is in the idle thread or not but wouldn't that require locking runqueues? Something like;
pm_idle = new_value;wmb();busy_map = cpu_online_map;
for_each_online_cpu(cpu) {
runqueue_t *rq = cpu_rq(cpu);
spin_lock_irq(&rq->lock);
if (rq->curr != rq->idle)
cpu_clear(cpu, busy_map);
spin_unlock_irq(&rq->lock);
}cpu_idle_map = busy_map;wmb();
while (!cpus_empty(cpu_idle_map)) {
cpus_and(cpu_idle_map, cpu_idle_map, cpu_online_map);
ssleep(1);
}Hmm then again, i think we could get away with doing an unlocked compare on the rq->curr and rq->idle since we've written back pm_idle and reading a stale rq->curr which isn't equal to rq->idle means that the remote processor should also have the new pm_idle. I'm still not convinced that it deserves this much complexity, this is a rarely carried out operation and usually at boottime or shutdown.
Thanks,
Zwane
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 11 lines · Zwane Mwaikambo
Quoted 2 lines · Nick Piggin
This thread can possibly be stalled forever if there is a CPU hog running, right?
Yep.
Quoted 1 line
In which case, you will want to use ssleep rather than a busy loop.
Well ssleep essentially does the same thing as the
schedule_timeout.
Yes - so long as you set ->state when using schedule_timeout ;)
Quoted 27 lines
Quoted 4 lines
Another alternative may be to use more complex logic to detect that a CPU is not in the idle loop at all. In that case, a simple
cpu_relaxtype spin loop should be OK, because the synchronisation would be achieved very quickly.I considered checking whether the cpu is in the idle thread or not but wouldn't that require locking runqueues? Something like;
pm_idle=new_value;wmb();busy_map=cpu_online_map;for_each_online_cpu(cpu) { runqueue_t *rq = cpu_rq(cpu); spin_lock_irq(&rq->lock); if (rq->curr != rq->idle) cpu_clear(cpu, busy_map); spin_unlock_irq(&rq->lock); }
cpu_idle_map=busy_map;wmb();while (!cpus_empty(cpu_idle_map)) { cpus_and(cpu_idle_map, cpu_idle_map, cpu_online_map); ssleep(1); }
Quoted 7 lines
Hmm then again, i think we could get away with doing an unlocked compare on the
rq->currandrq->idlesince we've written backpm_idleand reading a stalerq->currwhich isn't equal torq->idlemeans that the remote processor should also have the newpm_idle. I'm still not convinced that it deserves this much complexity, this is a rarely carried out operation and usually at boottime or shutdown.
Hmm, yeah it is fairly complex, now that you've expanded on my handwaving!
I think you are right about not requiring locking, so long as you have the appropriate memory barriers in place... but it's probably not worth the effort, as you say.
Nick
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 6 lines · Nick Piggin
Quoted 5 lines · Zwane Mwaikambo
Quoted 2 lines · Nick Piggin
This thread can possibly be stalled forever if there is a CPU hog running, right?
Quoted 8 lines
Quoted 6 lines
Yep.
Quoted 1 line
In which case, you will want to use ssleep rather than a busy loop.
Well ssleep essentially does the same thing as the
schedule_timeout.Yes - so long as you set ->state when using
schedule_timeout;)
Nish could you please submit something to switch it to ssleep?
Thanks,
Zwane
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 10 lines · Zwane Mwaikambo
Quoted 8 lines · Nick Piggin
Quoted 7 lines · Zwane Mwaikambo
Quoted 2 lines · Nick Piggin
This thread can possibly be stalled forever if there is a CPU hog running, right?
Yep.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
In which case, you will want to use ssleep rather than a busy loop.
Well ssleep essentially does the same thing as the
schedule_timeout.Yes - so long as you set ->state when using
schedule_timeout;)Nish could you please submit something to switch it to ssleep?
Yup, I'll send it out tomorrow.
-Nish
Re: [PATCH] Remove RCU abuse in cpu_idle()
In reply to Zwane MwaikamboQuoted 10 lines · Zwane Mwaikambo
Quoted 8 lines · Nick Piggin
Quoted 7 lines · Zwane Mwaikambo
Quoted 2 lines · Nick Piggin
This thread can possibly be stalled forever if there is a CPU hog running, right?
Yep.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
In which case, you will want to use ssleep rather than a busy loop.
Well ssleep essentially does the same thing as the
schedule_timeout.Yes - so long as you set ->state when using
schedule_timeout;)Nish could you please submit something to switch it to ssleep?
I believe the only files/patches that needed to be changed were the process.c changes. Here they are re-worked to use ssleep(1) instead of schedule_timeout(HZ).
arch/i386/kernel/process.c +20 -7
@@ -72,6 +72,7 @@ unsigned long thread_saved_pc(struct tas* Powermanagement idle function, if any.. */ void (*pm_idle)(void ); +static cpumask_t cpu_idle_map;void disable_hlt (void ) {
@@ -142,16 +143,16 @@ 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)
@@ -159,12 +160,24 @@ void cpu_idle (void) irq_stat[smp_processor_id ()].idle_timestamp = jiffies;idle (); -rcu_read_unlock (); }schedule (); } } +void cpu_idle_wait (void ) +{ +int cpu; + +for_each_online_cpu (cpu) +cpu_set (cpu, cpu_idle_map); + +wmb (); +while (cpus_equal (cpu_idle_map, cpu_online_map)) +ssleep (1 ); +} +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.
arch/ia64/kernel/process.c +19 -7
@@ -46,6 +46,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 +226,24 @@ static inline void play_dead(void) }#endif /* CONFIG_HOTPLUG_CPU */ +void cpu_idle_wait (void ) +{ +int cpu; + +for_each_online_cpu (cpu) +cpu_set (cpu, cpu_idle_map); + +wmb (); +while (cpus_equal (cpu_idle_map, cpu_online_map)) +ssleep (1 ); +} +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 +256,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)
arch/x86_64/kernel/process.c +20 -7
@@ -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,19 @@ static void poll_idle (void) } } +void cpu_idle_wait (void ) +{ +int cpu; + +for_each_online_cpu (cpu) +cpu_set (cpu, cpu_idle_map); + +wmb (); +while (cpus_equal (cpu_idle_map, cpu_online_map)) +ssleep (1 ); +} +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 +145,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 (); }
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 2 lines · Nishanth Aravamudan
I believe the only files/patches that needed to be changed were the
process.cchanges. Here they are re-worked to use ssleep(1) instead of
This makes it hard for the person integrating the patches to graft them together. How about just rediffing the whole lot so that my original patch gets replaced? Ideally it should be able to resolve the issue in 2.6-bk standalone.
Thanks,
Zwane
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 6 lines · Zwane Mwaikambo
Quoted 3 lines
I believe the only files/patches that needed to be changed were the
process.cchanges. Here they are re-worked to use ssleep(1) instead ofThis makes it hard for the person integrating the patches to graft them together.
I gave up and just edited the original diff, with
s/schedule_timeout(HZ)/ssleep(1)/
Re: [PATCH] Remove RCU abuse in cpu_idle()
Quoted 10 lines · Andrew Morton
Quoted 6 lines · Zwane Mwaikambo
Quoted 3 lines
I believe the only files/patches that needed to be changed were the
process.cchanges. Here they are re-worked to use ssleep(1) instead ofThis makes it hard for the person integrating the patches to graft them together.
I gave up and just edited the original diff, with
s/schedule_timeout(HZ)/ssleep(1)/
Thanks Andrew.
Re: [PATCH] Remove RCU abuse in cpu_idle()
In reply to Zwane MwaikamboQuoted 9 lines · Zwane Mwaikambo
Quoted 2 lines · Nishanth Aravamudan
I believe the only files/patches that needed to be changed were the
process.cchanges. Here they are re-worked to use ssleep(1) instead ofThis makes it hard for the person integrating the patches to graft them together. How about just rediffing the whole lot so that my original patch gets replaced? Ideally it should be able to resolve the issue in 2.6-bk standalone.
Sorry about that, I will do that next time, thanks for the info.
-Nish
Re: [PATCH] Remove RCU abuse in cpu_idle()
In reply to Nick PigginQuoted 6 lines · Nick Piggin
Quoted 5 lines · Zwane Mwaikambo
Quoted 2 lines · Nick Piggin
This thread can possibly be stalled forever if there is a CPU hog running, right?
Quoted 8 lines
Quoted 6 lines
Yep.
Quoted 1 line
In which case, you will want to use ssleep rather than a busy loop.
Well ssleep essentially does the same thing as the
schedule_timeout.Yes - so long as you set ->state when using
schedule_timeout;)
You do not need to set the state before calling ssleep(). It is set to TASK_UNINTERRUPTIBLE within the loop of ssleep().
-Nish