Re: [PATCH net-next v5 12/22] ethtool: provide string sets with GET_STRSET request

From: Jiri Pirko
Date: Thu Mar 28 2019 - 09:43:26 EST

Thu, Mar 28, 2019 at 08:18:53AM CET, mkubecek@xxxxxxx wrote:
>On Wed, Mar 27, 2019 at 07:25:47PM -0700, Florian Fainelli wrote:
>> > +GET_STRSET
>> > +----------
>> > +
>> > +Requests contents of a string set as provided by ioctl commands
>> > +ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS. String sets are not user writeable so
>> > +that the corresponding SET_STRSET message is only used in kernel replies.
>> > +There are two types of string sets: global (independent of a device, e.g.
>> > +device feature names) and device specific (e.g. device private flags).
>> > +
>> > +Request contents:
>> > +
>> > + ETHA_STRSET_DEV (nested) device identification
>> > + ETHA_STRSET_COUNTS (flag) request only string counts
>>
>> Should not that be part of the nested attribute under
>> ETHA_STRSET_STRINGSET. We should probably think about adding another
>> flag which indicates that we want to get the stringset associated data,
>> see below why.
>>
>> > + ETHA_STRSET_STRINGSET (nested) string set to request
>> > + ETHA_STRINGSET_ID (u32) set id
>> > +
>> > +Kernel response contents:
>> > +
>> > + ETHA_STRSET_DEV (nested) device identification
>> > + ETHA_STRSET_STRINGSET (nested) string set to request
>>
>> string set requested?
>>
>> > + ETHA_STRINGSET_ID (u32) set id
>> > + ETHA_STRINGSET_COUNT (u32) number of strings
>> > + ETHA_STRINGSET_STRINGS (nested) array of strings
>> > + ETHA_STRING_INDEX (u32) string index
>> > + ETHA_STRING_VALUE (string) string value
>>
>> This is one of the areas where the legacy ethtool ioctl() is painful
>> because we need to request a string set to know how many of those exist
>> to allocate space for those in both kernel and user space.
>>
>> If we could find a way to have a single command that allows us to dump
>> stringset (count, values) and associated data, then we save ourselves a
>> context switch and having to pre-allocate memory accordingly.
>
>The way the proposed interface is designed, we can get both without an
>extra roundtrip but it's done the other way around, i.e. passing the
>strings with the request for data. The API allows two modes of
>operation:
>
>1. One shot requests, e.g. typical ethtool use. We use "verbose" mode
>(no ETHA_*_COMPACT flag in the request) and data is provided together
>with the names. E.g. "ethtool eth2" request would look like
>
> ETHA_SETTINGS_DEV
> ETHA_DEV_NAME = "eth2"
> ETHA_SETTINGS_INFO_MASK = ... | ETH_SETTINGS_IM_LINKMODES | ...
>
>and the reply would be
>
> ETHA_SETTINGS_DEV = {
> ETHA_DEV_INDEX = 4
> ETHA_DEV_NAME = "eth2"
> }
> ETHA_SETTINGS_LINK_INFO = {
> ...
> }
> ETHA_SETTRINGS_LINK_MODES = {
> ETHA_LINKMODES_AUTONEG = 1 (AUTONEG_ENABLE)
> ETHA_LINKMODES_OURS = {
> ETHA_BITSET_SIZE = 67
> ETHA_BITSET_BITS = {
> ETHA_BITS_BIT = {
> ETHA_BIT_INDEX = 0 (ETHTOOL_LINK_MODE_10baseT_Half_BIT)
> ETHA_BIT_NAME = "10baseT/Half"
> }
> ETHA_BITS_BIT = {
> ETHA_BIT_INDEX = 1 (ETHTOOL_LINK_MODE_10baseT_Full_BIT)
> ETHA_BIT_NAME = "10baseT/Full"
> }
> ETHA_BITS_BIT = {
> ETHA_BIT_INDEX = 2 (ETHTOOL_LINK_MODE_100baseT_Half_BIT)
> ETHA_BIT_NAME = "100baseT/Half"
> ETHA_BIT_VALUE
> }
> ETHA_BITS_BIT = {
> ETHA_BIT_INDEX = 3 (ETHTOOL_LINK_MODE_100baseT_Full_BIT)
> ETHA_BIT_NAME = "100baseT/Full"
> ETHA_BIT_VALUE
> }
> ETHA_BITS_BIT = {
> ETHA_BIT_INDEX = 5 (ETHTOOL_LINK_MODE_1000baseT_Full_BIT)
> ETHA_BIT_NAME = "1000baseT/Full"
> ETHA_BIT_VALUE

I don't like this. This should not be bitfield/set. This should be
simply nested array of enum values:

enum ethtool_link_mode {
ETHTOOL_LINK_MODE_10baseT_Half,
ETHTOOL_LINK_MODE_10baseT_Full,
ETHTOOL_LINK_MODE_100baseT_Half,
ETHTOOL_LINK_MODE_100baseT_Full,
ETHTOOL_LINK_MODE_1000baseT_Full,
};

and then there should be 2 attrs:
ETHTOOL_A_LINK_MODE_LIST_OUR /* nest */
ETHTOOL_A_LINK_MODE_LIST_PEER /* nest */
ETHTOOL_A_LINK_MODE /* u32 */

and then the message should look like:

ETHTOOL_A_LINK_MODE_LIST_OUR start nest
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Half
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Full
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Half
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Full
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_1000baseT_Full
ETHTOOL_A_LINK_MODE_LIST_OUR end nest
ETHTOOL_A_LINK_MODE_LIST_PEER start nest
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Half
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Full
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Half
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Full
ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_1000baseT_Full
ETHTOOL_A_LINK_MODE_LIST_PEER end nest

Nice and simple. No bits, no strings.



> }
> }
> }
> ETHA_LINKMODES_PEER = {
> ...
> }
> ETHA_LINKMODES_SPEED = 1000 (SPEED_1000)
> ETHA_LINKMODES_DUPLEX = 1 (DUPLEX_FULL)
> }
> ...
>
>2. Long time running applications, e.g. "ethtool --monitor", wicked,
>systemd-networkd, ... For these, it would make sense to cache the
>strings and either retrieve them in advance (on start and when new
>device appears) or when they are needed for the first time. The request
>would then include the ETHA_SETTINGS_COMPACT flag and reply would be
>
> ...
> ETHA_LINKMODES_OURS = {
> ETHA_BITSET_SIZE = 67
> ETHA_BITSET_VALUE = 0x0000002c 0x00000000 0x00000000
> ETHA_BITSET_MASK = 0x0000002f 0x00000000 0x00000000
> }
>
>For "set" type requests, it's up to the application if it uses verbose
>or compact form. The verbose form is most useful when we want e.g. to
>set the values of one or two bits which may be identified by their names
>(on command line or in config file).
>
>Michal