Skip to contentSkip to conversation

igb: statistic optimization

RFC In review Stephen Hemminger 4 participants 5 messages Oct 21, 2008 – Oct 22, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

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 <[email protected]>

---
Compile tested only, evaluation in progress

drivers/net/igb/igb_main.c +20 -6
@@ -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);
 
-	/* 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;
 }

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

#1 lore raw
Quoted 5 lines · Stephen Hemminger

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 <[email protected]>

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.

Quoted 1 line

Compile tested only, evaluation in progress

okay

Quoted 15 lines
drivers/net/igb/igb_main.c 2008-10-21 09:05:43.000000000 -0700 renamed from drivers/net/igb/igb_main.c 2008-10-21 08:57:11.000000000 -0700 +9 -0
@@ -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;

drivers/net/igb/igb_main.c 2008-10-21 09:05:43.000000000 -0700 static struct net_device_stats * · line 3050

Quoted 10 lines
drivers/net/igb/igb_main.c 2008-10-21 09:05:43.000000000 -0700 renamed from drivers/net/igb/igb_main.c 2008-10-21 08:57:11.000000000 -0700 +10 -0
+       }
+       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.

Quoted 16 lines
drivers/net/igb/igb_main.c 2008-10-21 09:05:43.000000000 -0700 renamed from drivers/net/igb/igb_main.c 2008-10-21 08:57:11.000000000 -0700 +1 -4
-       /* 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;
 }

drivers/net/igb/igb_main.c 2008-10-21 09:05:43.000000000 -0700 next_desc: · line 3973

Quoted 7 lines
drivers/net/igb/igb_main.c 2008-10-21 09:05:43.000000000 -0700 renamed from drivers/net/igb/igb_main.c 2008-10-21 08:57:11.000000000 -0700 +0 -2
@@ -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.

#2 lore raw
Quoted 2 lines

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

#3 lore raw
In reply to Brandeburg, Jesse
Quoted 6 lines

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

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

author
#4 lore raw
Quoted 11 lines · On Wed, 22 Oct 2008 10:37:44 -0700 Rick Jones <[email protected]> wrote:
Quoted 6 lines

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.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/