[PATCH 0/3] patches for stop_machine

[PATCH 0/3] patches for stop_machine

[Hidetoshi Seto]

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

[Hidetoshi Seto]

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.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 kernel/stop_machine.c |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 0101aee..5331ee5 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -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

[Rusty Russell]

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

[Hidetoshi Seto]

If 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.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 kernel/stop_machine.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 39 insertions(+), 1 deletions(-)

Index: GIT-torvalds/kernel/stop_machine.c
===================================================================
--- GIT-torvalds.orig/kernel/stop_machine.c	2008-04-29 00:29:20.000000000 +0900
+++ GIT-torvalds/kernel/stop_machine.c	2008-04-29 00:31:55.000000000 +0900
@@ -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

[Hidetoshi Seto]

There would be some system which cannot accept the arbitrary
timeout value(= 5sec).

This patch allow us to change the value via sysctl.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 kernel/stop_machine.c |    2 +-
 kernel/sysctl.c       |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index cd2ba88..cc6c8ec 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -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)
 {
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index fd33648..fe489a7 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -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

Re: [PATCH 0/3] patches for stop_machine

[Hidetoshi Seto]

Hidetoshi Seto wrote:
>  Are there any CPUs on file?
                         ^^^^
Oops, it's a typo of "fire"...

Sorry,
H.Seto

Re: [PATCH 0/3] patches for stop_machine

[Pavel Machek]

Hi!

> 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

I'd expect at least WARN_ON here. -EBUSY is not good enough indication
that one of your cpus is now dead.

							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html