[PATCH] b44: add 64 bit stats

[PATCH] b44: add 64 bit stats

[Kevin Groeneveld]

From: Kevin Groeneveld <kgroeneveld@gmail.com>

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

Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>
---
 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;

Re: [PATCH] b44: add 64 bit stats

[Eric Dumazet]

On Sat, 2012-07-14 at 21:51 -0400, Kevin Groeneveld wrote:
> From: Kevin Groeneveld <kgroeneveld@gmail.com>
> 
> Add support for 64 bit stats to Broadcom b44 ethernet driver.
> 
> Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>
> ---
>  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:

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

>  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)
>  {
>  
> -	nstat->tx_aborted_errors = hwstat->tx_underruns;
> +	unsigned int start;
> +
> +	do {
> +		start = u64_stats_fetch_begin(&hwstat->syncp);

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()

Re: [PATCH] b44: add 64 bit stats

[Kevin Groeneveld]

Hi Eric,

Thanks for the feedback on my patch.

On Sun, Jul 15, 2012 at 3:26 AM, Eric Dumazet <eric.dumazet@gmail.com> 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

[PATCH v2] b44: add 64 bit stats

[Kevin Groeneveld]

From: Kevin Groeneveld <kgroeneveld@gmail.com>

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

Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>
---
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;

Re: [PATCH v2] b44: add 64 bit stats

[Eric Dumazet]

On Sun, 2012-07-15 at 14:00 -0400, Kevin Groeneveld wrote:
> From: Kevin Groeneveld <kgroeneveld@gmail.com>
> 
> Add support for 64 bit stats to Broadcom b44 ethernet driver.
> 
> Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>
> ---
> 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>

Re: [PATCH v2] b44: add 64 bit stats

[David Miller]

From: Kevin Groeneveld <kgroeneveld@gmail.com>
Date: Sun, 15 Jul 2012 14:00:28 -0400

> From: Kevin Groeneveld <kgroeneveld@gmail.com>
> 
> Add support for 64 bit stats to Broadcom b44 ethernet driver.
> 
> Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>

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

Re: [PATCH v2] b44: add 64 bit stats

[Kevin Groeneveld]

On Tue, Jul 17, 2012 at 2:08 AM, David Miller <davem@davemloft.net> 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... :(

Re: [PATCH v2] b44: add 64 bit stats

[Eric Dumazet]

On Tue, 2012-07-17 at 22:02 -0400, Kevin Groeneveld wrote:
> On Tue, Jul 17, 2012 at 2:08 AM, David Miller <davem@davemloft.net> 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?
> 
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.

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

Re: [PATCH v2] b44: add 64 bit stats

[Kevin Groeneveld]

From: Kevin Groeneveld <kgroeneveld@gmail.com>

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

Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>
---
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

Re: [PATCH v2] b44: add 64 bit stats

[Eric Dumazet]

On Tue, 2012-07-17 at 23:46 -0400, Kevin Groeneveld wrote:
> From: Kevin Groeneveld <kgroeneveld@gmail.com>
> 
> Add support for 64 bit stats to Broadcom b44 ethernet driver.
> 
> Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>
> ---
> 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>

Re: [PATCH v2] b44: add 64 bit stats

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 18 Jul 2012 05:50:02 +0200

> On Tue, 2012-07-17 at 23:46 -0400, Kevin Groeneveld wrote:
>> From: Kevin Groeneveld <kgroeneveld@gmail.com>
>> 
>> Add support for 64 bit stats to Broadcom b44 ethernet driver.
>> 
>> Signed-off-by: Kevin Groeneveld <kgroeneveld@gmail.com>
>> ---
>> 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.

Re: [PATCH] b44: add 64 bit stats

[Kevin Groeneveld]

Hi Eric,

On Sun, Jul 15, 2012 at 3:26 AM, Eric Dumazet <eric.dumazet@gmail.com> 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

Re: [PATCH] b44: add 64 bit stats

[Eric Dumazet]

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.

Re: [PATCH] b44: add 64 bit stats

[Eric Dumazet]

On Fri, 2012-07-20 at 06:53 +0200, Eric Dumazet wrote:
> 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.
> 

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 !

Re: [PATCH] b44: add 64 bit stats

[Ben Hutchings]

On Fri, 2012-07-20 at 07:24 +0200, Eric Dumazet wrote:
> On Fri, 2012-07-20 at 06:53 +0200, Eric Dumazet wrote:
> > 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.
> > 
> 
> 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.

Re: [PATCH] b44: add 64 bit stats

[Eric Dumazet]

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.

Re: [PATCH] b44: add 64 bit stats

[Kevin Groeneveld]

On Fri, Jul 20, 2012 at 1:24 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> 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

Re: [PATCH] b44: add 64 bit stats

[Kevin Groeneveld]

On Fri, Jul 20, 2012 at 2:56 PM, Kevin Groeneveld <kgroeneveld@gmail.com> wrote:
>> 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

Re: [PATCH] b44: add 64 bit stats

[Eric Dumazet]

On Fri, 2012-07-20 at 22:22 -0400, Kevin Groeneveld wrote:
> On Fri, Jul 20, 2012 at 2:56 PM, Kevin Groeneveld <kgroeneveld@gmail.com> wrote:
> >> 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 !

Re: [PATCH] b44: add 64 bit stats

[Julian Anastasov]

	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.
> 
> > 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 <hans.schillstrom@ericsson.com>
Date:   Mon Jan 3 14:44:56 2011 +0100

    IPVS: netns, ip_vs_stats and its procfs

> > 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

> > Kevin
> 
> Thanks !

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] b44: add 64 bit stats

[Kevin Groeneveld]

On Sat, Jul 21, 2012 at 1:09 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> 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