Re: [PATCH 5/5] alx: add stats to ethtool

[Sabrina Dubroca <sd@queasysnail.net>]

[2014-01-02, 14:09:14] Johannes Berg wrote:
> On Thu, 2014-01-02 at 00:40 +0100, Sabrina Dubroca wrote:
> 
> > +static const char alx_gstrings_stats[][ETH_GSTRING_LEN] = {
> 
> You should probably have a comment here and on the stats struct
> declaration that they must absolutely match in order/size/etc.


With these comments, and something similar before
__alx_update_hw_stats, should I use the code for __alx_update_hw_stats
mentionned in mail 0?


/* Statistics counters collected by the MAC
 *
 * The order of the fields must match the strings in alx_gstrings_stats
 * See ethtool.c
 */
struct alx_hw_stats {
	/* ... */
}

/* The order of these strings must match the order of the fields in
 * struct alx_hw_stats
 * See hw.h
 */
static const char alx_gstrings_stats[][ETH_GSTRING_LEN] = {
	/* ... */
}



> Maybe try to put in some BUILD_BUG_ON() as well, at least checking the
> sizeof() vs. ARRAY_SIZE*sizeof(u64) - that might already have caught the
> bug that Ben pointed out.


static void alx_get_ethtool_stats(struct net_device *netdev,
				  struct ethtool_stats *estats, u64 *data)
{
	struct alx_priv *alx = netdev_priv(netdev);
	struct alx_hw *hw = &alx->hw;

	spin_lock(&alx->stats_lock);

	__alx_update_hw_stats(hw);
	BUILD_BUG_ON(sizeof(hw->stats) - offsetof(struct alx_hw_stats, rx_ok) <
		     ALX_NUM_STATS * sizeof(u64));
	memcpy(data, &hw->stats.rx_ok, ALX_NUM_STATS * sizeof(u64));

	spin_unlock(&alx->stats_lock);
}

This way, rx_ok doesn't need to come first in struct alx_hw_stats, and
the size of the structure is checked as you said.



> > +	"rx_packets",
> 
> Is it useful to provide stats that are already elsewhere? Then again, it
> doesn't really hurt and simplifies the code ...

You're right, rx_packets is the sum of all the rx_SIZE_RANGE_packets
and is redundant information. The values split by size range are only
for ethtool. ndo_get_stats(64) uses only the sum. I think this
information is more relevant to the user, and anyway, it's available,
so we might as well provide it.


Thanks,

-- 
Sabrina Dubroca