[PATCH] b44: add 64 bit stats

From: Kevin Groeneveld <redacted>

Add support for 64 bit stats to Broadcom b44 ethernet driver.

Signed-off-by: Kevin Groeneveld <redacted>
---
 drivers/net/ethernet/broadcom/b44.c |   96 ++++++++++++++++++++---------------
 drivers/net/ethernet/broadcom/b44.h |    3 +-
 2 files changed, 58 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/b44.c

b/drivers/net/ethernet/broadcom/b44.c
index 46b8b7d..c799b5c 100644

--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c

@@ -483,9 +483,11 @@ out:
 static void b44_stats_update(struct b44 *bp)
 {
 	unsigned long reg;
-	u32 *val;
+	u64 *val;

 	val = &bp->hw_stats.tx_good_octets;
+	u64_stats_update_begin(&bp->hw_stats.syncp);
+
 	for (reg = B44_TX_GOOD_O; reg <= B44_TX_PAUSE; reg += 4UL) {
 		*val++ += br32(bp, reg);
 	}

@@ -496,6 +498,8 @@ static void b44_stats_update(struct b44 *bp)
 	for (reg = B44_RX_GOOD_O; reg <= B44_RX_NPAUSE; reg += 4UL) {
 		*val++ += br32(bp, reg);
 	}
+
+	u64_stats_update_end(&bp->hw_stats.syncp);
 }

 static void b44_link_report(struct b44 *bp)

@@ -1635,44 +1639,49 @@ static int b44_close(struct net_device *dev)
 	return 0;
 }

-static struct net_device_stats *b44_get_stats(struct net_device *dev)
+static struct rtnl_link_stats64 *b44_get_stats64(struct net_device *dev,
+					struct rtnl_link_stats64 *nstat)
 {
 	struct b44 *bp = netdev_priv(dev);
-	struct net_device_stats *nstat = &dev->stats;
 	struct b44_hw_stats *hwstat = &bp->hw_stats;
-
-	/* Convert HW stats into netdevice stats. */
-	nstat->rx_packets = hwstat->rx_pkts;
-	nstat->tx_packets = hwstat->tx_pkts;
-	nstat->rx_bytes   = hwstat->rx_octets;
-	nstat->tx_bytes   = hwstat->tx_octets;
-	nstat->tx_errors  = (hwstat->tx_jabber_pkts +
-			     hwstat->tx_oversize_pkts +
-			     hwstat->tx_underruns +
-			     hwstat->tx_excessive_cols +
-			     hwstat->tx_late_cols);
-	nstat->multicast  = hwstat->tx_multicast_pkts;
-	nstat->collisions = hwstat->tx_total_cols;
-
-	nstat->rx_length_errors = (hwstat->rx_oversize_pkts +
-				   hwstat->rx_undersize);
-	nstat->rx_over_errors   = hwstat->rx_missed_pkts;
-	nstat->rx_frame_errors  = hwstat->rx_align_errs;
-	nstat->rx_crc_errors    = hwstat->rx_crc_errs;
-	nstat->rx_errors        = (hwstat->rx_jabber_pkts +
-				   hwstat->rx_oversize_pkts +
-				   hwstat->rx_missed_pkts +
-				   hwstat->rx_crc_align_errs +
-				   hwstat->rx_undersize +
-				   hwstat->rx_crc_errs +
-				   hwstat->rx_align_errs +
-				   hwstat->rx_symbol_errs);
-
-	nstat->tx_aborted_errors = hwstat->tx_underruns;
+	unsigned int start;
+
+	do {
+		start = u64_stats_fetch_begin(&hwstat->syncp);
+
+		/* Convert HW stats into rtnl_link_stats64 stats. */
+		nstat->rx_packets = hwstat->rx_pkts;
+		nstat->tx_packets = hwstat->tx_pkts;
+		nstat->rx_bytes   = hwstat->rx_octets;
+		nstat->tx_bytes   = hwstat->tx_octets;
+		nstat->tx_errors  = (hwstat->tx_jabber_pkts +
+				     hwstat->tx_oversize_pkts +
+				     hwstat->tx_underruns +
+				     hwstat->tx_excessive_cols +
+				     hwstat->tx_late_cols);
+		nstat->multicast  = hwstat->tx_multicast_pkts;
+		nstat->collisions = hwstat->tx_total_cols;
+
+		nstat->rx_length_errors = (hwstat->rx_oversize_pkts +
+					   hwstat->rx_undersize);
+		nstat->rx_over_errors   = hwstat->rx_missed_pkts;
+		nstat->rx_frame_errors  = hwstat->rx_align_errs;
+		nstat->rx_crc_errors    = hwstat->rx_crc_errs;
+		nstat->rx_errors        = (hwstat->rx_jabber_pkts +
+					   hwstat->rx_oversize_pkts +
+					   hwstat->rx_missed_pkts +
+					   hwstat->rx_crc_align_errs +
+					   hwstat->rx_undersize +
+					   hwstat->rx_crc_errs +
+					   hwstat->rx_align_errs +
+					   hwstat->rx_symbol_errs);
+
+		nstat->tx_aborted_errors = hwstat->tx_underruns;
 #if 0
-	/* Carrier lost counter seems to be broken for some devices */
-	nstat->tx_carrier_errors = hwstat->tx_carrier_lost;
+		/* Carrier lost counter seems to be broken for some devices */
+		nstat->tx_carrier_errors = hwstat->tx_carrier_lost;
 #endif
+	} while (u64_stats_fetch_retry(&hwstat->syncp, start));

 	return nstat;
 }

@@ -1993,17 +2002,24 @@ static void b44_get_ethtool_stats(struct

net_device *dev,
 				  struct ethtool_stats *stats, u64 *data)
 {
 	struct b44 *bp = netdev_priv(dev);
-	u32 *val = &bp->hw_stats.tx_good_octets;
+	struct b44_hw_stats *hwstat = &bp->hw_stats;
+	u64 *data_src, *data_dst;
+	unsigned int start;
 	u32 i;

 	spin_lock_irq(&bp->lock);
-
 	b44_stats_update(bp);
+	spin_unlock_irq(&bp->lock);

-	for (i = 0; i < ARRAY_SIZE(b44_gstrings); i++)
-		*data++ = *val++;
+	do {
+		data_src = &hwstat->tx_good_octets;
+		data_dst = data;
+		start = u64_stats_fetch_begin(&hwstat->syncp);

-	spin_unlock_irq(&bp->lock);
+		for (i = 0; i < ARRAY_SIZE(b44_gstrings); i++)
+			*data_dst++ = *data_src++;
+
+	} while (u64_stats_fetch_retry(&hwstat->syncp, start));
 }

 static void b44_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)

@@ -2113,7 +2129,7 @@ static const struct net_device_ops b44_netdev_ops = {
 	.ndo_open		= b44_open,
 	.ndo_stop		= b44_close,
 	.ndo_start_xmit		= b44_start_xmit,
-	.ndo_get_stats		= b44_get_stats,
+	.ndo_get_stats64	= b44_get_stats64,
 	.ndo_set_rx_mode	= b44_set_rx_mode,
 	.ndo_set_mac_address	= b44_set_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,

diff --git a/drivers/net/ethernet/broadcom/b44.h

b/drivers/net/ethernet/broadcom/b44.h
index e1905a4..8993d72 100644

--- a/drivers/net/ethernet/broadcom/b44.h
+++ b/drivers/net/ethernet/broadcom/b44.h

@@ -338,9 +338,10 @@ struct ring_info {
  * the layout
  */
 struct b44_hw_stats {
-#define _B44(x)	u32 x;
+#define _B44(x)	u64 x;
 B44_STAT_REG_DECLARE
 #undef _B44
+	struct u64_stats_sync	syncp;
 };

 struct ssb_device;

On Sat, 2012-07-14 at 21:51 -0400, Kevin Groeneveld wrote:

b44_stats_update() is run from timer, (softirq), check b44_timer()

So you must use u64_stats_fetch_begin_bh()


Because on 32bit, uniprocessor, u64_stats_fetch_begin() only disables
preemption. (there is no seqlock in syncp)

So softirq are allowed to interrupt you and corrupt your stats while you
read them, and you dont notice you have to retry.

+
+		/* Convert HW stats into rtnl_link_stats64 stats. */
+		nstat->rx_packets = hwstat->rx_pkts;
+		nstat->tx_packets = hwstat->tx_pkts;
+		nstat->rx_bytes   = hwstat->rx_octets;
+		nstat->tx_bytes   = hwstat->tx_octets;
+		nstat->tx_errors  = (hwstat->tx_jabber_pkts +
+				     hwstat->tx_oversize_pkts +
+				     hwstat->tx_underruns +
+				     hwstat->tx_excessive_cols +
+				     hwstat->tx_late_cols);
+		nstat->multicast  = hwstat->tx_multicast_pkts;
+		nstat->collisions = hwstat->tx_total_cols;
+
+		nstat->rx_length_errors = (hwstat->rx_oversize_pkts +
+					   hwstat->rx_undersize);
+		nstat->rx_over_errors   = hwstat->rx_missed_pkts;
+		nstat->rx_frame_errors  = hwstat->rx_align_errs;
+		nstat->rx_crc_errors    = hwstat->rx_crc_errs;
+		nstat->rx_errors        = (hwstat->rx_jabber_pkts +
+					   hwstat->rx_oversize_pkts +
+					   hwstat->rx_missed_pkts +
+					   hwstat->rx_crc_align_errs +
+					   hwstat->rx_undersize +
+					   hwstat->rx_crc_errs +
+					   hwstat->rx_align_errs +
+					   hwstat->rx_symbol_errs);
+
+		nstat->tx_aborted_errors = hwstat->tx_underruns;
 #if 0
-	/* Carrier lost counter seems to be broken for some devices */
-	nstat->tx_carrier_errors = hwstat->tx_carrier_lost;
+		/* Carrier lost counter seems to be broken for some devices */
+		nstat->tx_carrier_errors = hwstat->tx_carrier_lost;
 #endif
+	} while (u64_stats_fetch_retry(&hwstat->syncp, start));

u64_stats_fetch_retry_bh()

Hi Eric,

Thanks for the feedback on my patch.

On Sun, Jul 15, 2012 at 3:26 AM, Eric Dumazet [off-list ref] wrote:

b44_stats_update() is run from timer, (softirq), check b44_timer()

Yes, I knew that.  b44_stats_update() is also called from
b44_get_ethtool_stats().

So you must use u64_stats_fetch_begin_bh()

That I did not know, although it makes sense after I read more about
it.  I will submit a new patch with this fixed.


Kevin

From: Kevin Groeneveld <redacted>

Add support for 64 bit stats to Broadcom b44 ethernet driver.

Signed-off-by: Kevin Groeneveld <redacted>
---
v2: use u64_stats_fetch_begin_bh/u64_stats_fetch_retry_bh instead of
    u64_stats_fetch_begin/u64_stats_fetch_retry as stats update happens in a
    timer interrupt

 drivers/net/ethernet/broadcom/b44.c |   96 ++++++++++++++++++++---------------
 drivers/net/ethernet/broadcom/b44.h |    3 +-
 2 files changed, 58 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/b44.c

b/drivers/net/ethernet/broadcom/b44.c
index 46b8b7d..30ad4ef 100644

--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c

@@ -483,9 +483,11 @@ out:
 static void b44_stats_update(struct b44 *bp)
 {
 	unsigned long reg;
-	u32 *val;
+	u64 *val;

 	val = &bp->hw_stats.tx_good_octets;
+	u64_stats_update_begin(&bp->hw_stats.syncp);
+
 	for (reg = B44_TX_GOOD_O; reg <= B44_TX_PAUSE; reg += 4UL) {
 		*val++ += br32(bp, reg);
 	}

@@ -496,6 +498,8 @@ static void b44_stats_update(struct b44 *bp)
 	for (reg = B44_RX_GOOD_O; reg <= B44_RX_NPAUSE; reg += 4UL) {
 		*val++ += br32(bp, reg);
 	}
+
+	u64_stats_update_end(&bp->hw_stats.syncp);
 }

 static void b44_link_report(struct b44 *bp)

@@ -1635,44 +1639,49 @@ static int b44_close(struct net_device *dev)
 	return 0;
 }

-static struct net_device_stats *b44_get_stats(struct net_device *dev)
+static struct rtnl_link_stats64 *b44_get_stats64(struct net_device *dev,
+					struct rtnl_link_stats64 *nstat)
 {
 	struct b44 *bp = netdev_priv(dev);
-	struct net_device_stats *nstat = &dev->stats;
 	struct b44_hw_stats *hwstat = &bp->hw_stats;
-
-	/* Convert HW stats into netdevice stats. */
-	nstat->rx_packets = hwstat->rx_pkts;
-	nstat->tx_packets = hwstat->tx_pkts;
-	nstat->rx_bytes   = hwstat->rx_octets;
-	nstat->tx_bytes   = hwstat->tx_octets;
-	nstat->tx_errors  = (hwstat->tx_jabber_pkts +
-			     hwstat->tx_oversize_pkts +
-			     hwstat->tx_underruns +
-			     hwstat->tx_excessive_cols +
-			     hwstat->tx_late_cols);
-	nstat->multicast  = hwstat->tx_multicast_pkts;
-	nstat->collisions = hwstat->tx_total_cols;
-
-	nstat->rx_length_errors = (hwstat->rx_oversize_pkts +
-				   hwstat->rx_undersize);
-	nstat->rx_over_errors   = hwstat->rx_missed_pkts;
-	nstat->rx_frame_errors  = hwstat->rx_align_errs;
-	nstat->rx_crc_errors    = hwstat->rx_crc_errs;
-	nstat->rx_errors        = (hwstat->rx_jabber_pkts +
-				   hwstat->rx_oversize_pkts +
-				   hwstat->rx_missed_pkts +
-				   hwstat->rx_crc_align_errs +
-				   hwstat->rx_undersize +
-				   hwstat->rx_crc_errs +
-				   hwstat->rx_align_errs +
-				   hwstat->rx_symbol_errs);
-
-	nstat->tx_aborted_errors = hwstat->tx_underruns;
+	unsigned int start;
+
+	do {
+		start = u64_stats_fetch_begin_bh(&hwstat->syncp);
+
+		/* Convert HW stats into rtnl_link_stats64 stats. */
+		nstat->rx_packets = hwstat->rx_pkts;
+		nstat->tx_packets = hwstat->tx_pkts;
+		nstat->rx_bytes   = hwstat->rx_octets;
+		nstat->tx_bytes   = hwstat->tx_octets;
+		nstat->tx_errors  = (hwstat->tx_jabber_pkts +
+				     hwstat->tx_oversize_pkts +
+				     hwstat->tx_underruns +
+				     hwstat->tx_excessive_cols +
+				     hwstat->tx_late_cols);
+		nstat->multicast  = hwstat->tx_multicast_pkts;
+		nstat->collisions = hwstat->tx_total_cols;
+
+		nstat->rx_length_errors = (hwstat->rx_oversize_pkts +
+					   hwstat->rx_undersize);
+		nstat->rx_over_errors   = hwstat->rx_missed_pkts;
+		nstat->rx_frame_errors  = hwstat->rx_align_errs;
+		nstat->rx_crc_errors    = hwstat->rx_crc_errs;
+		nstat->rx_errors        = (hwstat->rx_jabber_pkts +
+					   hwstat->rx_oversize_pkts +
+					   hwstat->rx_missed_pkts +
+					   hwstat->rx_crc_align_errs +
+					   hwstat->rx_undersize +
+					   hwstat->rx_crc_errs +
+					   hwstat->rx_align_errs +
+					   hwstat->rx_symbol_errs);
+
+		nstat->tx_aborted_errors = hwstat->tx_underruns;
 #if 0
-	/* Carrier lost counter seems to be broken for some devices */
-	nstat->tx_carrier_errors = hwstat->tx_carrier_lost;
+		/* Carrier lost counter seems to be broken for some devices */
+		nstat->tx_carrier_errors = hwstat->tx_carrier_lost;
 #endif
+	} while (u64_stats_fetch_retry_bh(&hwstat->syncp, start));

 	return nstat;
 }

@@ -1993,17 +2002,24 @@ static void b44_get_ethtool_stats(struct

net_device *dev,
 				  struct ethtool_stats *stats, u64 *data)
 {
 	struct b44 *bp = netdev_priv(dev);
-	u32 *val = &bp->hw_stats.tx_good_octets;
+	struct b44_hw_stats *hwstat = &bp->hw_stats;
+	u64 *data_src, *data_dst;
+	unsigned int start;
 	u32 i;

 	spin_lock_irq(&bp->lock);
-
 	b44_stats_update(bp);
+	spin_unlock_irq(&bp->lock);

-	for (i = 0; i < ARRAY_SIZE(b44_gstrings); i++)
-		*data++ = *val++;
+	do {
+		data_src = &hwstat->tx_good_octets;
+		data_dst = data;
+		start = u64_stats_fetch_begin_bh(&hwstat->syncp);

-	spin_unlock_irq(&bp->lock);
+		for (i = 0; i < ARRAY_SIZE(b44_gstrings); i++)
+			*data_dst++ = *data_src++;
+
+	} while (u64_stats_fetch_retry_bh(&hwstat->syncp, start));
 }

 static void b44_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)

@@ -2113,7 +2129,7 @@ static const struct net_device_ops b44_netdev_ops = {
 	.ndo_open		= b44_open,
 	.ndo_stop		= b44_close,
 	.ndo_start_xmit		= b44_start_xmit,
-	.ndo_get_stats		= b44_get_stats,
+	.ndo_get_stats64	= b44_get_stats64,
 	.ndo_set_rx_mode	= b44_set_rx_mode,
 	.ndo_set_mac_address	= b44_set_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,

diff --git a/drivers/net/ethernet/broadcom/b44.h

b/drivers/net/ethernet/broadcom/b44.h
index e1905a4..8993d72 100644

--- a/drivers/net/ethernet/broadcom/b44.h
+++ b/drivers/net/ethernet/broadcom/b44.h

@@ -338,9 +338,10 @@ struct ring_info {
  * the layout
  */
 struct b44_hw_stats {
-#define _B44(x)	u32 x;
+#define _B44(x)	u64 x;
 B44_STAT_REG_DECLARE
 #undef _B44
+	struct u64_stats_sync	syncp;
 };

 struct ssb_device;

On Sun, 2012-07-15 at 14:00 -0400, Kevin Groeneveld wrote:

From: Kevin Groeneveld <redacted>

Add support for 64 bit stats to Broadcom b44 ethernet driver.

Signed-off-by: Kevin Groeneveld <redacted>
---
v2: use u64_stats_fetch_begin_bh/u64_stats_fetch_retry_bh instead of
    u64_stats_fetch_begin/u64_stats_fetch_retry as stats update happens in a
    timer interrupt

 drivers/net/ethernet/broadcom/b44.c |   96 ++++++++++++++++++++---------------
 drivers/net/ethernet/broadcom/b44.h |    3 +-
 2 files changed, 58 insertions(+), 41 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

From: Kevin Groeneveld <redacted>
Date: Sun, 15 Jul 2012 14:00:28 -0400

From: Kevin Groeneveld <redacted>

Add support for 64 bit stats to Broadcom b44 ethernet driver.

Signed-off-by: Kevin Groeneveld <redacted>

This patch was corrupted by your email client and is therefore
unusable.

On Tue, Jul 17, 2012 at 2:08 AM, David Miller [off-list ref] wrote:

This patch was corrupted by your email client and is therefore
unusable.

If I resend the patch should I bump the version number in the subject?

Should I include "Acked-by" lines that people have posted?

I keep sending myself test messages with the patch but the white space
is always mangled.  I am not sure if Thunderbird is mangling it in the
sent message or the received message... :(

On Tue, 2012-07-17 at 22:02 -0400, Kevin Groeneveld wrote:

On Tue, Jul 17, 2012 at 2:08 AM, David Miller [off-list ref] wrote:

quoted

This patch was corrupted by your email client and is therefore
unusable.

If I resend the patch should I bump the version number in the subject?

It doesnt matter in this case, its a formatting issue

Should I include "Acked-by" lines that people have posted?

You can if no semantic change is done

I keep sending myself test messages with the patch but the white space
is always mangled.  I am not sure if Thunderbird is mangling it in the
sent message or the received message... :(

Documentation/email-clients.txt

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Thunderbird (GUI)

Thunderbird is an Outlook clone that likes to mangle text, but there are ways
to coerce it into behaving.

- Allows use of an external editor:
  The easiest thing to do with Thunderbird and patches is to use an
  "external editor" extension and then just use your favorite $EDITOR
  for reading/merging patches into the body text.  To do this, download
  and install the extension, then add a button for it using
  View->Toolbars->Customize... and finally just click on it when in the
  Compose dialog.

To beat some sense out of the internal editor, do this:

- Edit your Thunderbird config settings so that it won't use format=flowed.
  Go to "edit->preferences->advanced->config editor" to bring up the
  thunderbird's registry editor, and set "mailnews.send_plaintext_flowed" to
  "false".

- Disable HTML Format: Set "mail.identity.id1.compose_html" to "false".

- Enable "preformat" mode: Set "editor.quotesPreformatted" to "true".

- Enable UTF8: Set "prefs.converted-to-utf8" to "true".

- Install the "toggle wordwrap" extension.  Download the file from:
    https://addons.mozilla.org/thunderbird/addon/2351/
  Then go to "tools->add ons", select "install" at the bottom of the screen,
  and browse to where you saved the .xul file.  This adds an "Enable
  Wordwrap" entry under the Options menu of the message composer.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

From: Kevin Groeneveld <redacted>

Add support for 64 bit stats to Broadcom b44 ethernet driver.

Signed-off-by: Kevin Groeneveld <redacted>
---
v2: use u64_stats_fetch_begin_bh/u64_stats_fetch_retry_bh instead of
    u64_stats_fetch_begin/u64_stats_fetch_retry as stats update happens in a
    timer interrupt

 drivers/net/ethernet/broadcom/b44.c |   96 ++++++++++++++++++++---------------
 drivers/net/ethernet/broadcom/b44.h |    3 +-
 2 files changed, 58 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index d09c6b5..9786c0e 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c

@@ -483,9 +483,11 @@ out:
 static void b44_stats_update(struct b44 *bp)
 {
 	unsigned long reg;
-	u32 *val;
+	u64 *val;
 
 	val = &bp->hw_stats.tx_good_octets;
+	u64_stats_update_begin(&bp->hw_stats.syncp);
+
 	for (reg = B44_TX_GOOD_O; reg <= B44_TX_PAUSE; reg += 4UL) {
 		*val++ += br32(bp, reg);
 	}

@@ -496,6 +498,8 @@ static void b44_stats_update(struct b44 *bp)
 	for (reg = B44_RX_GOOD_O; reg <= B44_RX_NPAUSE; reg += 4UL) {
 		*val++ += br32(bp, reg);
 	}
+
+	u64_stats_update_end(&bp->hw_stats.syncp);
 }
 
 static void b44_link_report(struct b44 *bp)

@@ -1635,44 +1639,49 @@ static int b44_close(struct net_device *dev)
 	return 0;
 }
 
-static struct net_device_stats *b44_get_stats(struct net_device *dev)
+static struct rtnl_link_stats64 *b44_get_stats64(struct net_device *dev,
+					struct rtnl_link_stats64 *nstat)
 {
 	struct b44 *bp = netdev_priv(dev);
-	struct net_device_stats *nstat = &dev->stats;
 	struct b44_hw_stats *hwstat = &bp->hw_stats;
-
-	/* Convert HW stats into netdevice stats. */
-	nstat->rx_packets = hwstat->rx_pkts;
-	nstat->tx_packets = hwstat->tx_pkts;
-	nstat->rx_bytes   = hwstat->rx_octets;
-	nstat->tx_bytes   = hwstat->tx_octets;
-	nstat->tx_errors  = (hwstat->tx_jabber_pkts +
-			     hwstat->tx_oversize_pkts +
-			     hwstat->tx_underruns +
-			     hwstat->tx_excessive_cols +
-			     hwstat->tx_late_cols);
-	nstat->multicast  = hwstat->tx_multicast_pkts;
-	nstat->collisions = hwstat->tx_total_cols;
-
-	nstat->rx_length_errors = (hwstat->rx_oversize_pkts +
-				   hwstat->rx_undersize);
-	nstat->rx_over_errors   = hwstat->rx_missed_pkts;
-	nstat->rx_frame_errors  = hwstat->rx_align_errs;
-	nstat->rx_crc_errors    = hwstat->rx_crc_errs;
-	nstat->rx_errors        = (hwstat->rx_jabber_pkts +
-				   hwstat->rx_oversize_pkts +
-				   hwstat->rx_missed_pkts +
-				   hwstat->rx_crc_align_errs +
-				   hwstat->rx_undersize +
-				   hwstat->rx_crc_errs +
-				   hwstat->rx_align_errs +
-				   hwstat->rx_symbol_errs);
-
-	nstat->tx_aborted_errors = hwstat->tx_underruns;
+	unsigned int start;
+
+	do {
+		start = u64_stats_fetch_begin_bh(&hwstat->syncp);
+
+		/* Convert HW stats into rtnl_link_stats64 stats. */
+		nstat->rx_packets = hwstat->rx_pkts;
+		nstat->tx_packets = hwstat->tx_pkts;
+		nstat->rx_bytes   = hwstat->rx_octets;
+		nstat->tx_bytes   = hwstat->tx_octets;
+		nstat->tx_errors  = (hwstat->tx_jabber_pkts +
+				     hwstat->tx_oversize_pkts +
+				     hwstat->tx_underruns +
+				     hwstat->tx_excessive_cols +
+				     hwstat->tx_late_cols);
+		nstat->multicast  = hwstat->tx_multicast_pkts;
+		nstat->collisions = hwstat->tx_total_cols;
+
+		nstat->rx_length_errors = (hwstat->rx_oversize_pkts +
+					   hwstat->rx_undersize);
+		nstat->rx_over_errors   = hwstat->rx_missed_pkts;
+		nstat->rx_frame_errors  = hwstat->rx_align_errs;
+		nstat->rx_crc_errors    = hwstat->rx_crc_errs;
+		nstat->rx_errors        = (hwstat->rx_jabber_pkts +
+					   hwstat->rx_oversize_pkts +
+					   hwstat->rx_missed_pkts +
+					   hwstat->rx_crc_align_errs +
+					   hwstat->rx_undersize +
+					   hwstat->rx_crc_errs +
+					   hwstat->rx_align_errs +
+					   hwstat->rx_symbol_errs);
+
+		nstat->tx_aborted_errors = hwstat->tx_underruns;
 #if 0
-	/* Carrier lost counter seems to be broken for some devices */
-	nstat->tx_carrier_errors = hwstat->tx_carrier_lost;
+		/* Carrier lost counter seems to be broken for some devices */
+		nstat->tx_carrier_errors = hwstat->tx_carrier_lost;
 #endif
+	} while (u64_stats_fetch_retry_bh(&hwstat->syncp, start));
 
 	return nstat;
 }

@@ -1993,17 +2002,24 @@ static void b44_get_ethtool_stats(struct net_device *dev,
 				  struct ethtool_stats *stats, u64 *data)
 {
 	struct b44 *bp = netdev_priv(dev);
-	u32 *val = &bp->hw_stats.tx_good_octets;
+	struct b44_hw_stats *hwstat = &bp->hw_stats;
+	u64 *data_src, *data_dst;
+	unsigned int start;
 	u32 i;
 
 	spin_lock_irq(&bp->lock);
-
 	b44_stats_update(bp);
+	spin_unlock_irq(&bp->lock);
 
-	for (i = 0; i < ARRAY_SIZE(b44_gstrings); i++)
-		*data++ = *val++;
+	do {
+		data_src = &hwstat->tx_good_octets;
+		data_dst = data;
+		start = u64_stats_fetch_begin_bh(&hwstat->syncp);
 
-	spin_unlock_irq(&bp->lock);
+		for (i = 0; i < ARRAY_SIZE(b44_gstrings); i++)
+			*data_dst++ = *data_src++;
+
+	} while (u64_stats_fetch_retry_bh(&hwstat->syncp, start));
 }
 
 static void b44_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)

@@ -2113,7 +2129,7 @@ static const struct net_device_ops b44_netdev_ops = {
 	.ndo_open		= b44_open,
 	.ndo_stop		= b44_close,
 	.ndo_start_xmit		= b44_start_xmit,
-	.ndo_get_stats		= b44_get_stats,
+	.ndo_get_stats64	= b44_get_stats64,
 	.ndo_set_rx_mode	= b44_set_rx_mode,
 	.ndo_set_mac_address	= b44_set_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,

diff --git a/drivers/net/ethernet/broadcom/b44.h b/drivers/net/ethernet/broadcom/b44.h
index e1905a4..8993d72 100644
--- a/drivers/net/ethernet/broadcom/b44.h
+++ b/drivers/net/ethernet/broadcom/b44.h

@@ -338,9 +338,10 @@ struct ring_info {
  * the layout
  */
 struct b44_hw_stats {
-#define _B44(x)	u32 x;
+#define _B44(x)	u64 x;
 B44_STAT_REG_DECLARE
 #undef _B44
+	struct u64_stats_sync	syncp;
 };
 
 struct ssb_device;

-- 
1.7.9.5

On Tue, 2012-07-17 at 23:46 -0400, Kevin Groeneveld wrote:

From: Kevin Groeneveld <redacted>

Add support for 64 bit stats to Broadcom b44 ethernet driver.

Signed-off-by: Kevin Groeneveld <redacted>
---
v2: use u64_stats_fetch_begin_bh/u64_stats_fetch_retry_bh instead of
    u64_stats_fetch_begin/u64_stats_fetch_retry as stats update happens in a
    timer interrupt

Seems good this time, thanks

Signed-off-by: Eric Dumazet <edumazet@google.com>

From: Eric Dumazet <redacted>
Date: Wed, 18 Jul 2012 05:50:02 +0200

On Tue, 2012-07-17 at 23:46 -0400, Kevin Groeneveld wrote:

quoted

From: Kevin Groeneveld <redacted>

Add support for 64 bit stats to Broadcom b44 ethernet driver.

Signed-off-by: Kevin Groeneveld <redacted>
---
v2: use u64_stats_fetch_begin_bh/u64_stats_fetch_retry_bh instead of
    u64_stats_fetch_begin/u64_stats_fetch_retry as stats update happens in a
    timer interrupt

Seems good this time, thanks

Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks.

Hi Eric,

On Sun, Jul 15, 2012 at 3:26 AM, Eric Dumazet [off-list ref] wrote:

So you must use u64_stats_fetch_begin_bh()

Because on 32bit, uniprocessor, u64_stats_fetch_begin() only disables
preemption. (there is no seqlock in syncp)

So softirq are allowed to interrupt you and corrupt your stats while you
read them, and you dont notice you have to retry.

I am still trying to make sure I understand this fully.  I want to
update some other drivers with 64 bit stats as well.  What you said
seems to make sense, but...

I was looking at the virtio_net.c driver.  One spot in this driver
which updates the stats is the receive_buf function.  recive_buf is
called from virtnet_poll which is registered as a napi poll function.
According to Documentation/networking/netdevices.txt the poll function
is called in a softirq context.  However, the function which reads the
stats uses u64_stats_fetch_begin/u64_stats_fetch_retry.  Shouldn't
this be u64_stats_fetch_begin_bh/u64_stats_fetch_retry_bh for the
exact reasons you described for my b44 patch?


Kevin

On Thu, 2012-07-19 at 21:56 -0400, Kevin Groeneveld wrote:

I am still trying to make sure I understand this fully.  I want to
update some other drivers with 64 bit stats as well.  What you said
seems to make sense, but...

I was looking at the virtio_net.c driver.  One spot in this driver
which updates the stats is the receive_buf function.  recive_buf is
called from virtnet_poll which is registered as a napi poll function.
According to Documentation/networking/netdevices.txt the poll function
is called in a softirq context.  However, the function which reads the
stats uses u64_stats_fetch_begin/u64_stats_fetch_retry.  Shouldn't
this be u64_stats_fetch_begin_bh/u64_stats_fetch_retry_bh for the
exact reasons you described for my b44 patch?

Absolutely. You can argue that probably nobody use this driver on a
32bit UP machine, but technically speaking the current implementation is
racy.

On Fri, 2012-07-20 at 06:53 +0200, Eric Dumazet wrote:

On Thu, 2012-07-19 at 21:56 -0400, Kevin Groeneveld wrote:

quoted

I am still trying to make sure I understand this fully.  I want to
update some other drivers with 64 bit stats as well.  What you said
seems to make sense, but...

I was looking at the virtio_net.c driver.  One spot in this driver
which updates the stats is the receive_buf function.  recive_buf is
called from virtnet_poll which is registered as a napi poll function.
According to Documentation/networking/netdevices.txt the poll function
is called in a softirq context.  However, the function which reads the
stats uses u64_stats_fetch_begin/u64_stats_fetch_retry.  Shouldn't
this be u64_stats_fetch_begin_bh/u64_stats_fetch_retry_bh for the
exact reasons you described for my b44 patch?

Absolutely. You can argue that probably nobody use this driver on a
32bit UP machine, but technically speaking the current implementation is
racy.

In fact all network drivers should use the _bh version.

Could you send a patch for all of them, based on net-next tree ?

Thanks !

On Fri, 2012-07-20 at 07:24 +0200, Eric Dumazet wrote:

On Fri, 2012-07-20 at 06:53 +0200, Eric Dumazet wrote:

quoted

On Thu, 2012-07-19 at 21:56 -0400, Kevin Groeneveld wrote:

quoted

I am still trying to make sure I understand this fully.  I want to
update some other drivers with 64 bit stats as well.  What you said
seems to make sense, but...

I was looking at the virtio_net.c driver.  One spot in this driver
which updates the stats is the receive_buf function.  recive_buf is
called from virtnet_poll which is registered as a napi poll function.
According to Documentation/networking/netdevices.txt the poll function
is called in a softirq context.  However, the function which reads the
stats uses u64_stats_fetch_begin/u64_stats_fetch_retry.  Shouldn't
this be u64_stats_fetch_begin_bh/u64_stats_fetch_retry_bh for the
exact reasons you described for my b44 patch?

Absolutely. You can argue that probably nobody use this driver on a
32bit UP machine, but technically speaking the current implementation is
racy.

In fact all network drivers should use the _bh version.

Could you send a patch for all of them, based on net-next tree ?

Thanks !

Don't we need an _irq variant for drivers that support netpoll?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

On Fri, 2012-07-20 at 15:33 +0100, Ben Hutchings wrote:

Don't we need an _irq variant for drivers that support netpoll?

netpoll is such a hack I would not bother having a 0.000001 % chance to
get a "about to be wrapped 64bit counter" on a 32bit cpu.

On Fri, Jul 20, 2012 at 1:24 AM, Eric Dumazet [off-list ref] wrote:

quoted

Absolutely. You can argue that probably nobody use this driver on a
32bit UP machine, but technically speaking the current implementation is
racy.

In fact all network drivers should use the _bh version.

Okay, thanks for clarifying.

Could you send a patch for all of them, based on net-next tree ?

Sure, I can work on that.  It should be a relatively easy thing to
update.  I can probably send a patch within the next couple days.


Kevin

On Fri, Jul 20, 2012 at 2:56 PM, Kevin Groeneveld [off-list ref] wrote:

quoted

In fact all network drivers should use the _bh version.
Could you send a patch for all of them, based on net-next tree ?

Sure, I can work on that.  It should be a relatively easy thing to
update.  I can probably send a patch within the next couple days.

As I have been working on the patch I have been trying convince myself
that each case I change actually needs the _bh version of the
functions instead of blindly changing them.  So far I have found the
following where the change seems to make sense:

drivers/net/dummy.c
drivers/net/ethernet/neterion/vxge/vxge-main.c
drivers/net/loopback.c
drivers/net/virtio_net.c
net/bridge/br_device.c

The only two other places in the networking code that use
u64_stats_fetch_begin/u64_stats_fetch_retry are:

net/l2tp/l2tp_netlink.c
net/netfilter/ipvs/ip_vs_est.c

Do these need to be updated as well?  Looking at these files quickly
and with my limited knowledge of the kernel I am not sure if they
update the stats in a BH context or not.


Kevin

On Fri, 2012-07-20 at 22:22 -0400, Kevin Groeneveld wrote:

On Fri, Jul 20, 2012 at 2:56 PM, Kevin Groeneveld [off-list ref] wrote:

quoted

quoted

In fact all network drivers should use the _bh version.
Could you send a patch for all of them, based on net-next tree ?

Sure, I can work on that.  It should be a relatively easy thing to
update.  I can probably send a patch within the next couple days.

As I have been working on the patch I have been trying convince myself
that each case I change actually needs the _bh version of the
functions instead of blindly changing them.  So far I have found the
following where the change seems to make sense:

drivers/net/dummy.c
drivers/net/ethernet/neterion/vxge/vxge-main.c
drivers/net/loopback.c
drivers/net/virtio_net.c
net/bridge/br_device.c

Thats right.

The only two other places in the networking code that use
u64_stats_fetch_begin/u64_stats_fetch_retry are:

net/l2tp/l2tp_netlink.c

This one is completely buggy, dont waste your time on it.

My plan for this one : dont try to have 64bit stats on 32bit arches, and
use plain "unsigned long" counters (if they are percpu), or
atomic_long_t (if they are shared by all cpus)

The writer sides might be run concurrently by several cpus, so
u64_stats_update_begin(&sstats->syncp); are racy : a reader can
be trapped forever.

net/netfilter/ipvs/ip_vs_est.c

Same problem for this one, I think.

I CCed ipvs maintainers so that they can take a look.

Do these need to be updated as well?  Looking at these files quickly
and with my limited knowledge of the kernel I am not sure if they
update the stats in a BH context or not.


Kevin

Thanks !

	Hello,

On Sat, 21 Jul 2012, Eric Dumazet wrote:

The writer sides might be run concurrently by several cpus, so
u64_stats_update_begin(&sstats->syncp); are racy : a reader can
be trapped forever.

quoted

net/netfilter/ipvs/ip_vs_est.c

Same problem for this one, I think.

I CCed ipvs maintainers so that they can take a look.

	IPVS moved to percpu counters, i.e. even on 32-bit SMP
we do not use locks to protect the seqcounter:

commit b17fc9963f837ef1acfe36e193108fb16ed58647
Author: Hans Schillstrom [off-list ref]
Date:   Mon Jan 3 14:44:56 2011 +0100

    IPVS: netns, ip_vs_stats and its procfs

quoted

Do these need to be updated as well?  Looking at these files quickly
and with my limited knowledge of the kernel I am not sure if they
update the stats in a BH context or not.

	We have 2 kinds of readers:

- timer context (ip_vs_est.c): no _bh is used for fetch
- user context (ip_vs_ctl.c): _bh is used for fetch

quoted

Kevin

Thanks !

Regards

--
Julian Anastasov [off-list ref]

On Sat, Jul 21, 2012 at 1:09 AM, Eric Dumazet [off-list ref] wrote:

quoted

drivers/net/dummy.c
drivers/net/ethernet/neterion/vxge/vxge-main.c
drivers/net/loopback.c
drivers/net/virtio_net.c
net/bridge/br_device.c

Thats right.

I just submitted a patch for these five drivers.  I don't want to mess
with the other two when I don't fully know what I am doing.


Kevin