Regression after "Remove support for reporting coalesced APIC IRQs"
Hi Jan,
I bisected [1] to f1ed0450a5fac7067590317cbf027f566b6ccbca. Fortunately further investigation showed that it is not really related to removing APIC timer interrupt reinjection and the real problem is that we cannot assume that __apic_accept_irq() always injects interrupts like the patch does because the function skips interrupt injection if APIC is disabled. This misreporting screws RTC interrupt tracking, so further RTC interrupt are stopped to be injected. The simplest solution that I see is to revert most of the commit and only leave APIC timer interrupt reinjection.
If you have more elegant solution let me know.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=58931
--
Gleb.
Quoted 8 lines
-----Original Message-----
From: Gleb Natapov [mailto:[email protected]]
Sent: Thursday, June 06, 2013 4:54 PM
To: Jan Kiszka
Cc: [email protected]; Ren, Yongjie
Subject: Regression after "Remove support for reporting coalesced APIC IRQs"
Quoted 4 lines
Hi Jan,
I bisected [1] to
f1ed0450a5fac7067590317cbf027f566b6ccbca.
Right. Before Gleb's mail, I also just did the bisection for this bug. The first bad commit is Jan's f1ed0450.
Quoted 14 lines
Fortunately
further investigation showed that it is not really related to removing APIC timer interrupt reinjection and the real problem is that we cannot assume that__apic_accept_irq()always injects interrupts like the patch does because the function skips interrupt injection if APIC is disabled. This misreporting screws RTC interrupt tracking, so further RTC interrupt are stopped to be injected. The simplest solution that I see is to revert most of the commit and only leave APIC timer interrupt reinjection.If you have more elegant solution let me know.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=58931
--
Gleb.
Jan ping, are you OK with what I proposed below?
Quoted 13 lines · Gleb Natapov
Hi Jan,
I bisected [1] to
f1ed0450a5fac7067590317cbf027f566b6ccbca. Fortunately further investigation showed that it is not really related to removing APIC timer interrupt reinjection and the real problem is that we cannot assume that__apic_accept_irq()always injects interrupts like the patch does because the function skips interrupt injection if APIC is disabled. This misreporting screws RTC interrupt tracking, so further RTC interrupt are stopped to be injected. The simplest solution that I see is to revert most of the commit and only leave APIC timer interrupt reinjection.If you have more elegant solution let me know.
Quoted 7 lines
[1] https://bugzilla.kernel.org/show_bug.cgi?id=58931
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.
Quoted 13 lines · Gleb Natapov
Jan ping, are you OK with what I proposed below?
Quoted 10 lines · Gleb Natapov
Hi Jan,
I bisected [1] to
f1ed0450a5fac7067590317cbf027f566b6ccbca. Fortunately further investigation showed that it is not really related to removing APIC timer interrupt reinjection and the real problem is that we cannot assume that__apic_accept_irq()always injects interrupts like the patch does because the function skips interrupt injection if APIC is disabled. This misreporting screws RTC interrupt tracking, so further RTC interrupt are stopped to be injected. The simplest solution that I see is to revert most of the commit and only leave APIC timer interrupt reinjection.
I'm not understanding the precise error yet and how __apic_accept_irq should be (properly) involved in its solution. Which code path depend on the information that the APIC is enabled?
The point is that preserving the return value of __apic_accept_irq, just redefining it to "delivery_mode != APIC_DM_FIXED || apic_enabled()" creates a pretty ugly interface, no? Can't we address the specific issue of the RTC at a different level?
Jan
Quoted 15 lines · Jan Kiszka
Quoted 13 lines · Gleb Natapov
Jan ping, are you OK with what I proposed below?
Quoted 10 lines · Gleb Natapov
Hi Jan,
I bisected [1] to
f1ed0450a5fac7067590317cbf027f566b6ccbca. Fortunately further investigation showed that it is not really related to removing APIC timer interrupt reinjection and the real problem is that we cannot assume that__apic_accept_irq()always injects interrupts like the patch does because the function skips interrupt injection if APIC is disabled. This misreporting screws RTC interrupt tracking, so further RTC interrupt are stopped to be injected. The simplest solution that I see is to revert most of the commit and only leave APIC timer interrupt reinjection.
Quoted 4 lines
I'm not understanding the precise error yet and how
__apic_accept_irqshould be (properly) involved in its solution. Which code path depend on the information that the APIC is enabled?
RTC interrupt injection tracking in virt/kvm/ioapic.c depends on accurate information about which vcpus interrupt was injected into since it expects EOI from each vcpu before injection next RTC interrupt. Since now kvm_apic_set_irq() reports interrupt as injected for vcpus with disabled apic the logic breaks because EOI will never happen.
Quoted 5 lines
The point is that preserving the return value of
__apic_accept_irq, just redefining it to "delivery_mode!=APIC_DM_FIXED||apic_enabled()" creates a pretty ugly interface, no? Can't we address the specific issue of the RTC at a different level?
Do not see how.
--
Gleb.
Quoted 20 lines · Gleb Natapov
Quoted 15 lines · Jan Kiszka
Quoted 13 lines · Gleb Natapov
Jan ping, are you OK with what I proposed below?
Quoted 10 lines · Gleb Natapov
Hi Jan,
I bisected [1] to
f1ed0450a5fac7067590317cbf027f566b6ccbca. Fortunately further investigation showed that it is not really related to removing APIC timer interrupt reinjection and the real problem is that we cannot assume that__apic_accept_irq()always injects interrupts like the patch does because the function skips interrupt injection if APIC is disabled. This misreporting screws RTC interrupt tracking, so further RTC interrupt are stopped to be injected. The simplest solution that I see is to revert most of the commit and only leave APIC timer interrupt reinjection.Quoted 4 lines
I'm not understanding the precise error yet and how
__apic_accept_irqshould be (properly) involved in its solution. Which code path depend on the information that the APIC is enabled?
Quoted 5 lines
RTC interrupt injection tracking in
virt/kvm/ioapic.cdepends on accurate information about which vcpus interrupt was injected into since it expects EOI from each vcpu before injection next RTC interrupt. Since nowkvm_apic_set_irq()reports interrupt as injected for vcpus with disabled apic the logic breaks because EOI will never happen.
OK, so we may have to enhance kvm_apic_set_irq. Or implement the apic_enabled check properly in __apic_accept_irq (though only kvm_apic_set_irq will have a use for it).
The (historic) check looks very strange, misplaced, and carries a comment that is probably also outdated. The check used to protect the only functioning delivery mode, but then was left in place, ignoring all the other modes.
Yes, that's not directly related to this regression. We can revert first and then rework this interface. But the latter should definitely be done as the revert will make the interface even worse IMHO.
Jan
Quoted 27 lines · Jan Kiszka
Quoted 20 lines · Gleb Natapov
Quoted 15 lines · Jan Kiszka
Quoted 13 lines · Gleb Natapov
Jan ping, are you OK with what I proposed below?
Quoted 10 lines · Gleb Natapov
Hi Jan,
I bisected [1] to
f1ed0450a5fac7067590317cbf027f566b6ccbca. Fortunately further investigation showed that it is not really related to removing APIC timer interrupt reinjection and the real problem is that we cannot assume that__apic_accept_irq()always injects interrupts like the patch does because the function skips interrupt injection if APIC is disabled. This misreporting screws RTC interrupt tracking, so further RTC interrupt are stopped to be injected. The simplest solution that I see is to revert most of the commit and only leave APIC timer interrupt reinjection.Quoted 4 lines
I'm not understanding the precise error yet and how
__apic_accept_irqshould be (properly) involved in its solution. Which code path depend on the information that the APIC is enabled?Quoted 5 lines
RTC interrupt injection tracking in
virt/kvm/ioapic.cdepends on accurate information about which vcpus interrupt was injected into since it expects EOI from each vcpu before injection next RTC interrupt. Since nowkvm_apic_set_irq()reports interrupt as injected for vcpus with disabled apic the logic breaks because EOI will never happen.
Quoted 9 lines
OK, so we may have to enhance
kvm_apic_set_irq. Or implement theapic_enabledcheck properly in__apic_accept_irq(though onlykvm_apic_set_irqwill have a use for it).The (historic) check looks very strange, misplaced, and carries a comment that is probably also outdated. The check used to protect the only functioning delivery mode, but then was left in place, ignoring all the other modes.
The only other delivery mode that needs to be protected is NMI as far as I see and check can be relaxed to kvm_apic_sw_enabled() since if apic is hw disabled the function should not be called. I do not think the check is strange or misplaced, where do you propose to put it instead? The comment is probably outdated since I cannot figure out what it means :)
Quoted 7 lines
Yes, that's not directly related to this regression. We can revert first and then rework this interface. But the latter should definitely be done as the revert will make the interface even worse IMHO.
Jan
--
Gleb.