qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] implement moderation registers for e1000


From: Stefan Hajnoczi
Subject: Re: [Qemu-devel] [PATCH] implement moderation registers for e1000
Date: Fri, 8 Feb 2013 11:02:09 +0100
User-agent: Mutt/1.5.21 (2010-09-15)

On Wed, Feb 06, 2013 at 03:23:41PM +0100, Luigi Rizzo wrote:
> The following patch implements interrupt moderation registers
> for the e1000 adapter. These feature is normally used by OS
> drivers, and their implementation improves performance significantly,
> especially on the transmit path.
> The feature can be disabled through a command line option.
> We have seen only benefits in throughput, while latency slightly
> increases (that is an inherent feature of interrupt moderation)
> because very short delays cannot be emulated precisely.
> 
> For those curious on performance, there is a set of measurements
> (of this and other changes that we will post separately) at
> 
> http://info.iet.unipi.it/~luigi/research.html#qemu

http://info.iet.unipi.it/~luigi/papers/20130206-qemu.pdf is 404.

> +static void
> +mit_set_ics(E1000State *s, uint32_t cause)
> +{
> +    if (s->mit_on == 0) {
> +        set_ics(s, 0, cause);
> +        return;
> +    }
> +    s->mit_cause |= cause;
> +    if (!s->mit_timer_on)
> +        mit_rearm_and_int(s);

Please run scripts/checkpatch.pl.  QEMU coding style always uses braces,
even for an if statement with a single line body.

> @@ -1302,6 +1377,10 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>  
>      d->autoneg_timer = qemu_new_timer_ms(vm_clock, e1000_autoneg_timer, d);
>  
> +    d->mit_cause = 0;
> +    d->mit_timer_on = 0;
> +    d->mit_timer = qemu_new_timer_ns(vm_clock, mit_rearm_and_int, d);

Missing qemu_del_timer(d->mit_timer) and qemu_free_timer(d->mit_timer)
in pci_e1000_uninit().

> +
>      return 0;
>  }
>  
> @@ -1313,6 +1392,7 @@ static void qdev_e1000_reset(DeviceState *dev)
>  
>  static Property e1000_properties[] = {
>      DEFINE_NIC_PROPERTIES(E1000State, conf),
> +    DEFINE_PROP_UINT32("mit_on", E1000State, mit_on, 1), /* default on */

Can access the performance results you linked to.  Therefore it's hard
to know whether default on makes sense.  We must avoid performance
regressions.

Stefan



reply via email to

[Prev in Thread] Current Thread [Next in Thread]