Re: [Xen-devel] [GIT PULL] Fix lost interrupt race in Xen event channels

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

 On 08/25/2010 12:52 AM, Jan Beulich wrote:
>  >>> On 24.08.10 at 23:35, Jeremy Fitzhardinge <jeremy@xxxxxxxx> wrote:
>> We worked out the root cause was that it was incorrectly treating Xen
>> events as level rather than edge triggered interrupts, which works fine
>> unless you're handling one interrupt, the interrupt gets migrated to
>> another cpu and then re-raised.  This ends up losing the interrupt
>> because the edge-triggering of the second interrupt is lost.
> While this description would seem plausible at the first glance, it
> doesn't match up with unmask_evtchn() already taking care of
> exactly this case. Or are you implicitly saying that this code is
> broken in some way (if so, how, and shouldn't it then be that
> code that needs fixing, or removing if you want to stay with the
> edge handling)?
>
> I do however agree that using handle_level_irq() is problematic
> (see 
> http://lists.xensource.com/archives/html/xen-devel/2010-04/msg01178.html),
> but as said there I think using the fasteoi logic is preferable. No
> matter whether using edge or level, the ->end() method will
> never be called (whereas fasteoi calls ->eoi(), which would
> just need to be vectored to the same function as ->end()).
> Without end_pirq() ever called, you can't let Xen know of
> bad PIRQs (so that it can disable them instead of continuing
> to call the [now shortcut] handler in the owning domain).

Note that this patch is specifically for upstream Xen, which doesn't
have any pirq support in it at present.

However,  I did consider using fasteoi, but I couldn't see how to make
it work.  The problem is that it only does a single call into the
irq_chip for EOI after calling the interrupt handler, but there is no
call beforehand to ack the interrupt (which means clear the event flag
in our case).  This leads to a race where an event can be lost after the
interrupt handler has returned, but before the event flag has been
cleared (because Xen won't set pending or call the upcall function if
the event is already set).  I guess I could pre-clear the event in the
upcall function, but I'm not sure that's any better.

In the dom0 kernels, I followed the example of the IOAPIC irq_chip
implementation, which does the hardware EOI in the ack call at the start
of handle_edge_irq, can did the EOI hypercall (when necessary) there.  I
welcome a reviewer's eye on this though.

>> The other change changes IPI and VIRQ event sources to use
>> handle_percpu_irq, because treating them as level is also wrong, and
>> they're actually inherently percpu events, much like LAPIC vectors.
> This doesn't seem right for the general VIRQ case: global ones
> should not be disallowed migration between CPUs. Since in your
> model the requestor has to pass IRQF_PERCPU anyway,
> shouldn't you make the selection of the handler dependent
> upon this flag?

I was thinking specifically of the timer, debug and console virqs.  The
last is the only one which could conceivably be non-percpu, but in
practice I think it would be a bad idea to put it on anything other than
cpu0.  What other global virqs are there?  Nothing that's high-frequency
enough to be worth migrating?

But yes, if necessary, I could make it depend on the IRQF_PERCPU flag.

    J

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