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

[Zhang Haoyu]

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

>
>> >
>> > <at>  <at>  -375,12 +414,14  <at>  <at>  void kvm_ioapic_reset(struct 
>> > kvm_ioapic *ioapic)
>> > {
>> >    int i;
>> >
>> >+   cancel_delayed_work_sync(&ioapic->eoi_inject);
>> >    for (i = 0; i < IOAPIC_NUM_PINS; i++)
>> >            ioapic->redirtbl[i].fields.mask = 1;
>> >    ioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
>> >    ioapic->ioregsel = 0;
>> >    ioapic->irr = 0;
>> >    ioapic->id = 0;
>> >+   ioapic->irq_eoi = 0;
>> -+   ioapic->irq_eoi = 0;
>> ++   memset(ioapic->irq_eoi, 0x00, IOAPIC_NUM_PINS);
>> >    update_handled_vectors(ioapic);
>> > }
>> >
>> > <at>  <at>  -398,6 +439,7  <at>  <at>  int kvm_ioapic_init(struct kvm *kvm)
>> >    if (!ioapic)
>> >            return -ENOMEM;
>> >    spin_lock_init(&ioapic->lock);
>> >+   INIT_DELAYED_WORK(&ioapic->eoi_inject, kvm_ioapic_eoi_inject_work);
>> >    kvm->arch.vioapic = ioapic;
>> >    kvm_ioapic_reset(ioapic);
>> >    kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
>> > <at>  <at>  -418,6 +460,7  <at>  <at>  void kvm_ioapic_destroy(struct kvm 
>> > *kvm)
>> > {
>> >    struct kvm_ioapic *ioapic = kvm->arch.vioapic;
>> >
>> >+   cancel_delayed_work_sync(&ioapic->eoi_inject);
>> >    if (ioapic) {
>> >            kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, &ioapic->dev);
>> >            kvm->arch.vioapic = NULL;
>> >diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
>> >index 0b190c3..8938e66 100644
>> >--- a/virt/kvm/ioapic.h
>> >+++ b/virt/kvm/ioapic.h
>> > <at>  <at>  -47,6 +47,8  <at>  <at>  struct kvm_ioapic {
>> >    void (*ack_notifier)(void *opaque, int irq);
>> >    spinlock_t lock;
>> >    DECLARE_BITMAP(handled_vectors, 256);
>> >+   struct delayed_work eoi_inject;
>> >+   u32 irq_eoi;
>> -+   u32 irq_eoi;
>> ++   u32 irq_eoi[IOAPIC_NUM_PINS];
>> > };
>> >
>> > #ifdef DEBUG
>>