This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


[Xen-devel] Re: Using handle_fasteoi_irq for pirqs

To: Jan Beulich <JBeulich@xxxxxxxxxx>
Subject: [Xen-devel] Re: Using handle_fasteoi_irq for pirqs
From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Date: Tue, 07 Sep 2010 11:53:43 +1000
Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, "Xen-devel@xxxxxxxxxxxxxxxxxxx" <Xen-devel@xxxxxxxxxxxxxxxxxxx>, Tom Kopec <tek@xxxxxxx>, Daniel Stodden <daniel.stodden@xxxxxxxxxx>
Delivery-date: Mon, 06 Sep 2010 18:54:27 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4C84BB580200007800014717@xxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4C743B2C.8070208@xxxxxxxx> <4C74E7C802000078000120C0@xxxxxxxxxxxxxxxxxx> <4C814278.5070904@xxxxxxxx> <4C84BB580200007800014717@xxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/20100806 Fedora/3.1.2-1.fc13 Lightning/1.0b2pre Thunderbird/3.1.2
 On 09/06/2010 05:58 PM, Jan Beulich wrote:
>  >>> On 03.09.10 at 20:46, Jeremy Fitzhardinge <jeremy@xxxxxxxx> 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?


Xen-devel mailing list