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

From: Jiri Pirko <jiri@resnulli.us>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: David Miller <davem@davemloft.net>,
	netdev@vger.kernel.org,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	John Linville <linville@tuxdriver.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v5 12/22] ethtool: provide string sets with GET_STRSET request
Date: Fri, 29 Mar 2019 09:43:43 +0100	[thread overview]
Message-ID: <20190329084343.GU14297@nanopsycho> (raw)
In-Reply-To: <20190327225607.GW26076@unicorn.suse.cz>

Wed, Mar 27, 2019 at 11:56:08PM CET, mkubecek@suse.cz wrote:
>On Wed, Mar 27, 2019 at 09:12:37PM +0100, Jiri Pirko wrote:
>> Mon, Mar 25, 2019 at 06:08:30PM CET, mkubecek@suse.cz wrote:
>> >Requests a contents of one or more string sets, i.e. indexed arrays of
>> >strings; this information is provided by ETHTOOL_GSSET_INFO and
>> >ETHTOOL_GSTRINGS commands of ioctl interface. There are three types of
>> >requests:
>> >
>> >  - no NLM_F_DUMP, no device: get "global" stringsets
>> >  - no NLM_F_DUMP, with device: get string sets related to the device
>> >  - NLM_F_DUMP, no device: get device related string sets for all devices
>> >
>> >It's possible to request all string sets of given type or only specific
>> >sets. With ETHA_STRSET_COUNTS flag, only set sizes (number of strings) are
>> >returned.
>> >
>> >Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>> >---
>> > Documentation/networking/ethtool-netlink.txt |  46 +-
>> > include/uapi/linux/ethtool.h                 |   2 +
>> > include/uapi/linux/ethtool_netlink.h         |  43 ++
>> > net/ethtool/Makefile                         |   2 +-
>> > net/ethtool/netlink.c                        |   8 +
>> > net/ethtool/netlink.h                        |   4 +
>> > net/ethtool/strset.c                         | 447 +++++++++++++++++++
>> > 7 files changed, 549 insertions(+), 3 deletions(-)
>> > create mode 100644 net/ethtool/strset.c
>> 
>> First of all, the code is hard to follow. For reasons I mentioned in
>> other replies (lack of prefixes, wrappers, etc).
>> 
>> More importantly, why do we need this? This concept of having strings in
>> kernel for various things and features and sending them to userspace is
>> weird. Certainly not common for Netlink interface. I believe these
>> strings should be avoided and all should be communicated to userspace
>> and back in form of well-defined Netlink attributes. We are introducing
>> new Netlink API, lets do it properly and don't bring baggage from past.
>
>For some of the global string sets where the values have fixed meaning
>and new values are only appended to, it would be possible. But even for
>those, keeping the list in sync between kernel and ethtool is often less
>than perfect. And ethtool is only one of the tools using the interface.
>So even in this case, I don't see string identifiers (or tags or names
>or whatever we call them) as a baggage from past, rather as a solution
>to a problem some of the existing interfaces have.
>
>Then e.g. netdev features are not fixed in this sense: the same bit
>index represents different feature in different kernels. I guess we

Should not be exposed in bits in first place. See devlink params for
example.

>could introduce some fixed numeric identifiers for them and map between
>those and actual indices but I don't see an advantage of such approach.
>
>But the really important question is how would you handle what is
>currently described by "per device" string sets, i.e. private flags,
>(ethtool) statistics, tests, ...? For these, the list depends on the
>driver or even device.

Yes, those per-driver things have to be strings. That's what we did for
devlink params.

Tests also could be handled in similar way as devlink params.

For stats, I believe that uint->string per-driver mapping
needs to be exposed.