Skip to contentSkip to conversation

patches for stop_machine

PATCH In review Hidetoshi Seto 3 participants 7 messages Apr 29, 2008 – May 6, 2008 Proc sysctl

lore
Share & tools
lore
#0 lore raw

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

#1 lore raw

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
index 0101aee..5331ee5 100644
@@ -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)
#2 lore raw

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.

#3 lore raw

PATCH 2/3 stop_machine: add timeout for child thread deployment

In reply to 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.

---
 kernel/stop_machine.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 39 insertions(+), 1 deletions(-)
kernel/stop_machine.c +39 -1
Index: GIT-torvalds/kernel/stop_machine.c
===================================================================
@@ -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);
#4 lore raw

PATCH 3/3 stop_machine: add stopmachine_timeout sysctl entry

In reply to 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.

---
 kernel/stop_machine.c |    2 +-
 kernel/sysctl.c       |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)
kernel/stop_machine.c +1 -1
index cd2ba88..cc6c8ec 100644
@@ -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
index fd33648..fe489a7 100644
@@ -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
#6 lore raw
In reply to Hidetoshi 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: 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)
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