[PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[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 <jeremy.fitzhardinge@citrix.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index ba5070c..2df15ca 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -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);

Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[Steven Rostedt]

On Fri, 2011-09-30 at 09:28 -0700, Jeremy Fitzhardinge wrote:
> 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 <jeremy.fitzhardinge@citrix.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: H. Peter Anvin <hpa@linux.intel.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> index ba5070c..2df15ca 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -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

> +
>  	/* 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);
> 

Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[Jeremy Fitzhardinge]

On 09/30/2011 09:34 AM, Steven Rostedt wrote:
> 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.

    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.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index ba5070c..a45b36d 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -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);

Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[Andrew Morton]

On Fri, 30 Sep 2011 10:00:06 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> On 09/30/2011 09:34 AM, Steven Rostedt wrote:
> > 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.
> 
>     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?

>
> ...
>
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -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;
> +	}

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.


--- a/kernel/stop_machine.c~stop_machine-make-stop_machine-safe-and-efficient-to-call-early-fix
+++ a/kernel/stop_machine.c
@@ -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);
 
_

Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[Steven Rostedt]

On Fri, 2011-09-30 at 14:06 -0700, Andrew Morton wrote:
> On Fri, 30 Sep 2011 10:00:06 -0700
> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> 
> > On 09/30/2011 09:34 AM, Steven Rostedt wrote:
> > > 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.
> > 
> >     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

> 
> >
> > ...
> >
> > --- a/kernel/stop_machine.c
> > +++ b/kernel/stop_machine.c
> > @@ -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;
> > +	}
> 
> 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.
> 
> 
> --- a/kernel/stop_machine.c~stop_machine-make-stop_machine-safe-and-efficient-to-call-early-fix
> +++ a/kernel/stop_machine.c
> @@ -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);

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


>  		ret = (*fn)(data);
>  		local_irq_restore(flags);
>  
> _
> 

Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[Andrew Morton]

On Fri, 30 Sep 2011 12:34:54 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 2011-09-30 at 09:28 -0700, Jeremy Fitzhardinge wrote:
> > 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 <jeremy.fitzhardinge@citrix.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: H. Peter Anvin <hpa@linux.intel.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > 
> > diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> > index ba5070c..2df15ca 100644
> > --- a/kernel/stop_machine.c
> > +++ b/kernel/stop_machine.c
> > @@ -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.
> 

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

Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[Tejun Heo]

Hello, Jeremy.

On Fri, Sep 30, 2011 at 09:28:15AM -0700, Jeremy Fitzhardinge wrote:
> 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.
...
> @@ -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.

-- 
tejun

Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[Jeremy Fitzhardinge]

On 09/30/2011 04:06 PM, Tejun Heo wrote:
> Hello, Jeremy.
>
> On Fri, Sep 30, 2011 at 09:28:15AM -0700, Jeremy Fitzhardinge wrote:
>> 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.
> ...
>> @@ -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.

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?

    J

diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index f5855fe3..70b3be4 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -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.

Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early

[Tejun Heo]

Hello,

On Fri, Sep 30, 2011 at 04:32:40PM -0700, Jeremy Fitzhardinge wrote:
> > 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.

> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> index f5855fe3..70b3be4 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -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) {

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

Thank you.

-- 
tejun