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

[Jiri Pirko <jiri@resnulli.us>]

Thu, Mar 28, 2019 at 08:18:53AM CET, mkubecek@suse.cz 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