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

[Jakub Kicinski <kuba@kernel.org>]

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!