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

[Jakub Kicinski]

On Sat, 17 Apr 2021 21:53:40 +0300 Ido Schimmel wrote:
> On Sat, Apr 17, 2021 at 11:13:51AM -0700, Jakub Kicinski wrote:
> > On Sat, 17 Apr 2021 10:57:42 -0700 Jakub Kicinski wrote:  
> > > 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?  
> > 
> > FWIW ethnl_parse_bit() -> ETHTOOL_A_BITSET_BIT_NAME
> > User space can also use raw flags like --groups 0xf but that's perhaps
> > too spartan for serious use.  
> 
> So the kernel can work with ETHTOOL_A_BITSET_BIT_INDEX /
> ETHTOOL_A_BITSET_BIT_NAME, but I was wondering if using ethtool binary
> we can query the strings that the kernel will accept. I think not?

For request user space sends the strings inside the netlink message:

ethtool --debug 0xff -S eth0 --groups eth-phy eth-mac

sending genetlink packet (80 bytes):
    msg length 80 ethool ETHTOOL_MSG_STATS_GET
    ETHTOOL_MSG_STATS_GET
        ETHTOOL_A_STATS_HEADER
            ETHTOOL_A_HEADER_DEV_NAME = "eth0"
        ETHTOOL_A_STATS_GROUPS
            ETHTOOL_A_BITSET_NOMASK = true
            ETHTOOL_A_BITSET_BITS
                ETHTOOL_A_BITSET_BITS_BIT
                    ETHTOOL_A_BITSET_BIT_NAME = "eth-phy"
                ETHTOOL_A_BITSET_BITS_BIT
                    ETHTOOL_A_BITSET_BIT_NAME = "eth-mac"

Kernel will then search the bitset and respond with:

netlink error: bit name not found

if string is not found. So upfront enumeration is not strictly
necessary.

The only way to query what will be accepted AFAIU is to query 
the string set via ETHTOOL_MSG_STRSET_GET. The string set is part 
of the uAPI:

const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN] = {
        [ETHTOOL_STATS_ETH_PHY]                 = "eth-phy",
        [ETHTOOL_STATS_ETH_MAC]                 = "eth-mac",
        [ETHTOOL_STATS_ETH_CTRL]                = "eth-ctrl",
        [ETHTOOL_STATS_RMON]                    = "rmon",
};

        [ETH_SS_STATS_STD] = {
                .per_dev        = false,
                .count          = __ETHTOOL_STATS_CNT,
                .strings        = stats_std_names,
        },

I believe string set must have the same number of bits as the bitset,
so there's full equivalency. We could try to express the limits in a
static netlink policy one day but today parameters of the bitset are
passed in the code.

> Anyway, I'm fine with implementing '--all-groups' via
> ETHTOOL_MSG_STRSET_GET.

Cool, will do!

> We can always add a new attribute later, but I
> don't see a reason to do so.