Regression after "Remove support for reporting coalesced APIC IRQs"

Regression after "Remove support for reporting coalesced APIC IRQs"

[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.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=58931
--
			Gleb.

RE: Regression after "Remove support for reporting coalesced APIC IRQs"

[Ren, Yongjie]

> -----Original Message-----
> From: Gleb Natapov [mailto:gleb@redhat.com]
> Sent: Thursday, June 06, 2013 4:54 PM
> To: Jan Kiszka
> Cc: kvm@vger.kernel.org; Ren, Yongjie
> Subject: Regression after "Remove support for reporting coalesced APIC
> IRQs"
> 
> 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.

> 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.

Re: Regression after "Remove support for reporting coalesced APIC IRQs"

[Gleb Natapov]

Jan ping, are you OK with what I proposed below?

On Thu, Jun 06, 2013 at 11:53:52AM +0300, Gleb Natapov wrote:
> 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.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: Regression after "Remove support for reporting coalesced APIC IRQs"

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

On 2013-06-20 13:47, Gleb Natapov wrote:
> Jan ping, are you OK with what I proposed below?
> 
> On Thu, Jun 06, 2013 at 11:53:52AM +0300, Gleb Natapov wrote:
>> 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: Regression after "Remove support for reporting coalesced APIC IRQs"

[Gleb Natapov]

On Thu, Jun 20, 2013 at 10:10:18PM +0200, Jan Kiszka wrote:
> On 2013-06-20 13:47, Gleb Natapov wrote:
> > Jan ping, are you OK with what I proposed below?
> > 
> > On Thu, Jun 06, 2013 at 11:53:52AM +0300, Gleb Natapov wrote:
> >> 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?
> 
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.

> 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.

Re: Regression after "Remove support for reporting coalesced APIC IRQs"

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

On 2013-06-20 22:29, Gleb Natapov wrote:
> On Thu, Jun 20, 2013 at 10:10:18PM +0200, Jan Kiszka wrote:
>> On 2013-06-20 13:47, Gleb Natapov wrote:
>>> Jan ping, are you OK with what I proposed below?
>>>
>>> On Thu, Jun 06, 2013 at 11:53:52AM +0300, Gleb Natapov wrote:
>>>> 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?
>>
> 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.

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: Regression after "Remove support for reporting coalesced APIC IRQs"

[Gleb Natapov]

On Thu, Jun 20, 2013 at 11:01:54PM +0200, Jan Kiszka wrote:
> On 2013-06-20 22:29, Gleb Natapov wrote:
> > On Thu, Jun 20, 2013 at 10:10:18PM +0200, Jan Kiszka wrote:
> >> On 2013-06-20 13:47, Gleb Natapov wrote:
> >>> Jan ping, are you OK with what I proposed below?
> >>>
> >>> On Thu, Jun 06, 2013 at 11:53:52AM +0300, Gleb Natapov wrote:
> >>>> 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?
> >>
> > 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.
> 
> 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.
> 
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 :)

> 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.