[Qemu-devel] [PATCH 0/2] e1000: Introducing an upper bound of interrupts

[Qemu-devel] [PATCH 0/2] e1000: Introducing an upper bound of interrupts

[Sameeh Jubran]

This patch series introduces an upper bound for the number of interrupts
per second. This feature is supported by the real hardware, however up
until now it wasn't implemented in e1000. This feature is very
significant, it can prevent an interrupt storm by giving the driver
a bounded inter-interrupt interval to handle interrupts.

This patch was made after observing an interrupt storm in Windows 10
when disabling e1000.

How reproducible:

Steps to reproduce:
1. Start Win 10 guest with e1000 device.
2. Go to device manager and try to disable and enable the device.
3. After several enable/disable to the device the guest hangs when
the device is being disabled.

Actual results:
Guest hang after click OK button.

Expected results:
Device is disabled.

After applying the patch the guest no longer hangs, and an Iperf test
ran successfully.

Sameeh Jubran (2):
  e1000: Fixing interrupts pace.
  Revert "e1000: fix hang of win2k12 shutdown with flood ping"

 hw/net/e1000.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/2] e1000: Fixing interrupts pace.

[Sameeh Jubran]

This patch introduces an upper bound for number of interrupts
per second. Without this bound an interrupt storm can occur as
it has been observed on Windows 10 when disabling the device.

According to the SPEC - Intel PCI/PCI-X Family of Gigabit
Ethernet Controllers Software Developer's Manual, section
13.4.18 - the Ethernet controller guarantees a maximum
observable interrupt rate of 7813 interrupts/sec. If there is
no upper bound this could lead to an interrupt storm by e1000
(when mit_delay < 500) causing interrupts to fire at a very high
pace.
Thus if mit_delay < 500 then the delay should be set to the
minimum delay possible which is 500. This can be calculated
easily as follows:

Interval = 10^9 / (7813 * 256) = 500.

Signed-off-by: Sameeh Jubran <sameeh@daynix.com>
---
 hw/net/e1000.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0387fa0..09b9ab5 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -357,6 +357,14 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val)
             }
             mit_update_delay(&mit_delay, s->mac_reg[ITR]);
 
+            /*
+             * According to e1000 SPEC, the Ethernet controller guarantees
+             * a maximum observable interrupt rate of 7813 interrupts/sec.
+             * Thus if mit_delay < 500 then the delay should be set to the
+             * minimum delay possible which is 500.
+             */
+            mit_delay = (mit_delay < 500) ? 500 : mit_delay;
+
             if (mit_delay) {
                 s->mit_timer_on = 1;
                 timer_mod(s->mit_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/2] e1000: Fixing interrupts pace.

[Shmulik Ladkani]

Hi Sameeh,

On Thu, 17 Mar 2016 09:37:57 +0200, sameeh@daynix.com wrote:
> @@ -357,6 +357,14 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val)
>              }
>              mit_update_delay(&mit_delay, s->mac_reg[ITR]);
>  
> +            /*
> +             * According to e1000 SPEC, the Ethernet controller guarantees
> +             * a maximum observable interrupt rate of 7813 interrupts/sec.
> +             * Thus if mit_delay < 500 then the delay should be set to the
> +             * minimum delay possible which is 500.
> +             */
> +            mit_delay = (mit_delay < 500) ? 500 : mit_delay;
> +
>              if (mit_delay) {
>                  s->mit_timer_on = 1;
>                  timer_mod(s->mit_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +

Sorry for late response.

Formerly, 'mit_delay' could possibly be 0 (as being not updated by
any of the mit_update_delay calls), thus 'mit_timer' wouldn't be
armed.

The new logic forces mit_delay to be set to 500, even if it was 0
("unset").

Which approach is correct:
- Either the 'if (mit_delay)' is now superflous,
- Or, do we need to keep the "unset" sematics (i.e. mit_delay==0 means
  don't use the timer)

Regards,
Shmulik

Re: [Qemu-devel] [PATCH 1/2] e1000: Fixing interrupts pace.

[Sameeh Jubran]

As mentioned in the patch:
"According to the SPEC - intel PCI/PCI-X Family of Gigabit
Ethernet Controllers Software Developer's Manual, section
13.4.18 - the Ethernet controller guarantees a maximum
observable interrupt rate of 7813 interrupts/sec. If there is
no upper bound this could lead to an interrupt storm by e1000
(when mit_delay < 500) causing interrupts to fire at a very high
pace."
This means that on a real hardware when mit_delay==0 ( don't use the timer
) the Ethernet controller guarantees a maximum
observable interrupt rate of 7813 interrupts/sec. Unfortunately that isn't
the case in the emulated device and the interrupt
rate bypass the rate of the real hardware which could lead to an interrupt
storm. Setting mit_delay to 500 guarantees a maximum
interrupt rate of 7813 interrupts/sec.

Regards,
Sameeh

On Wed, May 4, 2016 at 2:34 PM, Shmulik Ladkani <
shmulik.ladkani@ravellosystems.com> wrote:

> Hi Sameeh,
>
> On Thu, 17 Mar 2016 09:37:57 +0200, sameeh@daynix.com wrote:
> > @@ -357,6 +357,14 @@ set_interrupt_cause(E1000State *s, int index,
> uint32_t val)
> >              }
> >              mit_update_delay(&mit_delay, s->mac_reg[ITR]);
> >
> > +            /*
> > +             * According to e1000 SPEC, the Ethernet controller
> guarantees
> > +             * a maximum observable interrupt rate of 7813
> interrupts/sec.
> > +             * Thus if mit_delay < 500 then the delay should be set to
> the
> > +             * minimum delay possible which is 500.
> > +             */
> > +            mit_delay = (mit_delay < 500) ? 500 : mit_delay;
> > +
> >              if (mit_delay) {
> >                  s->mit_timer_on = 1;
> >                  timer_mod(s->mit_timer,
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>
> Sorry for late response.
>
> Formerly, 'mit_delay' could possibly be 0 (as being not updated by
> any of the mit_update_delay calls), thus 'mit_timer' wouldn't be
> armed.
>
> The new logic forces mit_delay to be set to 500, even if it was 0
> ("unset").
>
> Which approach is correct:
> - Either the 'if (mit_delay)' is now superflous,
> - Or, do we need to keep the "unset" sematics (i.e. mit_delay==0 means
>   don't use the timer)
>
> Regards,
> Shmulik
>



-- 
Respectfully,
*Sameeh Jubran*
*Mobile: +972 054-2509642*

*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>Junior
Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH 1/2] e1000: Fixing interrupts pace.

[Shmulik Ladkani]

Hi,

On Mon, 16 May 2016 08:58:32 +0300, sameeh@daynix.com wrote:
> This means that on a real hardware when mit_delay==0 ( don't use the timer
> ) the Ethernet controller guarantees a maximum
> observable interrupt rate of 7813 interrupts/sec. Unfortunately that isn't
> the case in the emulated device and the interrupt
> rate bypass the rate of the real hardware which could lead to an interrupt
> storm. Setting mit_delay to 500 guarantees a maximum
> interrupt rate of 7813 interrupts/sec.

OK, if that is the case, then

> > > +            mit_delay = (mit_delay < 500) ? 500 : mit_delay;
> > > +
> > >              if (mit_delay) {
> > >                  s->mit_timer_on = 1;
> > >                  timer_mod(s->mit_timer,

The existing 'if (mit_delay)' is now superflous, since always true,
hence can be removed.

Re: [Qemu-devel] [PATCH 1/2] e1000: Fixing interrupts pace.

[Sameeh Jubran]

This seems like a good idea, thanks!

On Mon, May 16, 2016 at 1:55 PM, Shmulik Ladkani <
shmulik.ladkani@ravellosystems.com> wrote:

> Hi,
>
> On Mon, 16 May 2016 08:58:32 +0300, sameeh@daynix.com wrote:
> > This means that on a real hardware when mit_delay==0 ( don't use the
> timer
> > ) the Ethernet controller guarantees a maximum
> > observable interrupt rate of 7813 interrupts/sec. Unfortunately that
> isn't
> > the case in the emulated device and the interrupt
> > rate bypass the rate of the real hardware which could lead to an
> interrupt
> > storm. Setting mit_delay to 500 guarantees a maximum
> > interrupt rate of 7813 interrupts/sec.
>
> OK, if that is the case, then
>
> > > > +            mit_delay = (mit_delay < 500) ? 500 : mit_delay;
> > > > +
> > > >              if (mit_delay) {
> > > >                  s->mit_timer_on = 1;
> > > >                  timer_mod(s->mit_timer,
>
> The existing 'if (mit_delay)' is now superflous, since always true,
> hence can be removed.
>



-- 
Respectfully,
*Sameeh Jubran*
*Mobile: +972 054-2509642*

*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>Junior
Software Engineer @ Daynix <http://www.daynix.com>.*

[Qemu-devel] [PATCH 2/2] Revert "e1000: fix hang of win2k12 shutdown with flood ping"

[Sameeh Jubran]

This reverts commit 9596ef7c7b8528bedb240792ea1fb598543ad3c4.

This workaround in order to fix endless interrupts is no
longer needed because it was superseded by the previous patch
(e1000: Fixing interrupt pace).

Signed-off-by: Sameeh Jubran <sameeh@daynix.com>
---
 hw/net/e1000.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 09b9ab5..8e79b55 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -456,11 +456,6 @@ static void e1000_reset(void *opaque)
         e1000_link_down(d);
     }
 
-    /* Throttle interrupts to prevent guest (e.g Win 2012) from
-     * reinjecting interrupts endlessly. TODO: fix non ITR case.
-     */
-    d->mac_reg[ITR] = 250;
-
     /* Some guests expect pre-initialized RAH/RAL (AddrValid flag + MACaddr) */
     d->mac_reg[RA] = 0;
     d->mac_reg[RA + 1] = E1000_RAH_AV;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 0/2] e1000: Introducing an upper bound of interrupts

[Denis V. Lunev]

On 03/17/2016 10:37 AM, Sameeh Jubran wrote:
> This patch series introduces an upper bound for the number of interrupts
> per second. This feature is supported by the real hardware, however up
> until now it wasn't implemented in e1000. This feature is very
> significant, it can prevent an interrupt storm by giving the driver
> a bounded inter-interrupt interval to handle interrupts.
>
> This patch was made after observing an interrupt storm in Windows 10
> when disabling e1000.
>
> How reproducible:
>
> Steps to reproduce:
> 1. Start Win 10 guest with e1000 device.
> 2. Go to device manager and try to disable and enable the device.
> 3. After several enable/disable to the device the guest hangs when
> the device is being disabled.
>
> Actual results:
> Guest hang after click OK button.
>
> Expected results:
> Device is disabled.
>
> After applying the patch the guest no longer hangs, and an Iperf test
> ran successfully.
>
> Sameeh Jubran (2):
>    e1000: Fixing interrupts pace.
>    Revert "e1000: fix hang of win2k12 shutdown with flood ping"
>
>   hw/net/e1000.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
In general I support the idea to have the minimal limit
and drop original patch.

Though I'd better keep the same delay as it was. The limit
for physical card is good for physical card. They have
limitations which are not available in virtualization
world.

AFAIR I have used 250 as the number from the article
of the original author of ITR support as the best value.

Den

Re: [Qemu-devel] [PATCH 0/2] e1000: Introducing an upper bound of interrupts

[Jason Wang]

On 03/17/2016 03:37 PM, Sameeh Jubran wrote:
> This patch series introduces an upper bound for the number of interrupts
> per second. This feature is supported by the real hardware, however up
> until now it wasn't implemented in e1000. This feature is very
> significant, it can prevent an interrupt storm by giving the driver
> a bounded inter-interrupt interval to handle interrupts.
>
> This patch was made after observing an interrupt storm in Windows 10
> when disabling e1000.
>
> How reproducible:
>
> Steps to reproduce:
> 1. Start Win 10 guest with e1000 device.
> 2. Go to device manager and try to disable and enable the device.
> 3. After several enable/disable to the device the guest hangs when
> the device is being disabled.
>
> Actual results:
> Guest hang after click OK button.
>
> Expected results:
> Device is disabled.
>
> After applying the patch the guest no longer hangs, and an Iperf test
> ran successfully.
>
> Sameeh Jubran (2):
>   e1000: Fixing interrupts pace.
>   Revert "e1000: fix hang of win2k12 shutdown with flood ping"
>
>  hw/net/e1000.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>

Applied.

Thanks Sameeh

Re: [Qemu-devel] [PATCH 0/2] e1000: Introducing an upper bound of interrupts

[Sameeh Jubran]

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

You're welcome Jason.

Dear Denis,

The policy with e1000 was to keep it as much as possible to the real
Hardware, That's why I think we should keep it 500 even though 250 pushes
the performance to the limit.
Jason what do you think about that?

On Fri, Mar 18, 2016 at 3:34 AM, Jason Wang <jasowang@redhat.com> wrote:

>
>
> On 03/17/2016 03:37 PM, Sameeh Jubran wrote:
> > This patch series introduces an upper bound for the number of interrupts
> > per second. This feature is supported by the real hardware, however up
> > until now it wasn't implemented in e1000. This feature is very
> > significant, it can prevent an interrupt storm by giving the driver
> > a bounded inter-interrupt interval to handle interrupts.
> >
> > This patch was made after observing an interrupt storm in Windows 10
> > when disabling e1000.
> >
> > How reproducible:
> >
> > Steps to reproduce:
> > 1. Start Win 10 guest with e1000 device.
> > 2. Go to device manager and try to disable and enable the device.
> > 3. After several enable/disable to the device the guest hangs when
> > the device is being disabled.
> >
> > Actual results:
> > Guest hang after click OK button.
> >
> > Expected results:
> > Device is disabled.
> >
> > After applying the patch the guest no longer hangs, and an Iperf test
> > ran successfully.
> >
> > Sameeh Jubran (2):
> >   e1000: Fixing interrupts pace.
> >   Revert "e1000: fix hang of win2k12 shutdown with flood ping"
> >
> >  hw/net/e1000.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
>
> Applied.
>
> Thanks Sameeh
>



-- 
Respectfully,
*Sameeh Jubran*
*Mobile: +972 054-2509642*

*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>Junior
Software Engineer @ Daynix <http://www.daynix.com>.*

[-- Attachment #2: Type: text/html, Size: 3655 bytes --]

Re: [Qemu-devel] [PATCH 0/2] e1000: Introducing an upper bound of interrupts

[Michael S. Tsirkin]

On Thu, Mar 17, 2016 at 09:37:56AM +0200, Sameeh Jubran wrote:
> This patch series introduces an upper bound for the number of interrupts
> per second. This feature is supported by the real hardware, however up
> until now it wasn't implemented in e1000. This feature is very
> significant, it can prevent an interrupt storm by giving the driver
> a bounded inter-interrupt interval to handle interrupts.
> 
> This patch was made after observing an interrupt storm in Windows 10
> when disabling e1000.


Interesting. There's actually a kvm hack to work-around
this in older guests. We always assumed this is a guest bug,
but it might be worth testing whether the below patch is
still required after fixing qemu.

commit 184564efae4d775225c8fe3b762a56956fb1f827
Author: Zhang Haoyu <zhanghy@sangfor.com>
Date:   Thu Sep 11 16:47:04 2014 +0800

    kvm: ioapic: conditionally delay irq delivery duringeoi broadcast
    
    Currently, we call ioapic_service() immediately when we find the irq is still




> How reproducible:
> 
> Steps to reproduce:
> 1. Start Win 10 guest with e1000 device.
> 2. Go to device manager and try to disable and enable the device.
> 3. After several enable/disable to the device the guest hangs when
> the device is being disabled.
> 
> Actual results:
> Guest hang after click OK button.
> 
> Expected results:
> Device is disabled.
> 
> After applying the patch the guest no longer hangs, and an Iperf test
> ran successfully.
> 
> Sameeh Jubran (2):
>   e1000: Fixing interrupts pace.
>   Revert "e1000: fix hang of win2k12 shutdown with flood ping"
> 
>  hw/net/e1000.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> -- 
> 2.5.0
>