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 <
address@hidden> wrote:

> Hi Sameeh,
>
> On Thu, 17 Mar 2016 09:37:57 +0200, address@hidden 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>.*