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

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 6/8/2026 4:01 PM, Jacob Keller wrote:
> On 6/8/2026 3:31 PM, Jacob Keller wrote:
>> On 6/4/2026 5:29 PM, 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.
>>>
>>
>> Agreed. It might be nicer to opt-out first, and then opt-in the drivers
>> that don't do the update_features.. That would make it easier to prevent
>> buggy drivers slipping in as easily... But that would result in the
>> following messy situation:
>>
>> ethtool ops for ops-locked drivers don't hold RTNL lock, *except* some
>> which do because they might call update_features.. *except* those which
>> opt-out of RTNL because they know their driver implementation doesn't
>> call update_features...
>>
>> Yea. I think that is too messy. This approach requires slightly more
>> vigilance on the part of reviewers to ensure that ops-locked drivers
>> don't make a mistake here. However, the checks to see if features
>> changed while the RTNL wasn't held should be enough to help catch most
>> cases. Ok.
>>
>>> 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.
>>>
>>
>> Makes sense. Taking some steps towards the end goal is better than
>> trying to wait until every piece here is done.
>>
>> Whole series looks good to me. I had one minor nit about one patch
>> adding a bit flag at BIT(1) and then later changing that to BIT(5) but
>> its ultimately harmless and should not force a respin of this important
>> work.
>>
>> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>>
>>> Tested on mlx5, bnxt and fbnic.
>>>
>>
>> I think currently only iAVF is ops-locked for Intel. Tony, would it make
>> sense to see if our virtualization validation folks have cycles to give
>> this a quick pass on iAVF? (Regardless of whether it merges before or
>> after, it would be beneficial to make sure we don't have any lingering
>> issues there).
> 
> Just to clarify, I'm not suggesting a delay on merging and am 100% fine
> with merging this before such testing completes. I just want to make
> sure we (Intel) do double check the iavf code so that development team
> can try to fix any lurking bugs within the kernel cycle before they
> become wide spread.

+1. I've asked our validation to test iavf on this series.

Thanks,
Tony