Re: Using handle_fasteoi_irq for pirqs

[Jeremy Fitzhardinge <jeremy@goop.org>]

 On 09/06/2010 05:58 PM, Jan Beulich wrote:
>  >>> On 03.09.10 at 20:46, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>> Using .startup/.shutdown for enable/disable seems very heavyweight.  Do
>> we really want to be rebinding the pirq each time?  Isn't unmask/masking
>> the event channel sufficient?
> Depends - the original (2.6.18) implementation only makes enable_pirq()
> a conditional startup (and really startup_pirq() is conditional too), while
> disable_pirq() does nothing at all. While forward porting, considering
> the contexts in which ->disable() gets called (namely note_interrupt())
> and after initially having had no ->enable()/->disable() methods at all
> (for default_enable() calling ->unmask() anyway, and default_disable()
> being a no-op as much as disable_pirq() was), I got to the conclusion
> that it might be better to do a full shutdown/startup, since it isn't
> known whether a disable is permanent or just temporary.
>
> Now part of the question whether this is actually a good choice is
> why default_disable() doesn't mask the affected IRQ - likely because
> IRQ_DISABLED is checked for and handled accordingly in all non-
> trivial flow handlers.
>
> The other aspect is that with the (original) switch to use
> handle_level_irq() we noticed at some point that the disabling of
> bad IRQs (where e.g. storms are noticed) didn't work anymore,
> due to that logic sitting in ->end(), which doesn't usually get
> called at all (nor does any other method except ->unmask() for
> the level case). Right now I don't really remember whether making
> ->disable() an alias of shutdown wasn't just a (failed iirc) attempt
> at getting Xen to know of the need to shut down such a bad IRQ.
> After the switch to fasteoi this logic should now be working again
> independent of what ->disable() does, so I will have to consider
> to un-alias disable_pirq() and shutdown_pirq() again.

At the moment, I'm using this:

static struct irq_chip xen_pirq_chip __read_mostly = {
	.name		= "xen-pirq",

	.startup	= startup_pirq,
	.shutdown	= shutdown_pirq,

	.enable		= pirq_eoi,
	.unmask		= unmask_irq,

	.disable	= mask_irq,
	.mask		= mask_irq,

	.eoi		= ack_pirq,
	.end		= end_pirq,

	.set_affinity	= set_affinity_irq,

	.retrigger	= retrigger_irq,
};


which seems to work OK now.  The "enable=pirq_eoi" is essentially the
same as "enable=startup_pirq", because your startup_pirq just does an
EOI if the evtchn is already valid (and EOI always ends up unmasking too).

ack_pirq and pirq_eoi are almost the same, except the former also does
the call to move_masked_irq().

>> At the moment my xen_evtchn_do_upcall() is masking and clearing the
>> event channel before calling into generic_handle_irq_desc(), which will
>> call handle_fasteoi_irq fairly directly.  That runs straight through and
>> the priq_chip's eoi just does an EOI on the pirq if Xen says it needs one.
>>
>> But apparently this isn't enough.  Is there anything else I should be doing?
> I can't see anything, and our kernel also doesn't.

Where's the source to your kernel?  The one I looked at most recently
was using handle_level_irq for everything.

But anyway, I found my bug; I'd overlooked where MSI interrupts were
being set up, and they were still using handle_edge_irq; changing them
to fasteoi seems to have done trick.

>> (I just implemented PHYSDEVOP_pirq_eoi_gmfn method of getting the pirq
>> eoi flags, but I haven't tested it yet.  I'm also not really sure what
>> the advantage of it is.)
> This is for you to avoid the EOI hypercall if it would be a no-op in
> Xen anyway.

Yes, but there's also PHYSDEVOP_irq_status_query call.  Does the "needs
EOI" actually change from moment to moment in a way where having a
shared page makes sense?

    J