RE: [Xen-devel] RE: [PATCH v2 2/2] x86: don't unmask disabled irqs when migrating them

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Tian, Kevin
> Sent: Friday, May 06, 2011 8:55 PM
> 
> > From: Thomas Gleixner
> > Sent: Friday, May 06, 2011 6:00 PM
> >
> > On Fri, 6 May 2011, Tian, Kevin wrote:
> > > x86: don't unmask disabled irqs when migrating them
> > >
> > > it doesn't make sense to mask/unmask a disabled irq when migrating
> > > it from offlined cpu to another, because it's not expected to handle
> > > any instance of it. Current mask/set_affinity/unmask steps may
> > > trigger unexpected instance on disabled irq which then simply bug on
> > > when there is no handler for it. One failing example is observed in Xen.
> > > Xen pvops
> >
> > So there is no handler, why the heck is there an irq action?
> >
> >       if (!irq_has_action(irq) ....
> >             continue;
> >
> > Should have caught an uninitialized interrupt. If Xen abuses
> > interrupts that way, then it rightfully explodes. And we do not fix it by 
> > magic
> somewhere else.
> 
> sorry that my bad description here. there does be a dummy handler registered
> on such irqs which simply throws out a BUG_ON when hit. I should just say such
> injection is not expected instead of no handler. :-)
> 
> >
> > > guest marks a special type of irqs as disabled, which are simply
> > > used
> >
> > As I explained before several times, IRQF_DISABLED has absolutely
> > nothing to do with it and pvops _CANNOT_ mark an interrupt disabled.
> 
> I have to admit that I need more study about whole interrupt sub-system, to
> better understand your explanation here. Also here again my description is not
> accurate enough. I meant that Xen pvops request the special irq with below
> flags:
>       IRQF_DISABLED|IRQF_PERCPU|IRQF_NOBALANCING
> and then later explicitly disable it with disable_irq(). As you said that
> IRQF_DISABLED itself has nothing to do with it, and it's the later 
> disable_irq()
> which takes real effect because Xen event chip hooks this callback to mask the
> irq from the chip level.
> 
> >
> > >
> > >           chip = irq_data_get_irq_chip(data);
> > > -         if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
> > > +         do_mask = !irqd_irq_disabled(data) &&
> > > +                 !irqd_can_move_in_process_context(data) &&
> chip->irq_mask;
> > > +         if (do_mask)
> > >                   chip->irq_mask(data);
> >
> > This is completely wrong. irqd_irq_disabled() is a status information
> > which does not tell you whether the interrupt is actually masked at
> > the hardware level because we do lazy interrupt hardware masking. So
> > your change would keep the line unmasked at the hardware level for all
> > interrupts which are in the lazy disabled state.
> 
> Got it.
> 
> >
> > The only conditional which is interesting is the unmask path and
> > that's a simple optimization and not a correctness problem.
> >
> 
> So what's your suggestion based on my updated information? Is there any
> interface I may take to differentiate above exception with normal case?
> Basically in Xen usage we want such irqs permanently disabled at the chip 
> level.
> Or could we only do mask/unmask for irqs which are unmasked atm if as you
> said it's just an optimization step? :-)
> 

After another thought, I just got myself messed. This is not required by Xen
actually, as the 1st patch to handle IRQF_PERCPU has been enough to cover
the special requirement. Could you ack the 1st one if you agree?

This 2nd patch is cooked upon the question whether generally a irq disabled at 
chip level should be unmasked at migration. If there's no action registered,
it won't be migrated. The only question is if there does be action registered,
and this irq is disabled at chip level, will there be any issue to force unmask 
it 
at migration, or should we only mask/unmask for a irq which is unmasked at
the migration?

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel