Re: [PATCH] ia64: prevent irq migration race in __cpu_disable path

Alex Chiang
2009-02-06T18:43:20Z

* Paul E. McKenney <paulmck@linux.vnet.ibm.com>:
> On Fri, Feb 06, 2009 at 11:07:42AM -0700, Alex Chiang wrote:
> >
> > [removing stable@kernel.org for now while we figure this out]
> >
> > * Paul E. McKenney <paulmck@linux.vnet.ibm.com>:
> > > On Fri, Feb 06, 2009 at 09:22:13AM -0700, Alex Chiang wrote:
> > > > ---
> > > > In my opinion, this is .29 material.
> > > >
> > > > Sorry for the huge changelog:patch ratio, but this area is tricky
> > > > enough that more explanation is better than less, I think.
> > > >
> > > > Also, I'm still a little troubled by Paul's original patch. What
> > > > happens if we're trying to offline the CPEI target? The code in
> > > > migrate_platform_irqs() uses cpu_online_map to select the new
> > > > CPEI target, and it seems like we can end up in the same
> > > > situation as the problem I'm trying to fix now.
> > > >
> > > > Paul?
> > > >
> > > > My patch has held up for over 24 hours of stress testing, where
> > > > we put the system under a heavy load and then randomly
> > > > offline/online CPUs every 2 seconds. Without this patch, the
> > > > machine would crash reliably within 15 minutes.
> > >
> > > I don't claim much expertise on IA64 low-level architectural details,
> >
> > I'm starting to get a bit out of my depth here too... :-/
> >
> > > so am reduced to asking the usual question... Does this patch guarantee
> > > that a given CPU won't be executing irq handlers while marked offline?
> > > If there is no such guarantee, things can break. (See below.)
> >
> > My patch makes no guarantee. What it does do is prevent a NULL
> > deref while we are, in fact, executing an irq handler while
> > marked offline.
> >
> > > In any case, apologies for failing to correctly fix the original
> > > problem!!!
> >
> > I'm curious, reading through your old change log:
> >
> > Make ia64 refrain from clearing a given to-be-offlined CPU's
> > bit in the cpu_online_mask until it has processed pending
> > irqs. This change prevents other CPUs from being blindsided
> > by an apparently offline CPU nevertheless changing globally
> > visible state.
> >
> > Was your patch fixing a theoretical problem or a real bug? What
> > globally visible state were you referencing there?
> >
> > > > ---
> > > >
> > > > diff --git a/arch/ia64/kernel/smpboot.c b/arch/ia64/kernel/smpboot.c
> > > > index 1146399..2a17d1c 100644
> > > > --- a/arch/ia64/kernel/smpboot.c
> > > > +++ b/arch/ia64/kernel/smpboot.c
> > > > @@ -742,8 +742,8 @@ int __cpu_disable(void)
> > > > }
> > > >
> > > > remove_siblinginfo(cpu);
> > > > - fixup_irqs();
> > > > cpu_clear(cpu, cpu_online_map);
> > > > + fixup_irqs();
> > >
> > > So you argument is that because we are running in the context of
> > > stop_machine(), even though fixup_irqs() does in fact cause irq handlers
> > > to run on the current CPU which is marked offline, the fact that there
> > > is no one running to notice this misbehavior makes it OK? (Which
> > > perhaps it is, just asking the question.)
> >
> > I wouldn't say that I have a solid argument, per se, just fixing
> > symptoms. ;)
> >
> > My reading of the cpu_down() path makes it seem like we need to
> > process pending interrupts on the current CPU, and the original
> > author certainly thought it was ok to call an irq handler on the
> > current CPU. We don't disable local irqs until the very last step
> > of fixup_irqs().
> >
> > So the actual design of this path assumed it was ok to call an
> > irq handler on a marked-offline CPU.
> >
> > Can you educate me on the danger of doing such a thing? That
> > might help in how I interpret the code.
>
> Well, RCU happily ignores CPUs that don't have their bits set in
> cpu_online_map, so if there are RCU read-side critical sections in the
> irq handlers being run, RCU will ignore them. If the other CPUs were
> running, they might sequence through the RCU state machine, which could
> result in data structures being yanked out from under those irq handlers,
> which in turn could result in oopses or worse.

Ok, that makes sense.

As I'm continuing to dig, I took a look at the x86 side of the
house and they have this interesting sequence:

cpu_disable_common()
remove_cpu_from_maps() /* remove cpu from online map */
fixup_irqs()
[break irq-CPU affinity]

local_irq_enable();
mdelay(1);
local_irq_disable();

So in x86, we allow interrupt handlers to run on a CPU that's
already been removed from the online map.

Does that seem like an analagous situation to what we have in
ia64?

Thanks.

/ac, still digging