patches for stop_machine
cover patches for stop_machine
Hi Rusty and all,
This is a proposal of minor improvement for kernel/stop_machine.c
[PATCH 1/3] stop_machine: short exit path for if we cannot create enough threads [PATCH 2/3] stop_machine: add timeout for child thread deployment [PATCH 3/3] stop_machine: add stopmachine_timeout sysctl entry
The main topic is "how about adding timeout for stop_machine?" I think it will act as a safety net.
For example (of silly situation), system can hung with following way:
# ./silly.sh run an evil loop task on AP pid 6138's current affinity mask: ff pid 6138's new affinity mask: fe to pretend lock up, chrt -f -p 99 6138 loop[6138] is on CPU #4 to do stopmachine, try to off #7 echo 0 > /sys/devices/system/cpu/cpu7/online (never return)
After applying patch set here, it can be prevented.
# ./silly.sh : echo 0 > /sys/devices/system/cpu/cpu7/online stopmachine: Failed to stop machine in time(5s). Are there any CPUs on file? ./silly.sh: line 22: echo: write error: Device or resource busy offline is failed OK, kill evil loop[6138] try to off #7 again echo 0 > /sys/devices/system/cpu/cpu7/online CPU #7 is now offline done!
Please refer description of each patch for the detail. All comments are welcomed.
Thanks,
H.Seto
PATCH 1/3 stop_machine: short exit path for if we cannot create enough threads
stop_machine_run() invokes kthread 'kstopmachine' and the kthread creates its children for every other cpus.
The current implementation migrates the child thread to its target cpu as soon as it is created. It means that the children always bother some of other cpus even if we cannot create enough threads.
This patch enable us to skip the migration of children if we cannot have enough threads.
--- kernel/stop_machine.c | 29 +++++++++++++++++++---------- 1 files changed, 19 insertions(+), 10 deletions(-)
kernel/stop_machine.c +19 -10
@@ -20,6 +20,7 @@/* Thread to stop each CPU in user context. */ enum stopmachine_state {STOPMACHINE_WAIT , +STOPMACHINE_DEPLOY ,STOPMACHINE_PREPARE ,STOPMACHINE_DISABLE_IRQ ,STOPMACHINE_EXIT ,
@@ -34,9 +35,16 @@ static int stopmachine(void *cpu)int irqs_disabled =0 ;int prepared =0 ; +/* Wait sisters */ +while (stopmachine_state ==STOPMACHINE_WAIT ) +yield (); +/* short path for cancel */ +if (stopmachine_state ==STOPMACHINE_EXIT ) + goto exit; +set_cpus_allowed_ptr (current, &cpumask_of_cpu ((int )(long )cpu)); -/* Ack: we are alive */ +/* Ack: we arrived */ smp_mb ();/* Theoretically the ack = 0 might not be on this CPU yet. */ atomic_inc (&stopmachine_thread_ack);
@@ -65,7 +73,7 @@ static int stopmachine(void *cpu)else cpu_relax (); } - +exit:/* Ack: we are exiting. */ smp_mb ();/* Must read state first. */ atomic_inc (&stopmachine_thread_ack);
@@ -101,20 +109,15 @@ static int stop_machine(void)continue ; ret =kernel_thread (stopmachine, (void *)(long )i,CLONE_KERNEL );if (ret <0 ) -break ; + goto exit_threads; stopmachine_num_threads++; } -/* Wait for them all to come to life. */ +/* Wait for them all to come to life on the target. */ + stopmachine_state =STOPMACHINE_DEPLOY ;while (atomic_read (&stopmachine_thread_ack) != stopmachine_num_threads)yield (); -/* If some failed, kill them all. */ -if (ret <0 ) { -stopmachine_set_state (STOPMACHINE_EXIT ); -return ret; - } -/* Now they are all started, make them hold the CPUs, ready. */ preempt_disable ();stopmachine_set_state (STOPMACHINE_PREPARE );
@@ -125,6 +128,12 @@ static int stop_machine(void)stopmachine_set_state (STOPMACHINE_DISABLE_IRQ );return 0 ; + +exit_threads: +/* Wait for them all to exit, since stop is canceled */ +stopmachine_set_state (STOPMACHINE_EXIT ); + +return ret; }static void restart_machine (void )
Re: [PATCH 1/3] stop_machine: short exit path for if we cannot create enough threads
Quoted 2 lines · On Tuesday 29 April 2008 11:29:21 Hidetoshi Seto wrote:
stop_machine_run()invokes kthread 'kstopmachine' and the kthread creates its children for every other cpus.
Hi Hidetoshi,
I'm glad to see some work on stop_machine! I'd really like to rewrite it:
it has some nasty properties at the moment and introduces latency it doesn't have to.
Among the properties we want in a new stop_machine are:
1) timeout protection (as your patches),
2) arbitrary cpumask for what cpus to run function on,
3) ability to run something other than cpu_relax() on stopped cpus,
4) much lower latency for the common case,
5) simpler than the current code.
I expect to get to this in the next week or so, but please remind me if you don't see anything. I've applied your patches in the mean time (although #3 needed to be wrapped in #ifdef CONFIG_STOPMACHINE) in case I don't get that work finished for 2.6.27.
Thanks!
Rusty.
PATCH 2/3 stop_machine: add timeout for child thread deployment
In reply to Hidetoshi SetoIf stopmachine() invoked while one of onlined cpu is locked up by some reason, stopmachine cannot finish its work because the locked cpu cannot stop.
This patch allows stopmachine to return -EBUSY if any of kstopmachine's child threads cannot start running on its target cpu.
--- kernel/stop_machine.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 files changed, 39 insertions(+), 1 deletions(-)
kernel/stop_machine.c +39 -1
@@ -29,6 +29,9 @@static enum stopmachine_state stopmachine_state;static unsigned int stopmachine_num_threads;static atomic_t stopmachine_thread_ack; +static atomic_t stopmachine_busy_exit; + +static unsigned long stopmachine_timeout =5 ;/* secs, arbitrary */ static int stopmachine (void *cpu) {
@@ -42,6 +45,7 @@if (stopmachine_state ==STOPMACHINE_EXIT ) goto exit; +/* If target cpu is on fire, this call can stuck */ set_cpus_allowed_ptr (current, &cpumask_of_cpu ((int )(long )cpu));/* Ack: we arrived */
@@ -83,6 +87,12 @@if (prepared)preempt_enable (); +if (atomic_read (&stopmachine_busy_exit)) { +atomic_dec (&stopmachine_busy_exit); +printk (KERN_INFO "stopmachine: cpu#%d is not busy now.\n" , + (int )(long )cpu); + } +return 0 ; }
@@ -99,6 +109,15 @@static int stop_machine (void ) {int i, ret =0 ; +unsigned long limit; + +if (atomic_read (&stopmachine_busy_exit)) { +/* +* previous try was timeout, and still there is a unreachable +* cpu and abandoned child. +*/ +return -EBUSY ; + }atomic_set (&stopmachine_thread_ack,0 ); stopmachine_num_threads =0 ;
@@ -113,10 +132,15 @@ stopmachine_num_threads++; } + limit = jiffies +msecs_to_jiffies (stopmachine_timeout *MSEC_PER_SEC ); +/* Wait for them all to come to life on the target. */ stopmachine_state =STOPMACHINE_DEPLOY ;while (atomic_read (&stopmachine_thread_ack) != stopmachine_num_threads) -yield (); +if (time_is_after_jiffies (limit)) +yield (); +else + goto deploy_timeout;/* Now they are all started, make them hold the CPUs, ready. */ preempt_disable ();
@@ -129,6 +153,20 @@return 0 ; +deploy_timeout: +printk (KERN_CRIT "stopmachine: Failed to stop machine in time(%lds). " +"Are there any CPUs on file?\n" , stopmachine_timeout); + +/* defer exit check to the beginning of next try. */ +atomic_set (&stopmachine_busy_exit, stopmachine_num_threads); + +printk (KERN_INFO "stopmachine: cpu#%d is initiator of failed stop.\n" , +raw_smp_processor_id ()); +smp_wmb (); + stopmachine_state =STOPMACHINE_EXIT ; + +return -EBUSY ; + exit_threads:/* Wait for them all to exit, since stop is canceled */ stopmachine_set_state (STOPMACHINE_EXIT );
PATCH 3/3 stop_machine: add stopmachine_timeout sysctl entry
In reply to Hidetoshi SetoThere would be some system which cannot accept the arbitrary timeout value(= 5sec).
This patch allow us to change the value via sysctl.
--- kernel/stop_machine.c | 2 +- kernel/sysctl.c | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletions(-)
kernel/stop_machine.c +1 -1
@@ -31,7 +31,7 @@ static unsigned int stopmachine_num_threads;static atomic_t stopmachine_thread_ack;static atomic_t stopmachine_busy_exit; -static unsigned long stopmachine_timeout =5 ;/* secs, arbitrary */ +unsigned long stopmachine_timeout =5 ;/* secs, arbitrary */ static int stopmachine (void *cpu) {
kernel/sysctl.c +10 -0
@@ -80,6 +80,7 @@ extern int compat_log;extern int maps_protect;extern int sysctl_stat_interval;extern int latencytop_enabled; +extern unsigned long stopmachine_timeout;/* Constants used for minimum and maximum */ #if defined(CONFIG_DETECT_SOFTLOCKUP ) || defined(CONFIG_HIGHMEM )
@@ -809,6 +810,15 @@ static struct ctl_table kern_table[] = { .proc_handler = &proc_dostring, .strategy = &sysctl_string, }, + { + .ctl_name = CTL_UNNUMBERED, + .procname = "stopmachine_timeout", + .data = &stopmachine_timeout, + .maxlen = sizeof(unsigned long), + .mode = 0644, + .proc_handler = &proc_doulongvec_minmax, + .strategy = &sysctl_intvec, + },/* * NOTE: do not add new entries to this table unless you have read * Documentation/sysctl/ctl_unnumbered.txt
Quoted 1 line · Hidetoshi Seto
Are there any CPUs on file?
^^^^
Oops, it's a typo of "fire"...
Sorry,
H.Seto
Hi!
Quoted 23 lines
Hi Rusty and all,
This is a proposal of minor improvement for
kernel/stop_machine.c[PATCH 1/3]
stop_machine: short exit path for if we cannot create enough threads [PATCH 2/3]stop_machine: add timeout for child thread deployment [PATCH 3/3]stop_machine: addstopmachine_timeoutsysctl entryThe main topic is "how about adding timeout for
stop_machine?" I think it will act as a safety net.For example (of silly situation), system can hung with following way:
# ./silly.sh run an evil loop task on AP pid 6138's current affinity mask: ff pid 6138's new affinity mask: fe to pretend lock up, chrt -f -p 99 6138 loop[6138] is on CPU #4 to do stopmachine, try to off #7 echo 0 > /sys/devices/system/cpu/cpu7/online (never return)
Quoted 8 lines
After applying patch set here, it can be prevented.
# ./silly.sh : echo 0 > /sys/devices/system/cpu/cpu7/online stopmachine: Failed to stop machine in time(5s). Are there any CPUs on file? ./silly.sh: line 22: echo: write error: Device or resource busy offline is failed
I'd expect at least WARN_ON here. -EBUSY is not good enough indication that one of your cpus is now dead.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html