stop_machine: make stop_machine safe and efficient to call early

RFC PATCH In review Jeremy Fitzhardinge 4 participants 9 messages Sep 30, 2011

Share & tools

Jeremy Fitzhardinge

author

2011-09-30 16:28 UTC #0 lore raw

RFC PATCH 1/1 stop_machine: make stop_machine safe and efficient to call early

Make stop_machine() safe to call early in boot, before SMP has been set up, by simply calling the callback function directly if there's only one CPU online.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Rusty Russell <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Andrew Morton <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Steven Rostedt <[email protected]>

kernel/stop_machine.c +3 -0

index ba5070c..2df15ca 100644
@@ -485,6 +485,9 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 
+	if (smdata.num_threads == 1)
+		return (*fn)(data);
+
 	/* Set the initial state and stop all online cpus. */
 	set_state(&smdata, STOPMACHINE_PREPARE);
 	return stop_cpus(cpu_online_mask, stop_machine_cpu_stop, &smdata);

Steven Rostedt

2011-09-30 16:34 UTC #1 lore raw

Quoted 18 lines · Jeremy Fitzhardinge

Make stop_machine() safe to call early in boot, before SMP has been set up, by simply calling the callback function directly if there's only one CPU online.
Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Rusty Russell <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Andrew Morton <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Steven Rostedt <[email protected]>
kernel/stop_machine.c +0 -0
index ba5070c..2df15ca 100644

kernel/stop_machine.c int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus) · line 489

Quoted 5 lines

kernel/stop_machine.c +2 -0

@@ -485,6 +485,9 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 
+	if (smdata.num_threads == 1)
+		return (*fn)(data);

Doesn't interrupts need to be disabled here too? As stop machine functions also guarantee that they will not be interrupted by interrupts.

-- Steve

Quoted 5 lines

kernel/stop_machine.c +1 -0

+
 	/* Set the initial state and stop all online cpus. */
 	set_state(&smdata, STOPMACHINE_PREPARE);
 	return stop_cpus(cpu_online_mask, stop_machine_cpu_stop, &smdata);

Jeremy Fitzhardinge

author

2011-09-30 17:00 UTC #2 lore raw

Quoted 3 lines · Steven Rostedt

Doesn't interrupts need to be disabled here too? As stop machine functions also guarantee that they will not be interrupted by interrupts. -- Steve

Good point.

Make stop_machine() safe to call early in boot, before SMP has been set up, by simply calling the callback function directly if there's only one CPU online.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Rusty Russell <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Andrew Morton <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Steven Rostedt <[email protected]>

kernel/stop_machine.c +11 -0

index ba5070c..a45b36d 100644
@@ -485,6 +485,17 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 
+	if (smdata.num_threads == 1) {
+		unsigned long flags;
+		int ret;
+
+		local_save_flags(flags);
+		ret = (*fn)(data);
+		local_irq_restore(flags);
+
+		return ret;
+	}
+
 	/* Set the initial state and stop all online cpus. */
 	set_state(&smdata, STOPMACHINE_PREPARE);
 	return stop_cpus(cpu_online_mask, stop_machine_cpu_stop, &smdata);

Andrew Morton

2011-09-30 21:06 UTC #3 lore raw

Quoted 5 lines · On Fri, 30 Sep 2011 10:00:06 -0700 Jeremy Fitzhardinge <[email protected]> wrote:

Quoted 3 lines · Steven Rostedt
Doesn't interrupts need to be disabled here too? As stop machine functions also guarantee that they will not be interrupted by interrupts. -- Steve

Quoted 7 lines

Good point.
J
Make stop_machine() safe to call early in boot, before SMP has been set up, by simply calling the callback function directly if there's only one CPU online.

Being a trusting soul, I shall assume that you presently have or soon will have some code which requires this change?

Quoted 9 lines

...

kernel/stop_machine.c +0 -0

@@ -485,6 +485,17 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };

kernel/stop_machine.c int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus) · line 497

Quoted 10 lines

kernel/stop_machine.c +10 -0

+	if (smdata.num_threads == 1) {
+		unsigned long flags;
+		int ret;
+
+		local_save_flags(flags);
+		ret = (*fn)(data);
+		local_irq_restore(flags);
+
+		return ret;
+	}

It is quite unobvious to readers why this code exists. Therefore, we should tell them.

Also, bug. local_save_flags() is used to read the irq flags - it does not actually disable interrupts. We want local_irq_save() here. None of these interfaces are documented and their naming system is crappy. Cause, effect.

kernel/stop_machine.c renamed from kernel/stop_machine.c~stop_machine-make-stop_machine-safe-and-efficient-to-call-early-fix +5 -1

@@ -486,10 +486,14 @@ int __stop_machine(int (*fn)(void *), vo
 					    .active_cpus = cpus };
 
 	if (smdata.num_threads == 1) {
+		/*
+		 * Handle the case where stop_machine() is called early in boot
+		 * before SMP startup.
+		 */
 		unsigned long flags;
 		int ret;
 
-		local_save_flags(flags);
+		local_irq_save(flags);
 		ret = (*fn)(data);
 		local_irq_restore(flags);

Steven Rostedt

2011-09-30 21:21 UTC #4 lore raw

Quoted 10 lines · Andrew Morton

Quoted 7 lines · On Fri, 30 Sep 2011 10:00:06 -0700 Jeremy Fitzhardinge <[email protected]> wrote:
Quoted 3 lines · Steven Rostedt
Doesn't interrupts need to be disabled here too? As stop machine functions also guarantee that they will not be interrupted by interrupts. -- Steve
Good point.

Quoted 8 lines

Quoted 5 lines
J
Make stop_machine() safe to call early in boot, before SMP has been set up, by simply calling the callback function directly if there's only one CPU online.
Being a trusting soul, I shall assume that you presently have or soon will have some code which requires this change?

Yeah, it has to do with these patches:

https://lkml.org/lkml/2011/9/29/509

Quoted 35 lines

Quoted 10 lines
...
kernel/stop_machine.c +0 -0
@@ -485,6 +485,17 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 
kernel/stop_machine.c int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus) · line 497
Quoted 10 lines
kernel/stop_machine.c +10 -0
+	if (smdata.num_threads == 1) {
+		unsigned long flags;
+		int ret;
+
+		local_save_flags(flags);
+		ret = (*fn)(data);
+		local_irq_restore(flags);
+
+		return ret;
+	}
It is quite unobvious to readers why this code exists. Therefore, we should tell them.
Also, bug. local_save_flags() is used to read the irq flags - it does not actually disable interrupts. We want local_irq_save() here. None of these interfaces are documented and their naming system is crappy. Cause, effect.
kernel/stop_machine.c renamed from kernel/stop_machine.c~stop_machine-make-stop_machine-safe-and-efficient-to-call-early-fix +0 -0
@@ -486,10 +486,14 @@ int __stop_machine(int (*fn)(void *), vo
 					    .active_cpus = cpus };
 

kernel/stop_machine.c int __stop_machine(int (*fn)(void *), vo · line 496

Quoted 10 lines

kernel/stop_machine.c renamed from kernel/stop_machine.c~stop_machine-make-stop_machine-safe-and-efficient-to-call-early-fix +5 -1

 	if (smdata.num_threads == 1) {
+		/*
+		 * Handle the case where stop_machine() is called early in boot
+		 * before SMP startup.
+		 */
 		unsigned long flags;
 		int ret;
 
-		local_save_flags(flags);
+		local_irq_save(flags);

Nice catch. Yeah, the naming convention is not too nice. They probably should have been named: local_irq_disable_save() and local_irq_enable_restore(). But that's too much to type ;)

-- Steve

Quoted 5 lines

kernel/stop_machine.c renamed from kernel/stop_machine.c~stop_machine-make-stop_machine-safe-and-efficient-to-call-early-fix +0 -0
 		ret = (*fn)(data);
 		local_irq_restore(flags);
 
_

Andrew Morton

2011-09-30 21:23 UTC #5 lore raw

In reply to Steven Rostedt

Quoted 22 lines · On Fri, 30 Sep 2011 12:34:54 -0400 Steven Rostedt <[email protected]> wrote:

Quoted 21 lines · Jeremy Fitzhardinge
Make stop_machine() safe to call early in boot, before SMP has been set up, by simply calling the callback function directly if there's only one CPU online.
Signed-off-by Jeremy Fitzhardinge <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Rusty Russell <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Andrew Morton <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Steven Rostedt <[email protected]>
kernel/stop_machine.c +0 -0
index ba5070c..2df15ca 100644
@@ -485,6 +485,9 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 

Quoted 7 lines

kernel/stop_machine.c int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus) · line 489
Quoted 2 lines
kernel/stop_machine.c +2 -0
+	if (smdata.num_threads == 1)
+		return (*fn)(data);
Doesn't interrupts need to be disabled here too? As stop machine functions also guarantee that they will not be interrupted by interrupts.

If we wish to truly emulate the stop_machine_cpu_stop() callback environment then we should run hard_irq_disable() as well?

Tejun Heo

2011-09-30 23:06 UTC #6 lore raw

In reply to Jeremy Fitzhardinge

Hello, Jeremy.

Quoted 3 lines · Jeremy Fitzhardinge

Make stop_machine() safe to call early in boot, before SMP has been set up, by simply calling the callback function directly if there's only one CPU online.

...

kernel/stop_machine.c int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus) · line 490

Quoted 7 lines

kernel/stop_machine.c +3 -0

@@ -485,6 +485,9 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 
+	if (smdata.num_threads == 1)
+		return (*fn)(data);
+

As others have pointed out, you'll need to call both local and hardirq disables. Also, I think the description and the code are a bit misleading. How aobut setting cpu_stop_initialized in cpu_stop_init() and testing it from __stop_machine() instead? I think it would be better to keep the behavior as uniform as possible once things are up and running.

Thank you.

Signature · 1 line

tejun

Jeremy Fitzhardinge

author

2011-09-30 23:32 UTC #7 lore raw

Quoted 11 lines · Tejun Heo

Hello, Jeremy.
Quoted 3 lines · Jeremy Fitzhardinge
Make stop_machine() safe to call early in boot, before SMP has been set up, by simply calling the callback function directly if there's only one CPU online.
...
Quoted 4 lines
(quoted diff) +0 -0
@@ -485,6 +485,9 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 

Quoted 9 lines

Review int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus) · line 490
Quoted 3 lines
(quoted diff) +3 -0
+	if (smdata.num_threads == 1)
+		return (*fn)(data);
+
As others have pointed out, you'll need to call both local and hardirq disables. Also, I think the description and the code are a bit misleading. How aobut setting cpu_stop_initialized in cpu_stop_init() and testing it from __stop_machine() instead? I think it would be better to keep the behavior as uniform as possible once things are up and running.

Yes, I was wondering about that. Do you think the patch (with irq fixes in place) would affect the behaviour of an SMP kernel running UP?

kernel/stop_machine.c +4 -1

index f5855fe3..70b3be4 100644
@@ -41,6 +41,7 @@ struct cpu_stopper {
 };
 
 static DEFINE_PER_CPU(struct cpu_stopper, cpu_stopper);
+static bool stop_machine_initialized = false;
 
 static void cpu_stop_init_done(struct cpu_stop_done *done, unsigned int nr_todo)
 {

@@ -386,6 +387,8 @@ static int __init cpu_stop_init(void)
 	cpu_stop_cpu_callback(&cpu_stop_cpu_notifier, CPU_ONLINE, bcpu);
 	register_cpu_notifier(&cpu_stop_cpu_notifier);
 
+	stop_machine_initialized = true;
+
 	return 0;
 }
 early_initcall(cpu_stop_init);

@@ -485,7 +488,7 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 
-	if (smdata.num_threads == 1) {
+	if (!stop_machine_initialized) {
 		/*
 		 * Handle the case where stop_machine() is called early in boot
 		 * before SMP startup.

Tejun Heo

2011-09-30 23:39 UTC #8 lore raw

Hello,

Quoted 9 lines · Jeremy Fitzhardinge

Quoted 6 lines
As others have pointed out, you'll need to call both local and hardirq disables. Also, I think the description and the code are a bit misleading. How aobut setting cpu_stop_initialized in cpu_stop_init() and testing it from __stop_machine() instead? I think it would be better to keep the behavior as uniform as possible once things are up and running.
Yes, I was wondering about that. Do you think the patch (with irq fixes in place) would affect the behaviour of an SMP kernel running UP?

I can't think of anything which could go wrong from the top of my head but there's no reason to risk subtle issues when the alternative isn't difficult.

Quoted 22 lines

kernel/stop_machine.c +3 -0

index f5855fe3..70b3be4 100644
@@ -41,6 +41,7 @@ struct cpu_stopper {
 };
 
 static DEFINE_PER_CPU(struct cpu_stopper, cpu_stopper);
+static bool stop_machine_initialized = false;
 
 static void cpu_stop_init_done(struct cpu_stop_done *done, unsigned int nr_todo)
 {

@@ -386,6 +387,8 @@ static int __init cpu_stop_init(void)
 	cpu_stop_cpu_callback(&cpu_stop_cpu_notifier, CPU_ONLINE, bcpu);
 	register_cpu_notifier(&cpu_stop_cpu_notifier);
 
+	stop_machine_initialized = true;
+
 	return 0;
 }
 early_initcall(cpu_stop_init);

kernel/stop_machine.c int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus) · line 491

Quoted 5 lines

kernel/stop_machine.c +1 -1

@@ -485,7 +488,7 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 					    .num_threads = num_online_cpus(),
 					    .active_cpus = cpus };
 
-	if (smdata.num_threads == 1) {
+	if (!stop_machine_initialized) {

Yeah, this looks good to me. You might want to add WARN_ON_ONCE(smdata.num_threads != 1) inside if {} tho.

Thank you.

Signature · 1 line

tejun