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

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

[Sameeh Jubran]

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  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 Junior
Software Engineer @ Daynix .*

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

[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

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