Re: [Qemu-devel] [question] e1000 interrupt stormhappenedbecauseofits correspondingioapic->irr bit always set

[Jason Wang <jasowang@redhat.com>]

On 09/04/2014 09:56 AM, Zhang Haoyu wrote:
>>> Hi Jason,
>>> >> I tested below patch, it's okay, the e1000 interrupt storm disappeared.
>>> >> But I am going to make a bit change on it, could you help review it?
>>> >> 
>>>> >> >Currently, we call ioapic_service() immediately when we find the irq is still
>>>> >> >active during eoi broadcast. But for real hardware, there's some dealy between
>>>> >> >the EOI writing and irq delivery (system bus latency?). So we need to emulate
>>>> >> >this behavior. Otherwise, for a guest who haven't register a proper irq handler
>>>> >> >, it would stay in the interrupt routine as this irq would be re-injected
>>>> >> >immediately after guest enables interrupt. This would lead guest can't move
>>>> >> >forward and may miss the possibility to get proper irq handler registered (one
>>>> >> >example is windows guest resuming from hibernation).
>>>> >> >
>>>> >> >As there's no way to differ the unhandled irq from new raised ones, this patch
>>>> >> >solve this problems by scheduling a delayed work when the count of irq injected
>>>> >> >during eoi broadcast exceeds a threshold value. After this patch, the guest can
>>>> >> >move a little forward when there's no suitable irq handler in case it may
>>>> >> >register one very soon and for guest who has a bad irq detection routine ( such
>>>> >> >as note_interrupt() in linux ), this bad irq would be recognized soon as in the
>>>> >> >past.
>>>> >> >
>>>> >> >Signed-off-by: Jason Wang <jasowang <at> redhat.com>
>>>> >> >---
>>>> >> > virt/kvm/ioapic.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
>>>> >> > virt/kvm/ioapic.h |    2 ++
>>>> >> > 2 files changed, 47 insertions(+), 2 deletions(-)
>>>> >> >
>>>> >> >diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>>>> >> >index dcaf272..892253e 100644
>>>> >> >--- a/virt/kvm/ioapic.c
>>>> >> >+++ b/virt/kvm/ioapic.c
>>>> >> > <at>  <at>  -221,6 +221,24  <at>  <at>  int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
>>>> >> > 	return ret;
>>>> >> > }
>>>> >> >
>>>> >> >+static void kvm_ioapic_eoi_inject_work(struct work_struct *work)
>>>> >> >+{
>>>> >> >+	int i, ret;
>>>> >> >+	struct kvm_ioapic *ioapic = container_of(work, struct kvm_ioapic,
>>>> >> >+						 eoi_inject.work);
>>>> >> >+	spin_lock(&ioapic->lock);
>>>> >> >+	for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>>>> >> >+		union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i];
>>>> >> >+
>>>> >> >+		if (ent->fields.trig_mode != IOAPIC_LEVEL_TRIG)
>>>> >> >+			continue;
>>>> >> >+
>>>> >> >+		if (ioapic->irr & (1 << i) && !ent->fields.remote_irr)
>>>> >> >+			ret = ioapic_service(ioapic, i);
>>>> >> >+	}
>>>> >> >+	spin_unlock(&ioapic->lock);
>>>> >> >+}
>>>> >> >+
>>>> >> > static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
>>>> >> > 				     int trigger_mode)
>>>> >> > {
>>>> >> > <at>  <at>  -249,8 +267,29  <at>  <at>  static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
>>>> >> >
>>>> >> > 		ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
>>>> >> > 		ent->fields.remote_irr = 0;
>>>> >> >-		if (!ent->fields.mask && (ioapic->irr & (1 << i)))
>>>> >> >-			ioapic_service(ioapic, i);
>>>> >> >+		if (!ent->fields.mask && (ioapic->irr & (1 << i))) {
>>>> >> >+			++ioapic->irq_eoi;
>>> >> -+			++ioapic->irq_eoi;
>>> >> ++		    ++ioapic->irq_eoi[i];
>>>> >> >+			if (ioapic->irq_eoi == 100) {
>>> >> -+			if (ioapic->irq_eoi == 100) {
>>> >> ++			if (ioapic->irq_eoi[i] == 100) {
>>>> >> >+				/*
>>>> >> >+				 * Real hardware does not deliver the irq so
>>>> >> >+				 * immediately during eoi broadcast, so we need
>>>> >> >+				 * to emulate this behavior. Otherwise, for
>>>> >> >+				 * guests who has not registered handler of a
>>>> >> >+				 * level irq, this irq would be injected
>>>> >> >+				 * immediately after guest enables interrupt
>>>> >> >+				 * (which happens usually at the end of the
>>>> >> >+				 * common interrupt routine). This would lead
>>>> >> >+				 * guest can't move forward and may miss the
>>>> >> >+				 * possibility to get proper irq handler
>>>> >> >+				 * registered. So we need to give some breath to
>>>> >> >+				 * guest. TODO: 1 is too long?
>>>> >> >+				 */
>>>> >> >+				schedule_delayed_work(&ioapic->eoi_inject, 1);
>>>> >> >+				ioapic->irq_eoi = 0;
>>> >> -+				ioapic->irq_eoi = 0;
>>> >> ++				ioapic->irq_eoi[i] = 0;
>>>> >> >+			} else {
>>>> >> >+				ioapic_service(ioapic, i);
>>>> >> >+			}
>>>> >> >+		}
>>> >> ++		else {
>>> >> ++			ioapic->irq_eoi[i] = 0;
>>> >> ++		}
>>>> >> > 	}
>>>> >> > }
>>> >> I think ioapic->irq_eoi is prone to reach to 100, because during a eoi broadcast, 
>>> >> it's possible that another interrupt's (not current eoi's corresponding interrupt) irr is set, so the ioapic->irq_eoi will grow continually,
>>> >> and not too long, ioapic->irq_eoi will reach to 100.
>>> >> I want to add "u32 irq_eoi[IOAPIC_NUM_PINS];" instead of "u32 irq_eoi;".
>>> >> Any ideas?
>>> >> 
>>> >> Zhang Haoyu
>> >
>> >I'm a bit concerned how this will affect realtime guests.
>> >Worth adding a flag to enable this, so that e.g. virtio is not
>> >affected?
>> >
> Your concern is reasonable.
> If applying Jason's original patch, sometimes the virtio's interrupt delay is more than 4ms(my host's HZ=250), 
> but very rarely happened.
> And with my above change on it(per irq counter instead of total irq counter), the delayed virtio interrupt is more rarely happened,
> then I use 1000 instead of 100 on "if (ioapic->irq_eoi[i] == 1000)",  I made a test for 10min, the delayed virtio interrupt has not happened.
>
> Thanks,
> Zhang Haoyu
>

I agree 100 is too aggressive here. Probably you may use a number even
much higher than 1000.

One more thing, may worth to add a tracepoint also if we really want this.