| Message ID | 20240620114711.777046-7-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 |
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?
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 *);
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);
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(-)