Messages in this thread | | | Subject | Re: [PATCH RFC] stop_machine: make stop_machine safe and efficient to call early | From | Steven Rostedt <> | Date | Fri, 30 Sep 2011 17:21:51 -0400 |
| |
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); > > _ >
| |