[PATCH net-next 0/6] net: ethtool: reduce RTNL pressure in dev_ethtool()

[PATCH net-next 0/6] net: ethtool: reduce RTNL pressure in dev_ethtool()

[Eric Dumazet]

This series is inspired by Jakub feedback for one gve patch :

https://lore.kernel.org/lkml/20240614192721.02700256@kernel.org/

Refactor dev_ethtool() and start to implement parallel operations.

Eric Dumazet (6):
  net: ethtool: grab a netdev reference in dev_ethtool()
  net: ethtool: add dev_ethtool_cap_check()
  net: ethtool: perform pm duties outside of rtnl lock
  net: ethtool: call ethtool_get_one_feature() without RTNL
  net: ethtool: implement lockless ETHTOOL_GFLAGS
  net: ethtool: add the ability to run ethtool_[gs]et_rxnfc() without
    RTNL

 include/linux/ethtool.h |   2 +
 net/core/dev.c          |  10 +--
 net/ethtool/ioctl.c     | 159 ++++++++++++++++++++++++----------------
 3 files changed, 101 insertions(+), 70 deletions(-)

-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH net-next 1/6] net: ethtool: grab a netdev reference in dev_ethtool()

[Eric Dumazet]

We would like to not always grab RTNL for some commands in the future,
to decrease RTNL pressure.

Grab a reference on the device to ensure it will not disappear while
dev_ethtool() is running.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ethtool/ioctl.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index e645d751a5e8998064ef5fa239d465f66c044e6f..01c52159aef7a47165ef11aab2c599ffe4ad345d 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2851,18 +2851,14 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 static int
-__dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
-	      u32 ethcmd, struct ethtool_devlink_compat *devlink_state)
+__dev_ethtool(struct net *net, struct net_device *dev, struct ifreq *ifr,
+	      void __user *useraddr, u32 ethcmd,
+	      struct ethtool_devlink_compat *devlink_state)
 {
-	struct net_device *dev;
 	u32 sub_cmd;
 	int rc;
 	netdev_features_t old_features;
 
-	dev = __dev_get_by_name(net, ifr->ifr_name);
-	if (!dev)
-		return -ENODEV;
-
 	if (ethcmd == ETHTOOL_PERQUEUE) {
 		if (copy_from_user(&sub_cmd, useraddr + sizeof(ethcmd), sizeof(sub_cmd)))
 			return -EFAULT;
@@ -3153,6 +3149,8 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
 int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 {
 	struct ethtool_devlink_compat *state;
+	netdevice_tracker dev_tracker;
+	struct net_device *dev;
 	u32 ethcmd;
 	int rc;
 
@@ -3173,9 +3171,16 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 		break;
 	}
 
+	rc = -ENODEV;
+	dev = netdev_get_by_name(net, ifr->ifr_name, &dev_tracker, GFP_KERNEL);
+	if (!dev)
+		goto exit_free;
+
 	rtnl_lock();
-	rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
+	rc = __dev_ethtool(net, dev, ifr, useraddr, ethcmd, state);
 	rtnl_unlock();
+	netdev_put(dev, &dev_tracker);
+
 	if (rc)
 		goto exit_free;
 
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH net-next 2/6] net: ethtool: add dev_ethtool_cap_check()

[Eric Dumazet]

Perform capability check in a dedicated helper, before grabbing RTNL.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ethtool/ioctl.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 01c52159aef7a47165ef11aab2c599ffe4ad345d..45e7497839389bad9c6a6b238429b7534bfd6085 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2850,21 +2850,8 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
-static int
-__dev_ethtool(struct net *net, struct net_device *dev, struct ifreq *ifr,
-	      void __user *useraddr, u32 ethcmd,
-	      struct ethtool_devlink_compat *devlink_state)
+static int dev_ethtool_cap_check(struct net *net, u32 sub_cmd)
 {
-	u32 sub_cmd;
-	int rc;
-	netdev_features_t old_features;
-
-	if (ethcmd == ETHTOOL_PERQUEUE) {
-		if (copy_from_user(&sub_cmd, useraddr + sizeof(ethcmd), sizeof(sub_cmd)))
-			return -EFAULT;
-	} else {
-		sub_cmd = ethcmd;
-	}
 	/* Allow some commands to be done by anyone */
 	switch (sub_cmd) {
 	case ETHTOOL_GSET:
@@ -2908,6 +2895,16 @@ __dev_ethtool(struct net *net, struct net_device *dev, struct ifreq *ifr,
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 	}
+	return 0;
+}
+
+static int
+__dev_ethtool(struct net_device *dev, struct ifreq *ifr,
+	      void __user *useraddr, u32 ethcmd, u32 sub_cmd,
+	      struct ethtool_devlink_compat *devlink_state)
+{
+	netdev_features_t old_features;
+	int rc;
 
 	if (dev->dev.parent)
 		pm_runtime_get_sync(dev->dev.parent);
@@ -3151,7 +3148,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 	struct ethtool_devlink_compat *state;
 	netdevice_tracker dev_tracker;
 	struct net_device *dev;
-	u32 ethcmd;
+	u32 ethcmd, sub_cmd;
 	int rc;
 
 	if (copy_from_user(&ethcmd, useraddr, sizeof(ethcmd)))
@@ -3171,13 +3168,23 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 		break;
 	}
 
+	if (ethcmd == ETHTOOL_PERQUEUE) {
+		if (copy_from_user(&sub_cmd, useraddr + sizeof(ethcmd), sizeof(sub_cmd)))
+			return -EFAULT;
+	} else {
+		sub_cmd = ethcmd;
+	}
+	rc = dev_ethtool_cap_check(net, sub_cmd);
+	if (rc)
+		goto exit_free;
+
 	rc = -ENODEV;
 	dev = netdev_get_by_name(net, ifr->ifr_name, &dev_tracker, GFP_KERNEL);
 	if (!dev)
 		goto exit_free;
 
 	rtnl_lock();
-	rc = __dev_ethtool(net, dev, ifr, useraddr, ethcmd, state);
+	rc = __dev_ethtool(dev, ifr, useraddr, ethcmd, sub_cmd, state);
 	rtnl_unlock();
 	netdev_put(dev, &dev_tracker);
 
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH net-next 3/6] net: ethtool: perform pm duties outside of rtnl lock

[Eric Dumazet]

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(-)

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)
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH net-next 3/6] net: ethtool: perform pm duties outside of rtnl lock

[Jakub Kicinski]

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?

Re: [PATCH net-next 3/6] net: ethtool: perform pm duties outside of rtnl lock

[Andrew Lunn]

> 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

Re: [PATCH net-next 3/6] net: ethtool: perform pm duties outside of rtnl lock

[Jakub Kicinski]

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 :(

Re: [PATCH net-next 3/6] net: ethtool: perform pm duties outside of rtnl lock

[Eric Dumazet]

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.

[PATCH net-next 4/6] net: ethtool: call ethtool_get_one_feature() without RTNL

[Eric Dumazet]

ethtool_get_one_feature() is used by ETHTOOL_GTXCSUM, ETHTOOL_GRXCSUM,
ETHTOOL_GSG, ETHTOOL_GTSO, ETHTOOL_GGSO and ETHTOOL_GGRO.

Add WRITE_ONCE() and READ_ONCE() annotations on dev->features.

Note that many READ_ONCE() annotations are probably missing in many drivers,
but this is orthogonal to this patch.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c      | 10 +++++-----
 net/ethtool/ioctl.c | 30 ++++++++++++++++--------------
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 093d82bf0e2886d4948f4952353c71c92e3586c6..b18223ed269f24bedd02b7c435de0ce2f1f8edf3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3536,7 +3536,7 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
 netdev_features_t netif_skb_features(struct sk_buff *skb)
 {
 	struct net_device *dev = skb->dev;
-	netdev_features_t features = dev->features;
+	netdev_features_t features = READ_ONCE(dev->features);
 
 	if (skb_is_gso(skb))
 		features = gso_features_check(skb, dev, features);
@@ -10016,7 +10016,7 @@ int __netdev_update_features(struct net_device *dev)
 			 * but *after* calling udp_tunnel_drop_rx_info.
 			 */
 			if (features & NETIF_F_RX_UDP_TUNNEL_PORT) {
-				dev->features = features;
+				WRITE_ONCE(dev->features, features);
 				udp_tunnel_get_rx_info(dev);
 			} else {
 				udp_tunnel_drop_rx_info(dev);
@@ -10025,7 +10025,7 @@ int __netdev_update_features(struct net_device *dev)
 
 		if (diff & NETIF_F_HW_VLAN_CTAG_FILTER) {
 			if (features & NETIF_F_HW_VLAN_CTAG_FILTER) {
-				dev->features = features;
+				WRITE_ONCE(dev->features, features);
 				err |= vlan_get_rx_ctag_filter_info(dev);
 			} else {
 				vlan_drop_rx_ctag_filter_info(dev);
@@ -10034,14 +10034,14 @@ int __netdev_update_features(struct net_device *dev)
 
 		if (diff & NETIF_F_HW_VLAN_STAG_FILTER) {
 			if (features & NETIF_F_HW_VLAN_STAG_FILTER) {
-				dev->features = features;
+				WRITE_ONCE(dev->features, features);
 				err |= vlan_get_rx_stag_filter_info(dev);
 			} else {
 				vlan_drop_rx_stag_filter_info(dev);
 			}
 		}
 
-		dev->features = features;
+		WRITE_ONCE(dev->features, features);
 	}
 
 	return err < 0 ? 0 : 1;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 70bb0d2fa2ed416fdff3de71a4f752e4a1bba67a..d0c9d2ad9c3d0acb1be00eb4970d3a1ef9da030a 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -245,13 +245,13 @@ static netdev_features_t ethtool_get_feature_mask(u32 eth_cmd)
 	}
 }
 
-static int ethtool_get_one_feature(struct net_device *dev,
+static int ethtool_get_one_feature(const struct net_device *dev,
 	char __user *useraddr, u32 ethcmd)
 {
 	netdev_features_t mask = ethtool_get_feature_mask(ethcmd);
 	struct ethtool_value edata = {
 		.cmd = ethcmd,
-		.data = !!(dev->features & mask),
+		.data = !!(READ_ONCE(dev->features) & mask),
 	};
 
 	if (copy_to_user(useraddr, &edata, sizeof(edata)))
@@ -3049,14 +3049,6 @@ __dev_ethtool(struct net_device *dev, struct ifreq *ifr,
 	case ETHTOOL_SFEATURES:
 		rc = ethtool_set_features(dev, useraddr);
 		break;
-	case ETHTOOL_GTXCSUM:
-	case ETHTOOL_GRXCSUM:
-	case ETHTOOL_GSG:
-	case ETHTOOL_GTSO:
-	case ETHTOOL_GGSO:
-	case ETHTOOL_GGRO:
-		rc = ethtool_get_one_feature(dev, useraddr, ethcmd);
-		break;
 	case ETHTOOL_STXCSUM:
 	case ETHTOOL_SRXCSUM:
 	case ETHTOOL_SSG:
@@ -3178,10 +3170,20 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 	if (!netif_device_present(dev))
 		goto out_pm;
 
-	rtnl_lock();
-	rc = __dev_ethtool(dev, ifr, useraddr, ethcmd, sub_cmd, state);
-	rtnl_unlock();
-
+	switch (ethcmd) {
+	case ETHTOOL_GTXCSUM:
+	case ETHTOOL_GRXCSUM:
+	case ETHTOOL_GSG:
+	case ETHTOOL_GTSO:
+	case ETHTOOL_GGSO:
+	case ETHTOOL_GGRO:
+		rc = ethtool_get_one_feature(dev, useraddr, ethcmd);
+		break;
+	default:
+		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);
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH net-next 5/6] net: ethtool: implement lockless ETHTOOL_GFLAGS

[Eric Dumazet]

ETHTOOL_GFLAGS only reads dev->features, there is no need for RTNL protection.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ethtool/ioctl.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index d0c9d2ad9c3d0acb1be00eb4970d3a1ef9da030a..56b959495698c7cd0dfda995be7232e7cbb314a2 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -291,17 +291,18 @@ static int ethtool_set_one_feature(struct net_device *dev,
 
 static u32 __ethtool_get_flags(struct net_device *dev)
 {
+	netdev_features_t features = READ_ONCE(dev->features);
 	u32 flags = 0;
 
-	if (dev->features & NETIF_F_LRO)
+	if (features & NETIF_F_LRO)
 		flags |= ETH_FLAG_LRO;
-	if (dev->features & NETIF_F_HW_VLAN_CTAG_RX)
+	if (features & NETIF_F_HW_VLAN_CTAG_RX)
 		flags |= ETH_FLAG_RXVLAN;
-	if (dev->features & NETIF_F_HW_VLAN_CTAG_TX)
+	if (features & NETIF_F_HW_VLAN_CTAG_TX)
 		flags |= ETH_FLAG_TXVLAN;
-	if (dev->features & NETIF_F_NTUPLE)
+	if (features & NETIF_F_NTUPLE)
 		flags |= ETH_FLAG_NTUPLE;
-	if (dev->features & NETIF_F_RXHASH)
+	if (features & NETIF_F_RXHASH)
 		flags |= ETH_FLAG_RXHASH;
 
 	return flags;
@@ -2993,10 +2994,6 @@ __dev_ethtool(struct net_device *dev, struct ifreq *ifr,
 	case ETHTOOL_GPERMADDR:
 		rc = ethtool_get_perm_addr(dev, useraddr);
 		break;
-	case ETHTOOL_GFLAGS:
-		rc = ethtool_get_value(dev, useraddr, ethcmd,
-					__ethtool_get_flags);
-		break;
 	case ETHTOOL_SFLAGS:
 		rc = ethtool_set_value(dev, useraddr, __ethtool_set_flags);
 		break;
@@ -3179,6 +3176,10 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 	case ETHTOOL_GGRO:
 		rc = ethtool_get_one_feature(dev, useraddr, ethcmd);
 		break;
+	case ETHTOOL_GFLAGS:
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+					__ethtool_get_flags);
+		break;
 	default:
 		rtnl_lock();
 		rc = __dev_ethtool(dev, ifr, useraddr, ethcmd, sub_cmd, state);
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH net-next 6/6] net: ethtool: add the ability to run ethtool_[gs]et_rxnfc() without RTNL

[Eric Dumazet]

For better scalability, drivers can prefer to implement their own locking schem
(for instance one mutex per port or queue) instead of relying on RTNL.

This patch adds a new boolean field in ethtool_ops : rxnfc_parallel

Drivers can opt-in to this new behavior.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/ethtool.h |  2 ++
 net/ethtool/ioctl.c     | 43 +++++++++++++++++++++++++++--------------
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 6fd9107d3cc010dd2f1ecdb005c412145c461b6c..ee9b8054165361c9236186ff61f886e53cfa6b49 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -748,6 +748,7 @@ struct ethtool_rxfh_param {
  *	error code or zero.
  * @set_rxnfc: Set RX flow classification rules.  Returns a negative
  *	error code or zero.
+ * @rxnfc_parallel: true if @set_rxnfc, @get_rxnfc and @get_rxfh do not need RTNL.
  * @flash_device: Write a firmware image to device's flash memory.
  *	Returns a negative error code or zero.
  * @reset: Reset (part of) the device, as specified by a bitmask of
@@ -907,6 +908,7 @@ struct ethtool_ops {
 	int	(*get_rxnfc)(struct net_device *,
 			     struct ethtool_rxnfc *, u32 *rule_locs);
 	int	(*set_rxnfc)(struct net_device *, struct ethtool_rxnfc *);
+	bool	rxnfc_parallel;
 	int	(*flash_device)(struct net_device *, struct ethtool_flash *);
 	int	(*reset)(struct net_device *, u32 *);
 	u32	(*get_rxfh_key_size)(struct net_device *);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 56b959495698c7cd0dfda995be7232e7cbb314a2..e65bd08412aeaf35d276ba48e1ebe71656e486fc 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -986,26 +986,34 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 	if (rc)
 		return rc;
 
+	if (!ops->rxnfc_parallel)
+		rtnl_lock();
+
 	if (ops->get_rxfh) {
 		struct ethtool_rxfh_param rxfh = {};
 
 		rc = ops->get_rxfh(dev, &rxfh);
 		if (rc)
-			return rc;
+			goto unlock;
 
 		/* Sanity check: if symmetric-xor is set, then:
 		 * 1 - no other fields besides IP src/dst and/or L4 src/dst
 		 * 2 - If src is set, dst must also be set
 		 */
+		rc = -EINVAL;
 		if ((rxfh.input_xfrm & RXH_XFRM_SYM_XOR) &&
 		    ((info.data & ~(RXH_IP_SRC | RXH_IP_DST |
 				    RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
 		     (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
 		     (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
-			return -EINVAL;
+			goto unlock;
 	}
 
 	rc = ops->set_rxnfc(dev, &info);
+
+unlock:
+	if (!ops->rxnfc_parallel)
+		rtnl_unlock();
 	if (rc)
 		return rc;
 
@@ -1042,7 +1050,14 @@ static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
 		}
 	}
 
+	if (!ops->rxnfc_parallel)
+		rtnl_lock();
+
 	ret = ops->get_rxnfc(dev, &info, rule_buf);
+
+	if (!ops->rxnfc_parallel)
+		rtnl_unlock();
+
 	if (ret < 0)
 		goto err_out;
 
@@ -3007,18 +3022,6 @@ __dev_ethtool(struct net_device *dev, struct ifreq *ifr,
 		rc = ethtool_set_value(dev, useraddr,
 				       dev->ethtool_ops->set_priv_flags);
 		break;
-	case ETHTOOL_GRXFH:
-	case ETHTOOL_GRXRINGS:
-	case ETHTOOL_GRXCLSRLCNT:
-	case ETHTOOL_GRXCLSRULE:
-	case ETHTOOL_GRXCLSRLALL:
-		rc = ethtool_get_rxnfc(dev, ethcmd, useraddr);
-		break;
-	case ETHTOOL_SRXFH:
-	case ETHTOOL_SRXCLSRLDEL:
-	case ETHTOOL_SRXCLSRLINS:
-		rc = ethtool_set_rxnfc(dev, ethcmd, useraddr);
-		break;
 	case ETHTOOL_FLASHDEV:
 		rc = ethtool_flash_device(dev, devlink_state);
 		break;
@@ -3180,6 +3183,18 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 		rc = ethtool_get_value(dev, useraddr, ethcmd,
 					__ethtool_get_flags);
 		break;
+	case ETHTOOL_GRXFH:
+	case ETHTOOL_GRXRINGS:
+	case ETHTOOL_GRXCLSRLCNT:
+	case ETHTOOL_GRXCLSRULE:
+	case ETHTOOL_GRXCLSRLALL:
+		rc = ethtool_get_rxnfc(dev, ethcmd, useraddr);
+		break;
+	case ETHTOOL_SRXFH:
+	case ETHTOOL_SRXCLSRLDEL:
+	case ETHTOOL_SRXCLSRLINS:
+		rc = ethtool_set_rxnfc(dev, ethcmd, useraddr);
+		break;
 	default:
 		rtnl_lock();
 		rc = __dev_ethtool(dev, ifr, useraddr, ethcmd, sub_cmd, state);
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH net-next 6/6] net: ethtool: add the ability to run ethtool_[gs]et_rxnfc() without RTNL

[Willem de Bruijn]

Eric Dumazet wrote:
> For better scalability, drivers can prefer to implement their own locking schem
> (for instance one mutex per port or queue) instead of relying on RTNL.
> 
> This patch adds a new boolean field in ethtool_ops : rxnfc_parallel
> 
> Drivers can opt-in to this new behavior.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/linux/ethtool.h |  2 ++
>  net/ethtool/ioctl.c     | 43 +++++++++++++++++++++++++++--------------
>  2 files changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 6fd9107d3cc010dd2f1ecdb005c412145c461b6c..ee9b8054165361c9236186ff61f886e53cfa6b49 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -748,6 +748,7 @@ struct ethtool_rxfh_param {
>   *	error code or zero.
>   * @set_rxnfc: Set RX flow classification rules.  Returns a negative
>   *	error code or zero.
> + * @rxnfc_parallel: true if @set_rxnfc, @get_rxnfc and @get_rxfh do not need RTNL.
>   * @flash_device: Write a firmware image to device's flash memory.
>   *	Returns a negative error code or zero.
>   * @reset: Reset (part of) the device, as specified by a bitmask of
> @@ -907,6 +908,7 @@ struct ethtool_ops {
>  	int	(*get_rxnfc)(struct net_device *,
>  			     struct ethtool_rxnfc *, u32 *rule_locs);
>  	int	(*set_rxnfc)(struct net_device *, struct ethtool_rxnfc *);
> +	bool	rxnfc_parallel;

Would it make sense to make this a bit, as there already are u32 bits
at the start of the struct, with a 29-bit gap?

Re: [PATCH net-next 6/6] net: ethtool: add the ability to run ethtool_[gs]et_rxnfc() without RTNL

[Eric Dumazet]

On Thu, Jun 20, 2024 at 7:45 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Eric Dumazet wrote:
> > For better scalability, drivers can prefer to implement their own locking schem
> > (for instance one mutex per port or queue) instead of relying on RTNL.
> >
> > This patch adds a new boolean field in ethtool_ops : rxnfc_parallel
> >
> > Drivers can opt-in to this new behavior.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  include/linux/ethtool.h |  2 ++
> >  net/ethtool/ioctl.c     | 43 +++++++++++++++++++++++++++--------------
> >  2 files changed, 31 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index 6fd9107d3cc010dd2f1ecdb005c412145c461b6c..ee9b8054165361c9236186ff61f886e53cfa6b49 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -748,6 +748,7 @@ struct ethtool_rxfh_param {
> >   *   error code or zero.
> >   * @set_rxnfc: Set RX flow classification rules.  Returns a negative
> >   *   error code or zero.
> > + * @rxnfc_parallel: true if @set_rxnfc, @get_rxnfc and @get_rxfh do not need RTNL.
> >   * @flash_device: Write a firmware image to device's flash memory.
> >   *   Returns a negative error code or zero.
> >   * @reset: Reset (part of) the device, as specified by a bitmask of
> > @@ -907,6 +908,7 @@ struct ethtool_ops {
> >       int     (*get_rxnfc)(struct net_device *,
> >                            struct ethtool_rxnfc *, u32 *rule_locs);
> >       int     (*set_rxnfc)(struct net_device *, struct ethtool_rxnfc *);
> > +     bool    rxnfc_parallel;
>
> Would it make sense to make this a bit, as there already are u32 bits
> at the start of the struct, with a 29-bit gap?

Indeed, I will squash in V2 the following, thanks Willem.

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index ee9b8054165361c9236186ff61f886e53cfa6b49..2a385a9e5590ba4c629577c3679dd593d7d6e335
100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -853,6 +853,7 @@ struct ethtool_ops {
        u32     cap_link_lanes_supported:1;
        u32     cap_rss_ctx_supported:1;
        u32     cap_rss_sym_xor_supported:1;
+       u32     rxnfc_parallel:1;
        u32     supported_coalesce_params;
        u32     supported_ring_params;
        void    (*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
@@ -908,7 +909,6 @@ struct ethtool_ops {
        int     (*get_rxnfc)(struct net_device *,
                             struct ethtool_rxnfc *, u32 *rule_locs);
        int     (*set_rxnfc)(struct net_device *, struct ethtool_rxnfc *);
-       bool    rxnfc_parallel;
        int     (*flash_device)(struct net_device *, struct ethtool_flash *);
        int     (*reset)(struct net_device *, u32 *);
        u32     (*get_rxfh_key_size)(struct net_device *);

Re: [PATCH net-next 0/6] net: ethtool: reduce RTNL pressure in dev_ethtool()

[Willem de Bruijn]

Eric Dumazet wrote:
> This series is inspired by Jakub feedback for one gve patch :
> 
> https://lore.kernel.org/lkml/20240614192721.02700256@kernel.org/
> 
> Refactor dev_ethtool() and start to implement parallel operations.
> 
> Eric Dumazet (6):
>   net: ethtool: grab a netdev reference in dev_ethtool()
>   net: ethtool: add dev_ethtool_cap_check()
>   net: ethtool: perform pm duties outside of rtnl lock
>   net: ethtool: call ethtool_get_one_feature() without RTNL
>   net: ethtool: implement lockless ETHTOOL_GFLAGS
>   net: ethtool: add the ability to run ethtool_[gs]et_rxnfc() without
>     RTNL
> 
>  include/linux/ethtool.h |   2 +
>  net/core/dev.c          |  10 +--
>  net/ethtool/ioctl.c     | 159 ++++++++++++++++++++++++----------------
>  3 files changed, 101 insertions(+), 70 deletions(-)

Reviewed-by: Willem de Bruijn <willemb@google.com>

Thanks Eric!

One small comment in the ethtool_ops field, but not important.