netdev - Re: [RFC 1/2] igb: statistic optimization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.WNT.4.63.0810211224440.2736@jbrandeb-desk.amr.corp.intel.com>
Date:	Tue, 21 Oct 2008 12:30:11 -0700 (Pacific Daylight Time)
From:	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>
To:	Stephen Hemminger <shemminger@...tta.com>
cc:	"jeffery.t.kirsher@...el.com" <jeffery.t.kirsher@...el.com>,
	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
	"Allan, Bruce W" <bruce.w.allan@...el.com>,
	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>,
	"e1000-devel@...ts.sourceforge.net" 
	<e1000-devel@...ts.sourceforge.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [RFC 1/2] igb: statistic optimization

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@...tta.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html