| Message ID | 20210414202325.2225774-1-kuba@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | ethtool: add uAPI for reading standard stats | expand |
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 >
On Wed, 14 Apr 2021 22:51:39 -0700 Saeed Mahameed wrote: > 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.. No, no, the question was only whether we keep stat ids in the same attribute space as other group attributes (like string set ID) or whether they are nested somewhere deeper and have their own ID space. I went ahead and nested them yesterday. I had to engage in a little bit of black magic for pad, but it feels more right to nest.. static int stat_put(struct sk_buff *skb, u16 attrtype, u64 val) { struct nlattr *nest; int ret; if (val == ETHTOOL_STAT_NOT_SET) return 0; /* We want to start stats attr types from 0, so we don't have a type * for pad inside ETHTOOL_A_STATS_GRP_STAT. Pad things on the outside * of ETHTOOL_A_STATS_GRP_STAT. Since we're one nest away from the * actual attr we're 4B off - nla_need_padding_for_64bit() & co. * can't be used. */ #ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS if (!IS_ALIGNED((unsigned long)skb_tail_pointer(skb), 8)) if (!nla_reserve(skb, ETHTOOL_A_STATS_GRP_PAD, 0)) return -EMSGSIZE; #endif nest = nla_nest_start(skb, ETHTOOL_A_STATS_GRP_STAT); if (!nest) return -EMSGSIZE; ret = nla_put_u64_64bit(skb, attrtype, val, -1 /* not used */); if (ret) { nla_nest_cancel(skb, nest); return ret; } nla_nest_end(skb, nest); return 0; } > Just throwing crazy ideas.. BTF might be a useful tool here! :)) > > > allow seamless adding of legacy stats to this API > which legacy stats ? The ones from get_ethtool_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.. :) Hah! I hope it wasn't shot down as a bad idea :) Thanks a lot for the reviews!
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 allow seamless adding of legacy stats to this API (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 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