| Message ID | 20240620114711.777046-4-edumazet@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethtool: reduce RTNL pressure in dev_ethtool() | expand |
On Thu, 20 Jun 2024 11:47:08 +0000 Eric Dumazet wrote: > Move pm_runtime_get_sync() and pm_runtime_put() out of __dev_ethtool > to dev_ethtool() while RTNL is not yet held. > > These helpers do not depend on RTNL. The helpers themselves don't, but can we assume no drivers have implicit dependencies on calling netif_device_detach() under rtnl_lock, and since the presence checks are under rtnl_lock they are currently guaranteed not to get any callbacks past detach() + rtnl_unlock()? I think its better to completely skip PM + presence + ->begin if driver wants the op to be unlocked, but otherwise keep the locking as is I also keep wondering whether we shouldn't use this as an opportunity to introduce a "netdev instance lock". I think you mentioned we should move away from rtnl for locking ethtool and ndos since most drivers don't care at all about global state. Doing that is a huge project, but maybe this is where we start?
> I also keep wondering whether we shouldn't use this as an opportunity > to introduce a "netdev instance lock". I think you mentioned we should > move away from rtnl for locking ethtool and ndos since most drivers > don't care at all about global state. Doing that is a huge project, > but maybe this is where we start? Is there much benefit to the average system? Embedded systems typically have 1 or 2 netdevs. Laptops, desktops and the like have one, maybe two netdevs. VMs typically have one netdev. So we are talking about high end switches with lots of ports and servers hosting lots of VMs. So of the around 500 netdev drivers we have, only maybe a dozen drivers would benefit? It seems unlikely those 500 drivers will be reviewed and declared safe to not take RTNL. So maybe a better way forward is that struct ethtool_ops gains a flag indicating its ops can be called without first talking RTNL. Somebody can then look at those dozen drivers, and we leave the other 490 alone and don't need to worry about regressions. Andrew
On Fri, 21 Jun 2024 02:59:54 +0200 Andrew Lunn wrote: > > I also keep wondering whether we shouldn't use this as an opportunity > > to introduce a "netdev instance lock". I think you mentioned we should > > move away from rtnl for locking ethtool and ndos since most drivers > > don't care at all about global state. Doing that is a huge project, > > but maybe this is where we start? > > Is there much benefit to the average system? > > Embedded systems typically have 1 or 2 netdevs. Laptops, desktops and > the like have one, maybe two netdevs. VMs typically have one netdev. > So we are talking about high end switches with lots of ports and > servers hosting lots of VMs. So of the around 500 netdev drivers we > have, only maybe a dozen drivers would benefit? > > It seems unlikely those 500 drivers will be reviewed and declared safe > to not take RTNL. So maybe a better way forward is that struct > ethtool_ops gains a flag indicating its ops can be called without > first talking RTNL. Somebody can then look at those dozen drivers, and > we leave the other 490 alone and don't need to worry about > regressions. Right, we still need an opt in. My question is more whether we should offer an opt out from rtnl_lock, and beyond that driver is on its own (which reviewing the driver code - I believe will end pretty badly), or to also offer a per-netdev instance lock. Give the drivers a choice: - rtnl - netdev_lock(dev) - (my least preferred) nothing. The netdev lock would also be useful for things like napi and queue stats, RSS contexts, and whatever else we add for drivers in the core. For NAPI / queue info via netlink we currently require rtnl_lock, taking a global lock to access a couple of per-netdevs structs feels quite wasteful :(
On Fri, Jun 21, 2024 at 2:22 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 20 Jun 2024 11:47:08 +0000 Eric Dumazet wrote: > > Move pm_runtime_get_sync() and pm_runtime_put() out of __dev_ethtool > > to dev_ethtool() while RTNL is not yet held. > > > > These helpers do not depend on RTNL. > > The helpers themselves don't, but can we assume no drivers have > implicit dependencies on calling netif_device_detach() under rtnl_lock, > and since the presence checks are under rtnl_lock they are currently > guaranteed not to get any callbacks past detach() + rtnl_unlock()? > > I think its better to completely skip PM + presence + ->begin if driver > wants the op to be unlocked, but otherwise keep the locking as is This PM stuff came 3 years ago, for apparently lack of user space awareness. commit f32a213765739f2a1db319346799f130a3d08820 Author: Heiner Kallweit <hkallweit1@gmail.com> Date: Sun Aug 1 12:36:48 2021 +0200 ethtool: runtime-resume netdev parent before ethtool ioctl ops I have not looked closely at the ->begin() and ->close() stuff, I will do this next week (I am OOO this Friday) > > I also keep wondering whether we shouldn't use this as an opportunity > to introduce a "netdev instance lock". I think you mentioned we should > move away from rtnl for locking ethtool and ndos since most drivers > don't care at all about global state. Doing that is a huge project, > but maybe this is where we start? Yes, a per device mutex would probably be needed in the long term.
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 45e7497839389bad9c6a6b238429b7534bfd6085..70bb0d2fa2ed416fdff3de71a4f752e4a1bba67a 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -2906,14 +2906,6 @@ __dev_ethtool(struct net_device *dev, struct ifreq *ifr, netdev_features_t old_features; int rc; - if (dev->dev.parent) - pm_runtime_get_sync(dev->dev.parent); - - if (!netif_device_present(dev)) { - rc = -ENODEV; - goto out; - } - if (dev->ethtool_ops->begin) { rc = dev->ethtool_ops->begin(dev); if (rc < 0) @@ -3137,9 +3129,6 @@ __dev_ethtool(struct net_device *dev, struct ifreq *ifr, if (old_features != dev->features) netdev_features_change(dev); out: - if (dev->dev.parent) - pm_runtime_put(dev->dev.parent); - return rc; } @@ -3183,9 +3172,19 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr) if (!dev) goto exit_free; + if (dev->dev.parent) + pm_runtime_get_sync(dev->dev.parent); + + if (!netif_device_present(dev)) + goto out_pm; + rtnl_lock(); rc = __dev_ethtool(dev, ifr, useraddr, ethcmd, sub_cmd, state); rtnl_unlock(); + +out_pm: + if (dev->dev.parent) + pm_runtime_put(dev->dev.parent); netdev_put(dev, &dev_tracker); if (rc)
Move pm_runtime_get_sync() and pm_runtime_put() out of __dev_ethtool to dev_ethtool() while RTNL is not yet held. These helpers do not depend on RTNL. Signed-off-by: Eric Dumazet <edumazet@google.com> --- net/ethtool/ioctl.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)