Re: [PATCH net-next 12/20] net: hns3: Add packet statistics of netdev

[Jakub Kicinski <kubakici@wp.pl>]

On Mon, 8 Jan 2018 17:46:02 -0800, Jakub Kicinski wrote:
> On Mon, 08 Jan 2018 20:39:13 -0500 (EST), David Miller wrote:
> > From: Jakub Kicinski <kubakici@wp.pl>
> > Date: Mon, 8 Jan 2018 12:04:31 -0800
> >   
> > > Ugh, I so didn't review this in time :(  I think there is a consensus
> > > that we should avoid duplicating standard stats in ethtool.  Especially
> > > those old ones.  Like "collisions", I assume this is a modern NIC, are
> > > collisions still a thing?    
> > 
> > There is no standard way to get per-queue values, and ethtool stats are
> > how pretty much every driver provides it.  
> 
> Right, agreed.  I'm only objecting to this patch (12/20), where we can
> see the telltale code like this:
> 
> +	const struct rtnl_link_stats64 *net_stats;
> +	struct rtnl_link_stats64 temp;
> +
> +	net_stats = dev_get_stats(netdev, &temp);
> +	for (i = 0; i < HNS3_NETDEV_STATS_COUNT; i++) {
> +		stat = (u8 *)net_stats + hns3_netdev_stats[i].stats_offset;
> +		*data++ = *(u64 *)stat;
> +	}
> 
> Where:
> 
> +#define HNS3_NETDEV_STAT(_string, _member)	{			\
> +	.stats_string = _string,					\
> +	.stats_offset = offsetof(struct rtnl_link_stats64, _member)	\
> +}
> +
> +static const struct hns3_stats hns3_netdev_stats[] = {
> +	/* Rx per-queue statistics */

Oh, I only noticed this extra misleading comment now.  Unless each queue
has a netdev, I don't see how these are per-queue.

> +	HNS3_NETDEV_STAT("rx_packets", rx_packets),
> +	HNS3_NETDEV_STAT("tx_packets", tx_packets),
> 
> etc.  IOW dumping struct rtnl_link_stats64 to ethtool -S member by
> member.
> 
> Let me put the netlink per-queue stats on my soft TODO list :)
>