netdev - Re: [RFC net-next 0/6] ethtool: add uAPI for reading standard stats

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: <4425ee5839ac86270542ffa3d40cda67dc5068e1.camel@kernel.org>
Date:   Wed, 14 Apr 2021 22:51:39 -0700
From:   Saeed Mahameed <saeed@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, andrew@...n.ch, mkubecek@...e.cz,
        idosch@...dia.com, Ariel Almog <ariela@...dia.com>
Subject: Re: [RFC net-next 0/6] ethtool: add uAPI for reading standard stats

On Wed, 2021-04-14 at 13:23 -0700, Jakub Kicinski wrote:
> This series adds a new ethtool command to read well defined
> device statistics. There is nothing clever here, just a netlink
> API for dumping statistics defined by standards and RFCs which
> today end up in ethtool -S under infinite variations of names.
> 
> This series adds basic IEEE stats (for PHY, MAC, Ctrl frames)
> and RMON stats. AFAICT other RFCs only duplicate the IEEE
> stats.
> 
> This series does _not_ add a netlink API to read driver-defined
> stats. There seems to be little to gain from moving that part
> to netlink.
> 
> The netlink message format is very simple, and aims to allow
> adding stats and groups with no changes to user tooling (which
> IIUC is expected for ethtool). Stats are dumped directly
> into netlink with netlink attributes used as IDs. This is
> perhaps where the biggest question mark is. We could instead
> pack the stats into individual wrappers:
> 
>  [grp]
>    [stat] // nest
>      [id]    // u32
>      [value] // u64
>    [stat] // nest
>      [id]    // u32
>      [value] // u64
> 
> which would increase the message size 2x but allow
> to ID the stats from 0, saving strset space as well as

don't you need to translate such ids to strs in userspace ? 
I am not fond of upgrading userspace every time we add new stat.. 

Just throwing crazy ideas.. BTF might be a useful tool here! :)) 

> allow seamless adding of legacy stats to this API
which legacy stats ? 

> (which are IDed from 0).
> 
> On user space side we can re-use -S, and make it dump
> standard stats if --groups are defined.
> 
> $ ethtool -S eth0 --groups eth-phy eth-mac rmon eth-ctrl

Deja-vu, I honestly remember someone in mlnx suggsting this exact
command a couple of years ago.. :) 

> Stats for eth0:
> eth-phy-SymbolErrorDuringCarrier: 0
> eth-mac-FramesTransmittedOK: 0
> eth-mac-FrameTooLongErrors: 0
> eth-ctrl-MACControlFramesTransmitted: 0
> eth-ctrl-MACControlFramesReceived: 1
> eth-ctrl-UnsupportedOpcodesReceived: 0
> rmon-etherStatsUndersizePkts: 0
> rmon-etherStatsJabbers: 0
> rmon-rx-etherStatsPkts64Octets: 1
> rmon-rx-etherStatsPkts128to255Octets: 0
> rmon-rx-etherStatsPkts1024toMaxOctets: 1
> rmon-tx-etherStatsPkts64Octets: 1
> rmon-tx-etherStatsPkts128to255Octets: 0
> rmon-tx-etherStatsPkts1024toMaxOctets: 1
> 
> Jakub Kicinski (6):
>   docs: networking: extend the statistics documentation
>   docs: ethtool: document standard statistics
>   ethtool: add a new command for reading standard stats
>   ethtool: add interface to read standard MAC stats
>   ethtool: add interface to read standard MAC Ctrl stats
>   ethtool: add interface to read RMON stats
> 
>  Documentation/networking/ethtool-netlink.rst |  74 ++++
>  Documentation/networking/statistics.rst      |  44 ++-
>  include/linux/ethtool.h                      |  95 +++++
>  include/uapi/linux/ethtool.h                 |  10 +
>  include/uapi/linux/ethtool_netlink.h         | 134 +++++++
>  net/ethtool/Makefile                         |   2 +-
>  net/ethtool/netlink.c                        |  10 +
>  net/ethtool/netlink.h                        |   8 +
>  net/ethtool/stats.c                          | 374
> +++++++++++++++++++
>  net/ethtool/strset.c                         |  25 ++
>  10 files changed, 773 insertions(+), 3 deletions(-)
>  create mode 100644 net/ethtool/stats.c
>