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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180108175021.4a769c33@cakuba.netronome.com>
Date:   Mon, 8 Jan 2018 17:50:21 -0800
From:   Jakub Kicinski <kubakici@...pl>
To:     David Miller <davem@...emloft.net>
Cc:     lipeng321@...wei.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linuxarm@...wei.com,
        salil.mehta@...wei.com
Subject: Re: [PATCH net-next 12/20] net: hns3: Add packet statistics of
 netdev

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@...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 :)
>