| Message ID | b9bdb464a3fcfcfa7ab01b1cf5e0e312c04752f5.1694443665.git.ecree.xilinx@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: track custom RSS contexts in the core | expand |
On Tue, Sep 12, 2023 at 03:21:41PM +0100, edward.cree@amd.com wrote: > From: Edward Cree <ecree.xilinx@gmail.com> > > While this is not needed to serialise the ethtool entry points (which > are all under RTNL), drivers may have cause to asynchronously access > dev->ethtool->rss_ctx; taking dev->ethtool->rss_lock allows them to > do this safely without needing to take the RTNL. > > Signed-off-by: Edward Cree <ecree.xilinx@gmail.com> > --- > include/linux/ethtool.h | 3 +++ > net/core/dev.c | 5 +++++ > net/ethtool/ioctl.c | 7 +++++++ > 3 files changed, 15 insertions(+) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 8977aa8523e3..1f8293deebd5 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -1026,11 +1026,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev, > /** > * struct ethtool_netdev_state - per-netdevice state for ethtool features > * @rss_ctx: IDR storing custom RSS context state > + * @rss_lock: Protects entries in @rss_ctx. May be taken from > + * within RTNL. > * @rss_ctx_max_id: maximum (exclusive) supported RSS context ID > * @wol_enabled: Wake-on-LAN is enabled > */ > struct ethtool_netdev_state { > struct idr rss_ctx; > + struct mutex rss_lock; > u32 rss_ctx_max_id; > u32 wol_enabled:1; > }; > diff --git a/net/core/dev.c b/net/core/dev.c > index f12767466427..2acb4d8cd4c7 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -10054,6 +10054,7 @@ int register_netdevice(struct net_device *dev) > idr_init_base(&dev->ethtool->rss_ctx, 1); > > spin_lock_init(&dev->addr_list_lock); > + mutex_init(&dev->ethtool->rss_lock); Is there a reason to split this from the idr (eventually xarray) initialisation above? Surely initialisations for a feature (rss) should all be grouped together?
On 12/09/2023 17:40, Russell King (Oracle) wrote: > On Tue, Sep 12, 2023 at 03:21:41PM +0100, edward.cree@amd.com wrote: >> diff --git a/net/core/dev.c b/net/core/dev.c >> index f12767466427..2acb4d8cd4c7 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -10054,6 +10054,7 @@ int register_netdevice(struct net_device *dev) >> idr_init_base(&dev->ethtool->rss_ctx, 1); >> >> spin_lock_init(&dev->addr_list_lock); >> + mutex_init(&dev->ethtool->rss_lock); > > Is there a reason to split this from the idr (eventually xarray) > initialisation above? Surely initialisations for a feature (rss) > should all be grouped together? No real reason; I just thought "put locks together" made sense, but I guess "put rss stuff together" makes more, can change it. -e
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 8977aa8523e3..1f8293deebd5 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -1026,11 +1026,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev, /** * struct ethtool_netdev_state - per-netdevice state for ethtool features * @rss_ctx: IDR storing custom RSS context state + * @rss_lock: Protects entries in @rss_ctx. May be taken from + * within RTNL. * @rss_ctx_max_id: maximum (exclusive) supported RSS context ID * @wol_enabled: Wake-on-LAN is enabled */ struct ethtool_netdev_state { struct idr rss_ctx; + struct mutex rss_lock; u32 rss_ctx_max_id; u32 wol_enabled:1; }; diff --git a/net/core/dev.c b/net/core/dev.c index f12767466427..2acb4d8cd4c7 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10054,6 +10054,7 @@ int register_netdevice(struct net_device *dev) idr_init_base(&dev->ethtool->rss_ctx, 1); spin_lock_init(&dev->addr_list_lock); + mutex_init(&dev->ethtool->rss_lock); netdev_set_addr_lockdep_class(dev); ret = dev_get_valid_name(net, dev, dev->name); @@ -10863,6 +10864,7 @@ static void netdev_rss_contexts_free(struct net_device *dev) if (!dev->ethtool_ops->create_rxfh_context && !dev->ethtool_ops->set_rxfh_context) return; + mutex_lock(&dev->ethtool->rss_lock); idr_for_each_entry(&dev->ethtool->rss_ctx, ctx, context) { u32 *indir = ethtool_rxfh_context_indir(ctx); u8 *key = ethtool_rxfh_context_key(ctx); @@ -10877,6 +10879,7 @@ static void netdev_rss_contexts_free(struct net_device *dev) &context, true); kfree(ctx); } + mutex_unlock(&dev->ethtool->rss_lock); } /** @@ -10990,6 +10993,8 @@ void unregister_netdevice_many_notify(struct list_head *head, if (dev->netdev_ops->ndo_uninit) dev->netdev_ops->ndo_uninit(dev); + mutex_destroy(&dev->ethtool->rss_lock); + if (skb) rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, portid, nlh); diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 5b943db75974..70cca80cea70 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -1258,6 +1258,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, u8 *rss_config; u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]); bool create = false, delete = false; + bool locked = false; /* dev->ethtool->rss_lock taken */ if (!ops->get_rxnfc || !ops->set_rxfh) return -EOPNOTSUPP; @@ -1335,6 +1336,10 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, } } + if (rxfh.rss_context) { + mutex_lock(&dev->ethtool->rss_lock); + locked = true; + } if (create) { if (delete) { ret = -EINVAL; @@ -1457,6 +1462,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, } out: + if (locked) + mutex_unlock(&dev->ethtool->rss_lock); kfree(rss_config); return ret; }