[PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Sathya Perla]

Problem initially reproted and fixed by Eric Dumazet <eric.dumazet@gmail.com>

netdev_stats_update() resets netdev->stats and then accumulates stats from
various rings. This is wrong as stats readers can sometimes catch zero values.
Use temporary variables instead for accumulating per-ring values.

Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
 drivers/net/benet/be_main.c |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index c4f564c..5ca06b0 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -428,33 +428,38 @@ void netdev_stats_update(struct be_adapter *adapter)
 	struct net_device_stats *dev_stats = &adapter->netdev->stats;
 	struct be_rx_obj *rxo;
 	struct be_tx_obj *txo;
+	unsigned long pkts = 0, bytes = 0, mcast = 0, drops = 0;
 	int i;
 
-	memset(dev_stats, 0, sizeof(*dev_stats));
 	for_all_rx_queues(adapter, rxo, i) {
-		dev_stats->rx_packets += rx_stats(rxo)->rx_pkts;
-		dev_stats->rx_bytes += rx_stats(rxo)->rx_bytes;
-		dev_stats->multicast += rx_stats(rxo)->rx_mcast_pkts;
+		pkts += rx_stats(rxo)->rx_pkts;
+		bytes += rx_stats(rxo)->rx_bytes;
+		mcast += rx_stats(rxo)->rx_mcast_pkts;
 		/*  no space in linux buffers: best possible approximation */
 		if (adapter->generation == BE_GEN3) {
 			if (!(lancer_chip(adapter))) {
-				struct be_erx_stats_v1 *erx_stats =
+				struct be_erx_stats_v1 *erx =
 					be_erx_stats_from_cmd(adapter);
-				dev_stats->rx_dropped +=
-				erx_stats->rx_drops_no_fragments[rxo->q.id];
+				drops += erx->rx_drops_no_fragments[rxo->q.id];
 			}
 		} else {
-			struct be_erx_stats_v0 *erx_stats =
+			struct be_erx_stats_v0 *erx =
 					be_erx_stats_from_cmd(adapter);
-			dev_stats->rx_dropped +=
-				erx_stats->rx_drops_no_fragments[rxo->q.id];
+			drops += erx->rx_drops_no_fragments[rxo->q.id];
 		}
 	}
+	dev_stats->rx_packets = pkts;
+	dev_stats->rx_bytes = bytes;
+	dev_stats->multicast = mcast;
+	dev_stats->rx_dropped = drops;
 
+	pkts = bytes = 0;
 	for_all_tx_queues(adapter, txo, i) {
-		dev_stats->tx_packets += tx_stats(txo)->be_tx_pkts;
-		dev_stats->tx_bytes += tx_stats(txo)->be_tx_bytes;
+		pkts += tx_stats(txo)->be_tx_pkts;
+		bytes += tx_stats(txo)->be_tx_bytes;
 	}
+	dev_stats->tx_packets = pkts;
+	dev_stats->tx_bytes = bytes;
 
 	/* bad pkts received */
 	dev_stats->rx_errors = drvs->rx_crc_errors +
-- 
1.7.4

Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Eric Dumazet]

Le lundi 27 juin 2011 à 12:10 +0530, Sathya Perla a écrit :
> Problem initially reproted and fixed by Eric Dumazet <eric.dumazet@gmail.com>
> 
> netdev_stats_update() resets netdev->stats and then accumulates stats from
> various rings. This is wrong as stats readers can sometimes catch zero values.
> Use temporary variables instead for accumulating per-ring values.
> 
> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
> ---

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Thanks

Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 27 Jun 2011 08:56:29 +0200

> Le lundi 27 juin 2011 à 12:10 +0530, Sathya Perla a écrit :
>> Problem initially reproted and fixed by Eric Dumazet <eric.dumazet@gmail.com>
>> 
>> netdev_stats_update() resets netdev->stats and then accumulates stats from
>> various rings. This is wrong as stats readers can sometimes catch zero values.
>> Use temporary variables instead for accumulating per-ring values.
>> 
>> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
>> ---
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Stephen Hemminger]

On Mon, 27 Jun 2011 12:10:48 +0530
Sathya Perla <sathya.perla@emulex.com> wrote:

> Problem initially reproted and fixed by Eric Dumazet <eric.dumazet@gmail.com>
> 
> netdev_stats_update() resets netdev->stats and then accumulates stats from
> various rings. This is wrong as stats readers can sometimes catch zero values.
> Use temporary variables instead for accumulating per-ring values.
> 
> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>

Should also use u64_stats_sync to ensure correct rollover or 32 bit SMP
platform.

Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Eric Dumazet]

Le lundi 27 juin 2011 à 09:43 -0700, Stephen Hemminger a écrit :
> On Mon, 27 Jun 2011 12:10:48 +0530
> Sathya Perla <sathya.perla@emulex.com> wrote:
> 
> > Problem initially reproted and fixed by Eric Dumazet <eric.dumazet@gmail.com>
> > 
> > netdev_stats_update() resets netdev->stats and then accumulates stats from
> > various rings. This is wrong as stats readers can sometimes catch zero values.
> > Use temporary variables instead for accumulating per-ring values.
> > 
> > Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
> 
> Should also use u64_stats_sync to ensure correct rollover or 32 bit SMP
> platform.

These are "unsigned long" fields, you dont need u64_stats_sync.

[PATCH net-next] benet: convert to 64 bit stats

[Stephen Hemminger]

This changes how the benet driver does statistics:
  * use 64 bit statistics interface (old api was only 32 bit on 32 bit platform)
  * use u64_stats_sync to ensure atomic 64 bit on 32 bit SMP
  * only update statistics when needed

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

---

 drivers/net/benet/be.h      |    4 -
 drivers/net/benet/be_cmds.c |    1 
 drivers/net/benet/be_main.c |  161 ++++++++++++++++++++++++--------------------
 3 files changed, 93 insertions(+), 73 deletions(-)

--- a/drivers/net/benet/be.h	2011-06-27 10:37:01.519999268 -0700
+++ b/drivers/net/benet/be.h	2011-06-27 11:00:02.691999144 -0700
@@ -29,6 +29,7 @@
 #include <linux/interrupt.h>
 #include <linux/firmware.h>
 #include <linux/slab.h>
+#include <linux/u64_stats_sync.h>
 
 #include "be_hw.h"
 
@@ -176,6 +177,7 @@ struct be_tx_stats {
 	u64 be_tx_bytes_prev;
 	u64 be_tx_pkts;
 	u32 be_tx_rate;
+	struct u64_stats_sync syncp;
 };
 
 struct be_tx_obj {
@@ -210,6 +212,7 @@ struct be_rx_stats {
 	u32 rx_frags;
 	u32 prev_rx_frags;
 	u32 rx_fps;		/* Rx frags per second */
+	struct u64_stats_sync syncp;
 };
 
 struct be_rx_compl_info {
@@ -526,7 +529,6 @@ static inline bool be_multi_rxq(const st
 extern void be_cq_notify(struct be_adapter *adapter, u16 qid, bool arm,
 		u16 num_popped);
 extern void be_link_status_update(struct be_adapter *adapter, bool link_up);
-extern void netdev_stats_update(struct be_adapter *adapter);
 extern void be_parse_stats(struct be_adapter *adapter);
 extern int be_load_fw(struct be_adapter *adapter, u8 *func);
 #endif				/* BE_H */
--- a/drivers/net/benet/be_main.c	2011-06-27 10:26:59.635999322 -0700
+++ b/drivers/net/benet/be_main.c	2011-06-27 10:59:37.923999145 -0700
@@ -418,77 +418,6 @@ void be_parse_stats(struct be_adapter *a
 	}
 }
 
-void netdev_stats_update(struct be_adapter *adapter)
-{
-	struct be_drv_stats *drvs = &adapter->drv_stats;
-	struct net_device_stats *dev_stats = &adapter->netdev->stats;
-	struct be_rx_obj *rxo;
-	struct be_tx_obj *txo;
-	unsigned long pkts = 0, bytes = 0, mcast = 0, drops = 0;
-	int i;
-
-	for_all_rx_queues(adapter, rxo, i) {
-		pkts += rx_stats(rxo)->rx_pkts;
-		bytes += rx_stats(rxo)->rx_bytes;
-		mcast += rx_stats(rxo)->rx_mcast_pkts;
-		/*  no space in linux buffers: best possible approximation */
-		if (adapter->generation == BE_GEN3) {
-			if (!(lancer_chip(adapter))) {
-				struct be_erx_stats_v1 *erx =
-					be_erx_stats_from_cmd(adapter);
-				drops += erx->rx_drops_no_fragments[rxo->q.id];
-			}
-		} else {
-			struct be_erx_stats_v0 *erx =
-					be_erx_stats_from_cmd(adapter);
-			drops += erx->rx_drops_no_fragments[rxo->q.id];
-		}
-	}
-	dev_stats->rx_packets = pkts;
-	dev_stats->rx_bytes = bytes;
-	dev_stats->multicast = mcast;
-	dev_stats->rx_dropped = drops;
-
-	pkts = bytes = 0;
-	for_all_tx_queues(adapter, txo, i) {
-		pkts += tx_stats(txo)->be_tx_pkts;
-		bytes += tx_stats(txo)->be_tx_bytes;
-	}
-	dev_stats->tx_packets = pkts;
-	dev_stats->tx_bytes = bytes;
-
-	/* bad pkts received */
-	dev_stats->rx_errors = drvs->rx_crc_errors +
-		drvs->rx_alignment_symbol_errors +
-		drvs->rx_in_range_errors +
-		drvs->rx_out_range_errors +
-		drvs->rx_frame_too_long +
-		drvs->rx_dropped_too_small +
-		drvs->rx_dropped_too_short +
-		drvs->rx_dropped_header_too_small +
-		drvs->rx_dropped_tcp_length +
-		drvs->rx_dropped_runt +
-		drvs->rx_tcp_checksum_errs +
-		drvs->rx_ip_checksum_errs +
-		drvs->rx_udp_checksum_errs;
-
-	/* detailed rx errors */
-	dev_stats->rx_length_errors = drvs->rx_in_range_errors +
-		drvs->rx_out_range_errors +
-		drvs->rx_frame_too_long;
-
-	dev_stats->rx_crc_errors = drvs->rx_crc_errors;
-
-	/* frame alignment errors */
-	dev_stats->rx_frame_errors = drvs->rx_alignment_symbol_errors;
-
-	/* receiver fifo overrun */
-	/* drops_no_pbuf is no per i/f, it's per BE card */
-	dev_stats->rx_fifo_errors = drvs->rxpp_fifo_overflow_drop +
-				drvs->rx_input_fifo_overflow_drop +
-				drvs->rx_drops_no_pbuf;
-}
-
 void be_link_status_update(struct be_adapter *adapter, bool link_up)
 {
 	struct net_device *netdev = adapter->netdev;
@@ -586,8 +515,10 @@ static void be_tx_stats_update(struct be
 
 	stats->be_tx_reqs++;
 	stats->be_tx_wrbs += wrb_cnt;
+	u64_stats_update_begin(&stats->syncp);
 	stats->be_tx_bytes += copied;
 	stats->be_tx_pkts += (gso_segs ? gso_segs : 1);
+	u64_stats_update_end(&stats->syncp);
 	if (stopped)
 		stats->be_tx_stops++;
 }
@@ -793,6 +724,89 @@ static netdev_tx_t be_xmit(struct sk_buf
 	return NETDEV_TX_OK;
 }
 
+static struct rtnl_link_stats64 *be_get_stats(struct net_device *netdev,
+					      struct rtnl_link_stats64 *stats)
+{
+	struct be_adapter *adapter = netdev_priv(netdev);
+	struct be_drv_stats *drvs = &adapter->drv_stats;
+	const struct be_rx_obj *rxo;
+	const struct be_tx_obj *txo;
+	u64 pkts, bytes;
+	unsigned int start;
+	int i;
+
+	for_all_rx_queues(adapter, rxo, i) {
+		const struct be_rx_stats *rx_stats = rx_stats(rxo);
+		do {
+			start = u64_stats_fetch_begin(&rx_stats->syncp);
+			pkts = rx_stats->rx_pkts;
+			bytes = rx_stats->rx_bytes;
+		} while (u64_stats_fetch_retry(&rx_stats->syncp, start));
+
+		stats->rx_packets += pkts;
+		stats->rx_bytes += bytes;
+		stats->multicast += rx_stats->rx_mcast_pkts;
+
+		/*  no space in linux buffers: best possible approximation */
+		if (adapter->generation == BE_GEN3) {
+			if (!(lancer_chip(adapter))) {
+				const struct be_erx_stats_v1 *erx =
+					be_erx_stats_from_cmd(adapter);
+				stats->rx_dropped += erx->rx_drops_no_fragments[rxo->q.id];
+			}
+		} else {
+			const struct be_erx_stats_v0 *erx =
+				be_erx_stats_from_cmd(adapter);
+			stats->rx_dropped += erx->rx_drops_no_fragments[rxo->q.id];
+		}
+	}
+
+	for_all_tx_queues(adapter, txo, i) {
+		const struct be_tx_stats *tx_stats = tx_stats(txo);
+		do {
+			start = u64_stats_fetch_begin(&tx_stats->syncp);
+			pkts = tx_stats->be_tx_pkts;
+			bytes = tx_stats->be_tx_bytes;
+		} while (u64_stats_fetch_retry(&tx_stats->syncp, start));
+
+		stats->tx_packets += pkts;
+		stats->tx_bytes += bytes;
+	}
+
+	/* bad pkts received */
+	stats->rx_errors = drvs->rx_crc_errors +
+		drvs->rx_alignment_symbol_errors +
+		drvs->rx_in_range_errors +
+		drvs->rx_out_range_errors +
+		drvs->rx_frame_too_long +
+		drvs->rx_dropped_too_small +
+		drvs->rx_dropped_too_short +
+		drvs->rx_dropped_header_too_small +
+		drvs->rx_dropped_tcp_length +
+		drvs->rx_dropped_runt +
+		drvs->rx_tcp_checksum_errs +
+		drvs->rx_ip_checksum_errs +
+		drvs->rx_udp_checksum_errs;
+
+	/* detailed rx errors */
+	stats->rx_length_errors = drvs->rx_in_range_errors +
+		drvs->rx_out_range_errors +
+		drvs->rx_frame_too_long;
+
+	stats->rx_crc_errors = drvs->rx_crc_errors;
+
+	/* frame alignment errors */
+	stats->rx_frame_errors = drvs->rx_alignment_symbol_errors;
+
+	/* receiver fifo overrun */
+	/* drops_no_pbuf is no per i/f, it's per BE card */
+	stats->rx_fifo_errors = drvs->rxpp_fifo_overflow_drop +
+		drvs->rx_input_fifo_overflow_drop +
+		drvs->rx_drops_no_pbuf;
+
+	return stats;
+}
+
 static int be_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct be_adapter *adapter = netdev_priv(netdev);
@@ -1040,8 +1054,12 @@ static void be_rx_stats_update(struct be
 
 	stats->rx_compl++;
 	stats->rx_frags += rxcp->num_rcvd;
+
+	u64_stats_update_begin(&stats->syncp);
 	stats->rx_bytes += rxcp->pkt_size;
 	stats->rx_pkts++;
+	u64_stats_update_end(&stats->syncp);
+
 	if (rxcp->pkt_type == BE_MULTICAST_PACKET)
 		stats->rx_mcast_pkts++;
 	if (rxcp->err)
@@ -2918,6 +2936,7 @@ static struct net_device_ops be_netdev_o
 	.ndo_set_rx_mode	= be_set_multicast_list,
 	.ndo_set_mac_address	= be_mac_addr_set,
 	.ndo_change_mtu		= be_change_mtu,
+	.ndo_get_stats64	= be_get_stats,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_vlan_rx_register	= be_vlan_register,
 	.ndo_vlan_rx_add_vid	= be_vlan_add_vid,
--- a/drivers/net/benet/be_cmds.c	2011-06-27 10:59:50.935999145 -0700
+++ b/drivers/net/benet/be_cmds.c	2011-06-27 10:59:54.643999144 -0700
@@ -103,7 +103,6 @@ static int be_mcc_compl_process(struct b
 							sizeof(resp->hw_stats));
 			}
 			be_parse_stats(adapter);
-			netdev_stats_update(adapter);
 			adapter->stats_cmd_sent = false;
 		}
 	} else if ((compl_status != MCC_STATUS_NOT_SUPPORTED) &&

Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Stephen Hemminger]

On Mon, 27 Jun 2011 19:20:41 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le lundi 27 juin 2011 à 09:43 -0700, Stephen Hemminger a écrit :
> > On Mon, 27 Jun 2011 12:10:48 +0530
> > Sathya Perla <sathya.perla@emulex.com> wrote:
> > 
> > > Problem initially reproted and fixed by Eric Dumazet <eric.dumazet@gmail.com>
> > > 
> > > netdev_stats_update() resets netdev->stats and then accumulates stats from
> > > various rings. This is wrong as stats readers can sometimes catch zero values.
> > > Use temporary variables instead for accumulating per-ring values.
> > > 
> > > Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
> > 
> > Should also use u64_stats_sync to ensure correct rollover or 32 bit SMP
> > platform.
> 
> These are "unsigned long" fields, you dont need u64_stats_sync.

The source fields (in be.h) are u64, but since destination is unsigned long
that works.

RE: [PATCH net-next] benet: convert to 64 bit stats

[Sathya.Perla]

>-----Original Message-----
>From: Stephen Hemminger [mailto:shemminger@vyatta.com]
>Sent: Tuesday, June 28, 2011 12:13 AM
>To: David Miller
>Cc: Perla, Sathya; netdev@vger.kernel.org
>Subject: [PATCH net-next] benet: convert to 64 bit stats
>
>This changes how the benet driver does statistics:
>  * use 64 bit statistics interface (old api was only 32 bit on 32 bit
>platform)
>  * use u64_stats_sync to ensure atomic 64 bit on 32 bit SMP
>  * only update statistics when needed
>
>Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
>---
>
> drivers/net/benet/be.h      |    4 -
> drivers/net/benet/be_cmds.c |    1
> drivers/net/benet/be_main.c |  161 ++++++++++++++++++++++++----------------
>----
> 3 files changed, 93 insertions(+), 73 deletions(-)

Thanks for the patch!
be_ethtool.c:be_get_ethtool_stats() currently accesses old netdev->stats. This needs to be fixed.
I guess it would be OK to drop netdev stats from being reported via ethtool.
The routine also accesses per-ring rx_bytes/rx_pkts. I'm OK with dropping these too in ethtool for
the scope of this patch as the aggregates are reported in netdev stats.

Can you incorporate the following changes...

diff --git a/drivers/net/benet/be_ethtool.c b/drivers/net/benet/be_ethtool.c
index 30c1386..9a89caa 100644
--- a/drivers/net/benet/be_ethtool.c
+++ b/drivers/net/benet/be_ethtool.c
@@ -26,13 +26,9 @@ struct be_ethtool_stat {
 	int offset;
 };
 
-enum {NETSTAT, DRVSTAT_TX, DRVSTAT_RX, ERXSTAT,
-			DRVSTAT};
+enum {DRVSTAT_TX, DRVSTAT_RX, ERXSTAT, DRVSTAT};
 #define FIELDINFO(_struct, field) FIELD_SIZEOF(_struct, field), \
 					offsetof(_struct, field)
-#define NETSTAT_INFO(field) 	#field, NETSTAT,\
-					FIELDINFO(struct net_device_stats,\
-						field)
 #define DRVSTAT_TX_INFO(field)	#field, DRVSTAT_TX,\
 					FIELDINFO(struct be_tx_stats, field)
 #define DRVSTAT_RX_INFO(field)	#field, DRVSTAT_RX,\
@@ -44,14 +40,6 @@ enum {NETSTAT, DRVSTAT_TX, DRVSTAT_RX, ERXSTAT,
 						field)
 
 static const struct be_ethtool_stat et_stats[] = {
-	{NETSTAT_INFO(rx_packets)},
-	{NETSTAT_INFO(tx_packets)},
-	{NETSTAT_INFO(rx_bytes)},
-	{NETSTAT_INFO(tx_bytes)},
-	{NETSTAT_INFO(rx_errors)},
-	{NETSTAT_INFO(tx_errors)},
-	{NETSTAT_INFO(rx_dropped)},
-	{NETSTAT_INFO(tx_dropped)},
 	{DRVSTAT_INFO(be_tx_events)},
 	{DRVSTAT_INFO(rx_crc_errors)},
 	{DRVSTAT_INFO(rx_alignment_symbol_errors)},
@@ -94,8 +82,6 @@ static const struct be_ethtool_stat et_stats[] = {
 
 /* Stats related to multi RX queues */
 static const struct be_ethtool_stat et_rx_stats[] = {
-	{DRVSTAT_RX_INFO(rx_bytes)},
-	{DRVSTAT_RX_INFO(rx_pkts)},
 	{DRVSTAT_RX_INFO(rx_rate)},
 	{DRVSTAT_RX_INFO(rx_polls)},
 	{DRVSTAT_RX_INFO(rx_events)},
@@ -263,16 +249,7 @@ be_get_ethtool_stats(struct net_device *netdev,
 	int i, j, base;
 
 	for (i = 0; i < ETHTOOL_STATS_NUM; i++) {
-		switch (et_stats[i].type) {
-		case NETSTAT:
-			p = &netdev->stats;
-			break;
-		case DRVSTAT:
-			p = &adapter->drv_stats;
-			break;
-		}
-
-		p = (u8 *)p + et_stats[i].offset;
+		p = (u8 *)&adapter->drv_stats + et_stats[i].offset;
 		data[i] = (et_stats[i].size == sizeof(u64)) ?
 				*(u64 *)p: *(u32 *)p;
 	}
@@ -282,7 +259,7 @@ be_get_ethtool_stats(struct net_device *netdev,
 		for (i = 0; i < ETHTOOL_RXSTATS_NUM; i++) {
 			switch (et_rx_stats[i].type) {
 			case DRVSTAT_RX:
-				p = (u8 *)&rxo->stats + et_rx_stats[i].offset;
+				p = (u8 *)rx_stats(rxo) + et_rx_stats[i].offset;
 				break;
 			case ERXSTAT:
 				p = (u32 *)be_erx_stats_from_cmd(adapter) +
@@ -298,7 +275,7 @@ be_get_ethtool_stats(struct net_device *netdev,
 	base = ETHTOOL_STATS_NUM + adapter->num_rx_qs * ETHTOOL_RXSTATS_NUM;
 	for_all_tx_queues(adapter, txo, j) {
 		for (i = 0; i < ETHTOOL_TXSTATS_NUM; i++) {
-			p = (u8 *)&txo->stats + et_tx_stats[i].offset;
+			p = (u8 *)tx_stats(txo) + et_tx_stats[i].offset;
 			data[base + j * ETHTOOL_TXSTATS_NUM + i] =
 				(et_tx_stats[i].size == sizeof(u64)) ?
 					*(u64 *)p: *(u32 *)p;

Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[David Miller]

From: <Sathya.Perla@Emulex.Com>
Date: Sun, 26 Jun 2011 22:19:13 -0700

> Dave, Sure, I'm fine with that. Could you apply this patch by replacing
> my name with Eric's? Or should I do the same and send you another one...

Please resubmit the entire series with everything fixed, thanks.

RE: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Eric Dumazet]

Le dimanche 26 juin 2011 à 22:19 -0700, Sathya.Perla@Emulex.Com a
écrit :
> >-----Original Message-----
> >From: David Miller [mailto:davem@davemloft.net]
> >Sent: Saturday, June 25, 2011 1:29 AM
> >To: eric.dumazet@gmail.com
> >Cc: Perla, Sathya; netdev@vger.kernel.org
> >Subject: Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update
> >
> >From: Eric Dumazet <eric.dumazet@gmail.com>
> >Date: Fri, 24 Jun 2011 12:32:00 +0200
> >
> >> Hmm, isnt it a patch I provided 10 days ago ?
> >>
> >> I find very strange so few people are able to properly attribute work
> >> today...
> >
> >Sathya, if this patch is almost entriely the same as Eric's patch,
> >and you only made small minor changes, then it isn't your work.
> 
> Dave, Sure, I'm fine with that. Could you apply this patch by replacing
> my name with Eric's? Or should I do the same and send you another one...

Please resend the patch, I am fine with you being the committer, but
please add in Changelog _some_ attribution, like :

Problem initialy reported and fixed by Eric Dumazet.

You see, there is a difference if you at least acknowledge fact that
someone spent some time before you on the problem.

Correct attribution is an incent for people to look at your drivers and
fix their bugs, its a win-win.

RE: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Sathya.Perla]

>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net]
>Sent: Saturday, June 25, 2011 1:29 AM
>To: eric.dumazet@gmail.com
>Cc: Perla, Sathya; netdev@vger.kernel.org
>Subject: Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update
>
>From: Eric Dumazet <eric.dumazet@gmail.com>
>Date: Fri, 24 Jun 2011 12:32:00 +0200
>
>> Hmm, isnt it a patch I provided 10 days ago ?
>>
>> I find very strange so few people are able to properly attribute work
>> today...
>
>Sathya, if this patch is almost entriely the same as Eric's patch,
>and you only made small minor changes, then it isn't your work.

Dave, Sure, I'm fine with that. Could you apply this patch by replacing
my name with Eric's? Or should I do the same and send you another one...

Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 24 Jun 2011 12:32:00 +0200

> Hmm, isnt it a patch I provided 10 days ago ?
> 
> I find very strange so few people are able to properly attribute work
> today...

Sathya, if this patch is almost entriely the same as Eric's patch,
and you only made small minor changes, then it isn't your work.

Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Eric Dumazet]

Le vendredi 24 juin 2011 à 13:43 +0530, Sathya Perla a écrit :
> netdev_stats_update() resets netdev->stats and then accumulates stats from
> various rings. This is wrong as stats readers can sometimes catch zero values.
> Use temporary variables instead for accumulating per-ring values.
> 
> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
> ---
>  drivers/net/benet/be_main.c |   29 +++++++++++++++++------------
>  1 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
> index c4f564c..5ca06b0 100644
> --- a/drivers/net/benet/be_main.c
> +++ b/drivers/net/benet/be_main.c
> @@ -428,33 +428,38 @@ void netdev_stats_update(struct be_adapter *adapter)
>  	struct net_device_stats *dev_stats = &adapter->netdev->stats;
>  	struct be_rx_obj *rxo;
>  	struct be_tx_obj *txo;
> +	unsigned long pkts = 0, bytes = 0, mcast = 0, drops = 0;
>  	int i;
>  
> -	memset(dev_stats, 0, sizeof(*dev_stats));
>  	for_all_rx_queues(adapter, rxo, i) {
> -		dev_stats->rx_packets += rx_stats(rxo)->rx_pkts;
> -		dev_stats->rx_bytes += rx_stats(rxo)->rx_bytes;
> -		dev_stats->multicast += rx_stats(rxo)->rx_mcast_pkts;
> +		pkts += rx_stats(rxo)->rx_pkts;
> +		bytes += rx_stats(rxo)->rx_bytes;
> +		mcast += rx_stats(rxo)->rx_mcast_pkts;
>  		/*  no space in linux buffers: best possible approximation */
>  		if (adapter->generation == BE_GEN3) {
>  			if (!(lancer_chip(adapter))) {
> -				struct be_erx_stats_v1 *erx_stats =
> +				struct be_erx_stats_v1 *erx =
>  					be_erx_stats_from_cmd(adapter);
> -				dev_stats->rx_dropped +=
> -				erx_stats->rx_drops_no_fragments[rxo->q.id];
> +				drops += erx->rx_drops_no_fragments[rxo->q.id];
>  			}
>  		} else {
> -			struct be_erx_stats_v0 *erx_stats =
> +			struct be_erx_stats_v0 *erx =
>  					be_erx_stats_from_cmd(adapter);
> -			dev_stats->rx_dropped +=
> -				erx_stats->rx_drops_no_fragments[rxo->q.id];
> +			drops += erx->rx_drops_no_fragments[rxo->q.id];
>  		}
>  	}
> +	dev_stats->rx_packets = pkts;
> +	dev_stats->rx_bytes = bytes;
> +	dev_stats->multicast = mcast;
> +	dev_stats->rx_dropped = drops;
>  
> +	pkts = bytes = 0;
>  	for_all_tx_queues(adapter, txo, i) {
> -		dev_stats->tx_packets += tx_stats(txo)->be_tx_pkts;
> -		dev_stats->tx_bytes += tx_stats(txo)->be_tx_bytes;
> +		pkts += tx_stats(txo)->be_tx_pkts;
> +		bytes += tx_stats(txo)->be_tx_bytes;
>  	}
> +	dev_stats->tx_packets = pkts;
> +	dev_stats->tx_bytes = bytes;
>  
>  	/* bad pkts received */
>  	dev_stats->rx_errors = drvs->rx_crc_errors +



Hmm, isnt it a patch I provided 10 days ago ?

I find very strange so few people are able to properly attribute work
today...

RE: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Sathya.Perla]

>-----Original Message-----
>From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
>Sent: Friday, June 24, 2011 4:02 PM
>To: Perla, Sathya
>Cc: netdev@vger.kernel.org
>Subject: Re: [PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update
>
>Le vendredi 24 juin 2011 à 13:43 +0530, Sathya Perla a écrit :
>> netdev_stats_update() resets netdev->stats and then accumulates stats from
>> various rings. This is wrong as stats readers can sometimes catch zero
>values.
>> Use temporary variables instead for accumulating per-ring values.
>>
>> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
>> ---

>
>
>Hmm, isnt it a patch I provided 10 days ago ?
>
>I find very strange so few people are able to properly attribute work
>today...
>

Yes, this patch is indeed based on your suggested-fix patch last week.
Sorry, are you referring to the fact that I missed a "Signed-off-by-Eric"?
If so, my bad, I'll resend this patch with that line...

[PATCH net-next-2.6 1/3] be2net: fix netdev_stats_update

[Sathya Perla]

netdev_stats_update() resets netdev->stats and then accumulates stats from
various rings. This is wrong as stats readers can sometimes catch zero values.
Use temporary variables instead for accumulating per-ring values.

Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
 drivers/net/benet/be_main.c |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index c4f564c..5ca06b0 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -428,33 +428,38 @@ void netdev_stats_update(struct be_adapter *adapter)
 	struct net_device_stats *dev_stats = &adapter->netdev->stats;
 	struct be_rx_obj *rxo;
 	struct be_tx_obj *txo;
+	unsigned long pkts = 0, bytes = 0, mcast = 0, drops = 0;
 	int i;
 
-	memset(dev_stats, 0, sizeof(*dev_stats));
 	for_all_rx_queues(adapter, rxo, i) {
-		dev_stats->rx_packets += rx_stats(rxo)->rx_pkts;
-		dev_stats->rx_bytes += rx_stats(rxo)->rx_bytes;
-		dev_stats->multicast += rx_stats(rxo)->rx_mcast_pkts;
+		pkts += rx_stats(rxo)->rx_pkts;
+		bytes += rx_stats(rxo)->rx_bytes;
+		mcast += rx_stats(rxo)->rx_mcast_pkts;
 		/*  no space in linux buffers: best possible approximation */
 		if (adapter->generation == BE_GEN3) {
 			if (!(lancer_chip(adapter))) {
-				struct be_erx_stats_v1 *erx_stats =
+				struct be_erx_stats_v1 *erx =
 					be_erx_stats_from_cmd(adapter);
-				dev_stats->rx_dropped +=
-				erx_stats->rx_drops_no_fragments[rxo->q.id];
+				drops += erx->rx_drops_no_fragments[rxo->q.id];
 			}
 		} else {
-			struct be_erx_stats_v0 *erx_stats =
+			struct be_erx_stats_v0 *erx =
 					be_erx_stats_from_cmd(adapter);
-			dev_stats->rx_dropped +=
-				erx_stats->rx_drops_no_fragments[rxo->q.id];
+			drops += erx->rx_drops_no_fragments[rxo->q.id];
 		}
 	}
+	dev_stats->rx_packets = pkts;
+	dev_stats->rx_bytes = bytes;
+	dev_stats->multicast = mcast;
+	dev_stats->rx_dropped = drops;
 
+	pkts = bytes = 0;
 	for_all_tx_queues(adapter, txo, i) {
-		dev_stats->tx_packets += tx_stats(txo)->be_tx_pkts;
-		dev_stats->tx_bytes += tx_stats(txo)->be_tx_bytes;
+		pkts += tx_stats(txo)->be_tx_pkts;
+		bytes += tx_stats(txo)->be_tx_bytes;
 	}
+	dev_stats->tx_packets = pkts;
+	dev_stats->tx_bytes = bytes;
 
 	/* bad pkts received */
 	dev_stats->rx_errors = drvs->rx_crc_errors +
-- 
1.7.4