Re: [PATCH net-next v2 3/9] ethtool: add a new command for reading standard stats

[Jakub Kicinski <kuba@kernel.org>]

On Sat, 17 Apr 2021 20:15:11 +0300 Ido Schimmel wrote:
> On Fri, Apr 16, 2021 at 12:27:39PM -0700, Jakub Kicinski wrote:
> > Add an interface for reading standard stats, including
> > stats which don't have a corresponding control interface.
> > 
> > Start with IEEE 802.3 PHY stats. There seems to be only
> > one stat to expose there.
> > 
> > Define API to not require user space changes when new
> > stats or groups are added. Groups are based on bitset,
> > stats have a string set associated.  
> 
> I tried to understand how you add new groups without user space
> changes and I think this statement is not entirely accurate.
> 
> At minimum, user space needs to know the names of these groups, but
> currently there is no way to query the information, so it's added to
> ethtool's help message:
> 
> ethtool [ FLAGS ] -S|--statistics DEVNAME       Show adapter statistics       
>        [ --groups [eth-phy] [eth-mac] [eth-ctrl] [rmon] ]

Um, yes and now. The only places the user space puts those names 
is the help message and man page.

Thru the magic of bitsets it doesn't actually interpret them, so
with old user space you can still query a new group, it will just 
not show up in "ethtool -h".

Is that what you're saying?

> I was thinking about adding a new command (e.g.,
> ETHTOOL_MSG_STATS_GROUP_GET) to query available groups, but maybe it's
> an overkill. How about adding a new flag to ethtool:
> 
> ethtool [ FLAGS ] -S|--statistics DEVNAME       Show adapter statistics       
>        [ { --groups [eth-phy] [eth-mac] [eth-ctrl] [rmon] | --all-groups } ]
> 
> Which will be a new flag attribute (e.g., ETHTOOL_A_STATS_ALL_GROUPS) in
> ETHTOOL_MSG_STATS_GET. Kernel will validate that
> ETHTOOL_A_STATS_ALL_GROUPS and ETHTOOL_A_STATS_GROUPS are not passed
> together.

We don't need any new API, user space can just query the string set 
(ETH_SS_STATS_STD) and each string there corresponds to a bit.
Would that work our you think that's not clean enough?

> It's not the end of the world to leave it as-is, but the new flag will
> indeed allow you to continue using your existing ethtool binary when
> upgrading the kernel and still getting all the new stats.
> 
> Actually, if we ever get an exporter to query this information, it will
> probably want to use the new flag instead of having to be patched
> whenever a new group is added.

I like the idea of --all-groups, I'll add that to user space if you're
okay with doing it based on the strset. Or can add the new flag attr if
you prefer.

Unfortunately I did not come up with any way to auto-update the man
page which is a slight bummer.