Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

[John W. Linville]

Michal,

Thanks for looking at this issue.

While the kernel and userland bits here are at least somewhat
decoupled, as the current maintainer for the userland ethtool I
feel compelled to comment. FWIW I had started down a similar road,
but I became dissatisfied with my own results. The main problem was
that the only obvious API-conversion technique essentially amounted
to re-implementing the ioctl-based API, but on top of the netlink
transport. Having been burned by something similar in the pre-nl80211
days with wireless extensions, I really didn't want to repeat that.

Even without considering the ioctl problesms, the current ethtool
API seems a bit crufty. It has been a catch-all, "where else would it
go?" dumping ground for a long time, and it has accrued a number of
not-entirely-related bits of functionality. In my mind, what needs
to happen is that these various bits of functionality need to be
reorganized into a handful of groupings. Then, each group needs an
API designed around semantics that are natural to the functionality
being addressed. I believe this is essentially the idea that others
have expressed with the "move some of the ethtool bits to devlink"
comments. I think that probably makes sense, although trying to shove
everything into devlink probably makes no more sense than keeping
the entire ethtool API intact on top of a netlink transport. Anyway,
I think that with a reasonable set of groupings, the semantics would
fall-out naturally and implementing them on netlink or any other
suitable transport would be reasonably trivial.

Unfortunately, I have yet to formultate a useful set of abstractions
for grouping the various bits of the ethtool API. I have reached-out
to a few community folks seeking their wisdom, but since no one has
been forthcoming with such a set of abstractions, I'll presume that
either I failed to convey my message, my idea isn't too good, or none
of them are any smarter than me -- I'll avoid identifying any of them
here in order to save us all some embarassment! :-)

In short, what I would like to see is a true rethink of what APIs need
to be provided to NICs, outside of the basic netdev abstraction. I
wish I had the answer to what those APIs should be, but I don't. I do
believe that with a well-conceived group of APIs, the proper semantics
will fall-out naturally. Any takers? :-)

Michal, once again -- thanks for attempting to address this
issue. Please do not take any of the above as discouragement. It is
clear that ethtool needs replacement, and we all know that 'perfect'
is the enemy of 'good'. I just would hate to miss the opportunity for a
'better' API just because ethtool's ioctly API has lived too long.

John

On Mon, Dec 11, 2017 at 02:53:11PM +0100, Michal Kubecek wrote:
> This is still work in progress and only a very small part of the ioctl
> interface is reimplemented but I would like to get some comments before
> the patchset becomes too big and changing things becomes too tedious.
> 
> The interface used for communication between ethtool and kernel is based on
> ioctl() and suffers from many problems. The most pressing seems the be the
> lack of extensibility. While some of the newer commands use structures
> designed to allow future extensions (e.g. GFEATURES or TEST), most either
> allow no extension at all (GPAUSEPARAM, GCOALESCE) or only limited set of
> reserved fields (GDRVINFO, GEEE). Even most of those which support future
> extensions limit the data types that can be used.
> 
> This series aims to provide an alternative interface based on netlink which
> is what other network configuration utilities use. In particular, it uses
> generic netlink (family "ethtool"). The goal is to provide an interface
> which would be extensible, flexible and practical both for ethtool and for
> other network configuration tools (e.g. wicked or systemd-networkd).
> 
> The interface is documented in Documentation/networking/ethtool-netlink.txt
> 
> I'll post RFC patch series for ethtool in a few days, it still needs some
> more polishing.
> 
> Basic concepts:
> 
> - the interface is based on generic netlink (family name "ethtool")
> 
> - provide everything ioctl can do but allow easier future extensions
> 
> - inextensibility of ioctl interface resulted in way too many commands,
>   many of them obsoleted by newer ones; reduce the number by  ignoring the
>   obsolete commands and grouping some together
> 
> - for "set" type commands, netlink allows providing only the attributes to
>   be changed; therefore we don't need a get-modify-set cycle, userspace can
>   simply say what it wants to change and how
> 
> - be less dependent on ethtool and kernel being in sync; allow e.g. saying
>   "ethtool -s eth0 advertise xyz off" without knowing what "xyz" means;
>   it's kernel's job to know what mode "xyz" is and if it exists and is
>   supported
> 
> Unresolved questions/tasks:
> 
> - allow dumps for "get" type requests, e.g. listing EEE settings for all
>   interfaces in current netns
> 
> - find reasonable format for data transfers (e.g. eeprom dump or flash)
> 
> - while the netlink interface allows easy future extensions, ethtool_ops
>   interface does not; some settings could be implemented using tunables and
>   accessed via relevant netlink messages (as well as tunables) from
>   userspace but in the long term, something better will be needed
> 
> - it would be nice if driver could provide useful error/warning messages to
>   be passed to userspace via extended ACK; example: while testing, I found
>   a driver which only allows values 0, 1, 3 and 10000 for certain parameter
>   but the only way poor user can find out is either by trying all values or
>   by checking driver source
> 
> Michal Kubecek (9):
>   netlink: introduce nla_put_bitfield32()
>   ethtool: introduce ethtool netlink interface
>   ethtool: helper functions for netlink interface
>   ethtool: netlink bitset handling
>   ethtool: implement GET_DRVINFO message
>   ethtool: implement GET_SETTINGS message
>   ethtool: implement SET_SETTINGS message
>   ethtool: implement GET_PARAMS message
>   ethtool: implement SET_PARAMS message
> 
>  Documentation/networking/ethtool-netlink.txt |  466 ++++++
>  include/linux/ethtool_netlink.h              |   12 +
>  include/linux/netdevice.h                    |    2 +
>  include/net/netlink.h                        |   15 +
>  include/uapi/linux/ethtool.h                 |    3 +
>  include/uapi/linux/ethtool_netlink.h         |  239 +++
>  net/Kconfig                                  |    7 +
>  net/core/Makefile                            |    3 +-
>  net/core/ethtool.c                           |  150 +-
>  net/core/ethtool_common.c                    |  158 ++
>  net/core/ethtool_common.h                    |   19 +
>  net/core/ethtool_netlink.c                   | 2323 ++++++++++++++++++++++++++
>  12 files changed, 3260 insertions(+), 137 deletions(-)
>  create mode 100644 Documentation/networking/ethtool-netlink.txt
>  create mode 100644 include/linux/ethtool_netlink.h
>  create mode 100644 include/uapi/linux/ethtool_netlink.h
>  create mode 100644 net/core/ethtool_common.c
>  create mode 100644 net/core/ethtool_common.h
>  create mode 100644 net/core/ethtool_netlink.c
> 
> -- 
> 2.15.1
> 
> 

-- 
John W. Linville		Someday the world will need a hero, and you
linville@xxxxxxxxxxxxx			might be all we have.  Be ready.