Re: [PATCH v3 4/5] alx: add alx_get_stats64 operation

[Sabrina Dubroca <sd@queasysnail.net>]

Then the stats in all atheros drivers are a bit broken? They all use
the same formulas for ndo_get_stats.

Other atheros drivers (atl1e/atl1e.h and atl1c/atl1c.h) have the same
stats structure with a comment for each field. I will copy the
comments to alx.h.


2014-01-06, 19:07:33 +0000, Ben Hutchings wrote:
> On Mon, 2014-01-06 at 17:33 +0100, Sabrina Dubroca wrote:
> [...]
> > --- a/drivers/net/ethernet/atheros/alx/main.c
> > +++ b/drivers/net/ethernet/atheros/alx/main.c
> > @@ -1166,10 +1166,54 @@ static void alx_poll_controller(struct net_device *netdev)
> >  }
> >  #endif
> >  
> > +static struct rtnl_link_stats64 *alx_get_stats64(struct net_device *dev,
> > +					struct rtnl_link_stats64 *net_stats)
> > +{
> > +	struct alx_priv *alx = netdev_priv(dev);
> > +	struct alx_hw_stats *hw_stats = &alx->hw.stats;
> > +
> > +	spin_lock(&alx->stats_lock);
> > +
> > +	alx_update_hw_stats(&alx->hw);
> > +
> > +	net_stats->tx_packets = hw_stats->tx_ok;
> 
> I think this should be set to hw_stats->tx_ok + net_stats->tx_errors
> (after you set tx_errors).
> 
> > +	net_stats->tx_bytes   = hw_stats->tx_byte_cnt;
> > +	net_stats->rx_packets = hw_stats->rx_ok;
> 
> Similarly, I think this should be hw_stats->rx_ok +
> net_stats->rx_errors.
> 
> > +	net_stats->rx_bytes   = hw_stats->rx_byte_cnt;
> > +	net_stats->multicast  = hw_stats->rx_mcast;
> > +	net_stats->collisions = hw_stats->tx_single_col +
> > +				hw_stats->tx_multi_col * 2 +
> 
> I would expect this to count the number of packets that had collisions
> rather than total number of collisions (which you're only guessing at by
> using * 2).

Okay, all changed.


> > +				hw_stats->tx_late_col + hw_stats->tx_abort_col;
> > +
> > +	net_stats->rx_errors  = hw_stats->rx_frag + hw_stats->rx_fcs_err +
> > +				hw_stats->rx_len_err + hw_stats->rx_ov_sz +
> > +				hw_stats->rx_ov_rrd + hw_stats->rx_align_err;
> > +
> > +	net_stats->rx_fifo_errors   = hw_stats->rx_ov_rxf;
> > +	net_stats->rx_length_errors = hw_stats->rx_len_err;
> > +	net_stats->rx_crc_errors    = hw_stats->rx_fcs_err;
> > +	net_stats->rx_frame_errors  = hw_stats->rx_align_err;
> > +	net_stats->rx_over_errors   = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf;
> 
> rx_over_errors is commented as 'receiver ring buff overflow' and
> ifconfig includes it in the 'frame' error count.  I think it is intended
> to count frames which are too large for on-chip RX buffers and should
> always be 0 for devices that do RX DMA.
> 
> Each error should contribute to at most one specific error stat, so
> don't count rx_ov_rxf in both rx_fifo_errors and rx_over_errors.  I
> would guess rx_fifo_errors is the right counter.
>
> I don't know what rx_ov_rrd represents, but if it's the number of
> packets dropped because the RX descriptor ring was empty then it should
> be counted in rx_dropped not rx_over_errors.

in atl1e/atl1e.h:
rx_ov_rxf: The number of frame dropped due to occurrence of RX FIFO overflow.
rx_ov_rrd: The number of frame dropped due to occurrence of RRD overflow.

I'm not sure which counter fits these best.


> > +	net_stats->rx_missed_errors = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf;
> [...]
> 
> This counter is commented as 'receiver missed packet'.  I think this
> means the MAC detected SOF but was somehow too busy to receive the
> packet, but I'm not sure.  I don't think these hardware counters match
> the description, and certainly they shouldn't be counted here as well as
> the other specific error stats.

Okay.


Thanks,

-- 
Sabrina