* [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early
@ 2011-09-30 16:28 Jeremy Fitzhardinge
2011-09-30 16:34 ` Steven Rostedt
2011-09-30 23:06 ` Tejun Heo
0 siblings, 2 replies; 9+ messages in thread
From: Jeremy Fitzhardinge @ 2011-09-30 16:28 UTC (permalink / raw)
To: Tejun Heo
Cc: Rusty Russell, Peter Zijlstra, Andrew Morton, Ingo Molnar,
Steven Rostedt, Linux Kernel Mailing List, H. Peter Anvin
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);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early
2011-09-30 16:28 [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early Jeremy Fitzhardinge
@ 2011-09-30 16:34 ` Steven Rostedt
2011-09-30 17:00 ` Jeremy Fitzhardinge
2011-09-30 21:23 ` Andrew Morton
2011-09-30 23:06 ` Tejun Heo
1 sibling, 2 replies; 9+ messages in thread
From: Steven Rostedt @ 2011-09-30 16:34 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Tejun Heo, Rusty Russell, Peter Zijlstra, Andrew Morton,
Ingo Molnar, Linux Kernel Mailing List, H. Peter Anvin
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);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early
2011-09-30 16:34 ` Steven Rostedt
@ 2011-09-30 17:00 ` Jeremy Fitzhardinge
2011-09-30 21:06 ` Andrew Morton
2011-09-30 21:23 ` Andrew Morton
1 sibling, 1 reply; 9+ messages in thread
From: Jeremy Fitzhardinge @ 2011-09-30 17:00 UTC (permalink / raw)
To: Steven Rostedt
Cc: Tejun Heo, Rusty Russell, Peter Zijlstra, Andrew Morton,
Ingo Molnar, Linux Kernel Mailing List, H. Peter Anvin
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);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early
2011-09-30 17:00 ` Jeremy Fitzhardinge
@ 2011-09-30 21:06 ` Andrew Morton
2011-09-30 21:21 ` Steven Rostedt
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2011-09-30 21:06 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Steven Rostedt, Rusty Russell, Peter Zijlstra,
Linux Kernel Mailing List, H. Peter Anvin, Tejun Heo,
Ingo Molnar
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);
_
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early
2011-09-30 21:06 ` Andrew Morton
@ 2011-09-30 21:21 ` Steven Rostedt
0 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2011-09-30 21:21 UTC (permalink / raw)
To: Andrew Morton
Cc: Jeremy Fitzhardinge, Rusty Russell, Peter Zijlstra,
Linux Kernel Mailing List, H. Peter Anvin, Tejun Heo,
Ingo Molnar
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);
>
> _
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early
2011-09-30 16:34 ` Steven Rostedt
2011-09-30 17:00 ` Jeremy Fitzhardinge
@ 2011-09-30 21:23 ` Andrew Morton
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Morton @ 2011-09-30 21:23 UTC (permalink / raw)
To: Steven Rostedt
Cc: Jeremy Fitzhardinge, Rusty Russell, Peter Zijlstra,
Linux Kernel Mailing List, H. Peter Anvin, Tejun Heo,
Ingo Molnar
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early
2011-09-30 16:28 [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early Jeremy Fitzhardinge
2011-09-30 16:34 ` Steven Rostedt
@ 2011-09-30 23:06 ` Tejun Heo
2011-09-30 23:32 ` Jeremy Fitzhardinge
1 sibling, 1 reply; 9+ messages in thread
From: Tejun Heo @ 2011-09-30 23:06 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Rusty Russell, Peter Zijlstra, Andrew Morton, Ingo Molnar,
Steven Rostedt, Linux Kernel Mailing List, H. Peter Anvin
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early
2011-09-30 23:06 ` Tejun Heo
@ 2011-09-30 23:32 ` Jeremy Fitzhardinge
2011-09-30 23:39 ` Tejun Heo
0 siblings, 1 reply; 9+ messages in thread
From: Jeremy Fitzhardinge @ 2011-09-30 23:32 UTC (permalink / raw)
To: Tejun Heo
Cc: Rusty Russell, Peter Zijlstra, Andrew Morton, Ingo Molnar,
Steven Rostedt, Linux Kernel Mailing List, H. Peter Anvin
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.
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early
2011-09-30 23:32 ` Jeremy Fitzhardinge
@ 2011-09-30 23:39 ` Tejun Heo
0 siblings, 0 replies; 9+ messages in thread
From: Tejun Heo @ 2011-09-30 23:39 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Rusty Russell, Peter Zijlstra, Andrew Morton, Ingo Molnar,
Steven Rostedt, Linux Kernel Mailing List, H. Peter Anvin
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-09-30 23:39 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-30 16:28 [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early Jeremy Fitzhardinge
2011-09-30 16:34 ` Steven Rostedt
2011-09-30 17:00 ` Jeremy Fitzhardinge
2011-09-30 21:06 ` Andrew Morton
2011-09-30 21:21 ` Steven Rostedt
2011-09-30 21:23 ` Andrew Morton
2011-09-30 23:06 ` Tejun Heo
2011-09-30 23:32 ` Jeremy Fitzhardinge
2011-09-30 23:39 ` Tejun Heo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.