Patchwork [RFC,1/2] igb: statistic optimization

The network statistics were being updated by multiple CPU's causing cache
line bounces. Since this driver already keeps statistics per ring, use
those to compute the bytes/packet statistics.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Compile tested only, evaluation in progress


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 21 Oct 2008, Stephen Hemminger wrote:
> The network statistics were being updated by multiple CPU's causing cache
> line bounces. Since this driver already keeps statistics per ring, use
> those to compute the bytes/packet statistics.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Yeah, this is code we developed for e1000(e) and that was for only one 
queue, it should be a problem, and we should fix it one way or another.

Thanks Stephen, everything seems good after a quick review, except for one 
note: see below.

> Compile tested only, evaluation in progress

okay

> --- a/drivers/net/igb/igb_main.c        2008-10-21 08:57:11.000000000 -0700
> +++ b/drivers/net/igb/igb_main.c        2008-10-21 09:05:43.000000000 -0700
> @@ -3029,9 +3029,27 @@ static struct net_device_stats *
>  igb_get_stats(struct net_device *netdev)
>  {
>         struct igb_adapter *adapter = netdev_priv(netdev);
> +       struct net_device_stats *stats = &adapter->net_stats;
> +       int i;
> +
> +       stats->tx_bytes = 0;
> +       stats->tx_packets = 0;
> +       for (i = 0; i < adapter->num_tx_queues; i++) {
> +               struct igb_ring *tx_ring = &adapter->tx_ring[i];
> +               stats->tx_bytes += tx_ring->tx_stats.bytes;
> +               stats->tx_packets += tx_ring->tx_stats.packets;
> +       }
> +       stats->rx_bytes = 0;
> +       stats->rx_packets = 0;
> +       for (i = 0; i < adapter->num_rx_queues; i++) {
> +               struct igb_ring *rx_ring = &adapter->rx_ring[i];
> +               stats->rx_bytes += rx_ring->rx_stats.bytes;
> +               stats->rx_packets += rx_ring->rx_stats.packets;
> +       }
> +
> +       igb_update_stats(adapter);

I don't think you want to put this in the code that can be called directly 
from IOCTL from userspace.  This function can take a lot of cycles and 
some silly applications like gkrellm call it quite frequently.  The 
update_stats function will be called as part of the watchdog anyway, and 
you already got the interesting stats for realtime with the tx and rx 
bytes/packets.

> -       /* only return the current stats */
> -       return &adapter->net_stats;
> +       return stats;
>  }
> 
>  /**
> @@ -3711,8 +3729,6 @@ done_cleaning:
>         tx_ring->total_packets += total_packets;
>         tx_ring->tx_stats.bytes += total_bytes;
>         tx_ring->tx_stats.packets += total_packets;
> -       adapter->net_stats.tx_bytes += total_bytes;
> -       adapter->net_stats.tx_packets += total_packets;
>         return retval;
>  }
> 
> @@ -3953,8 +3969,6 @@ next_desc:
>         rx_ring->total_bytes += total_bytes;
>         rx_ring->rx_stats.packets += total_packets;
>         rx_ring->rx_stats.bytes += total_bytes;
> -       adapter->net_stats.rx_bytes += total_bytes;
> -       adapter->net_stats.rx_packets += total_packets;
>         return cleaned;
>  }

The same thing should be done to ixgbe too, but I'm just mentioning it for 
posterity, not because I'd expect you to do so.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> The same thing should be done to ixgbe too, but I'm just mentioning it for
> posterity, not because I'd expect you to do so.

I've already got it on my list.

-PJ
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> I don't think you want to put this in the code that can be called directly 
> from IOCTL from userspace.  This function can take a lot of cycles and 
> some silly applications like gkrellm call it quite frequently.  The 
> update_stats function will be called as part of the watchdog anyway, and 
> you already got the interesting stats for realtime with the tx and rx 
> bytes/packets.

On the flip side aren't there "out of phase" issues with pulling stats 
based on a timer vs something like a netstat -i 1 command?

rick jones
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 22 Oct 2008 10:37:44 -0700
Rick Jones <rick.jones2@hp.com> wrote:

> > I don't think you want to put this in the code that can be called directly 
> > from IOCTL from userspace.  This function can take a lot of cycles and 
> > some silly applications like gkrellm call it quite frequently.  The 
> > update_stats function will be called as part of the watchdog anyway, and 
> > you already got the interesting stats for realtime with the tx and rx 
> > bytes/packets.
> 
> On the flip side aren't there "out of phase" issues with pulling stats 
> based on a timer vs something like a netstat -i 1 command?
> 
> rick jones

The only stats pulled are the error stats, so this is probably okay.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html