Re: [PATCH] b44: add 64 bit stats

[Eric Dumazet <eric.dumazet@gmail.com>]

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