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

[Jason Wang]

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.