Re: [PATCH net-next v2 00/12] net: ethtool: let ops locked drivers run without rtnl_lock

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 06/04, Jakub Kicinski wrote:
> With the ethtool_get_link_ksettings() situation hopefully ironed out
> the previous series (commit 6a5d837f0ce2) let's return to the main
> part of the series.
> 
> We have been slowly moving towards removing the rtnl_lock dependency
> in driver ops since the concept of "ops-locked" drivers have been
> introduced last year. Since last year will take the netdev instance
> lock before invoking any ndo or ethtool op of "ops-locked" drivers.
> 
> We dipped our toes into rtnl_lock-less ops with the queue binding API.
> Queue stats, NAPI, and other netdev-netlink objects are also queried
> without holding rtnl_lock already. It's time to take the next logical
> step and lift the requirement from ethtool ops.
> 
> The direct motivation for this patchset is that ethtool ops often
> involve communicating with device FW, and may take a long time
> to complete. Aggressive polling of device state on machines
> with 10+ NICs have been shown to significantly increase rtnl_lock
> pressure.
> 
> There's a handful of areas which still need rtnl_lock (see below).
> I decided to convert everything to rtnl_lock-less by default, and
> add a set of flags which let the drivers request rtnl_lock to still
> be taken. I don't love this, but I'm worried that opt-in would be
> even more confusing.
> 
> Known issues / exclusions:
>  - qdiscs - qdisc configuration currently assumes rtnl_lock, this
>    is mostly impacting set_channels callback. qdisc config is probably
>    the easiest one of the exclusions to tackle, it's fairly self-contained.
>  - features - even tho feature changes are (correctly) plumbed to
>    the driver thru ndos they are part of ethtool uAPI. ethtool itself
>    calls netdev_features_change() if it has spotted device feature change
>    before vs after to the callback. Some drivers also call
>    netdev_features_change() directly in response to various changes,
>    e.g. setting priv flags.
>    Since features have to propagate to upper and lower devices anything
>    that touches features is quite hard to move from under rtnl_lock.
>  - phylink - phylink and SFP depend on rtnl_lock today, I suspect
>    that this is purely for historic reasons. I started poking at
>    it and don't really see a need for a global lock. But accessing
>    the netdev instance lock from the SFP entry points will require
>    some attention from the phylink folks.
>  - phydev - similar to phylink, looks quite doable. But no ops-locked
>    driver currently has a phydev (fbnic only uses phylink) so phydev
>    related paths retain a ASSERT_RTNL() for now.
> 
> Tested on mlx5, bnxt and fbnic.
> 
> Jakub Kicinski (12):
>   net: ethtool: serialize broadcast notification sequence allocation
>   net: ethtool: relax ethnl_req_get_phydev() locking assertion
>   net: ethtool: make dev->hwprov ops-protected
>   net: ethtool: optionally skip rtnl_lock on Netlink path for GET ops
>   net: ethtool: optionally skip rtnl_lock on Netlink path for SET ops
>   net: ethtool: optionally skip rtnl_lock in cable test handlers
>   net: ethtool: optionally skip rtnl_lock in ethnl_tsinfo_dumpit()
>   net: ethtool: optionally skip rtnl_lock in ethnl_act_module_fw_flash()
>   net: ethtool: optionally skip rtnl_lock in RSS context handlers
>   net: ethtool: ioctl: concentrate the locking
>   net: ethtool: optionally skip rtnl_lock on IOCTL path
>   docs: net: ethtool: document ops-locked drivers and op_needs_rtnl

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

netdev_ops_lock_dereference name trips me a bit (_compat suffix would've been
more obvious), but I do agree with the reasoning about needing 'a' lock..