| Message ID | 255a20efdbbaa1cd26f3ae1baf4a3379bf63aa5e.1680538846.git.ecree.xilinx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: track custom RSS contexts in the core | expand |
On Mon, 3 Apr 2023 17:33:02 +0100 edward.cree@amd.com wrote: > u32 rss_ctx_max_id; > struct idr rss_ctx; > + struct mutex rss_lock; Argh, the mutex doubles the size of the state, and most drivers don't implement this feature. My thinking was to add a "ethtool state" pointer to net_device which will be allocated by ethtool on demand and can hold all ethtool related state. For psychological reasons primarily (IOW I feel like people shy away from storing state in the core because it feels expensive to add stuff to net_device while it would not seem expensive to add it to struct ethtool_netdev_state). Maybe we can do the on-demand allocation later - but could we at least wrap the ethtool-related fields in a separate struct to hold them together?
On 03/04/2023 23:03, Jakub Kicinski wrote: > On Mon, 3 Apr 2023 17:33:02 +0100 edward.cree@amd.com wrote: >> u32 rss_ctx_max_id; >> struct idr rss_ctx; >> + struct mutex rss_lock; > > Argh, the mutex doubles the size of the state, and most drivers don't > implement this feature. My thinking was to add a "ethtool state"> pointer to net_device which will be allocated by ethtool on demand > and can hold all ethtool related state. Would any other existing net_device fields go in this struct, or is it just the RSS stuff so far?
On Tue, 4 Apr 2023 13:32:04 +0100 Edward Cree wrote: > > Argh, the mutex doubles the size of the state, and most drivers don't > > implement this feature. My thinking was to add a "ethtool state"> > > pointer to net_device which will be allocated by ethtool on demand > > and can hold all ethtool related state. > > Would any other existing net_device fields go in this struct, or is > it just the RSS stuff so far? Only wol_enabled possibly, nothing else looks relevant. But wol_enabled is one bit so up to you if you want to move it.
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 91f7dad070bd..598bbffdcfd2 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2030,6 +2030,8 @@ enum netdev_ml_priv_type { * @udp_tunnel_nic: UDP tunnel offload state * @rss_ctx_max_id: maximum (exclusive) supported RSS context ID * @rss_ctx: IDR storing custom RSS context state + * @rss_lock: Protects entries in @rss_ctx. May be taken from + * within RTNL. * @xdp_state: stores info on attached XDP BPF programs * * @nested_level: Used as a parameter of spin_lock_nested() of @@ -2401,6 +2403,7 @@ struct net_device { u32 rss_ctx_max_id; struct idr rss_ctx; + struct mutex rss_lock; /* protected by rtnl_lock */ struct bpf_xdp_entity xdp_state[__MAX_XDP_MODE]; diff --git a/net/core/dev.c b/net/core/dev.c index b2cfc631761d..8309178e9d1a 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -9987,6 +9987,7 @@ int register_netdevice(struct net_device *dev) idr_init_base(&dev->rss_ctx, 1); spin_lock_init(&dev->addr_list_lock); + mutex_init(&dev->rss_lock); netdev_set_addr_lockdep_class(dev); ret = dev_get_valid_name(net, dev, dev->name); @@ -10788,6 +10789,7 @@ static void netdev_rss_contexts_free(struct net_device *dev) if (!dev->ethtool_ops->set_rxfh_context && !dev->ethtool_ops->set_rxfh_context_old) return; + mutex_lock(&dev->rss_lock); idr_for_each_entry(&dev->rss_ctx, ctx, context) { u32 *indir = ethtool_rxfh_context_indir(ctx); u8 *key = ethtool_rxfh_context_key(ctx); @@ -10804,6 +10806,7 @@ static void netdev_rss_contexts_free(struct net_device *dev) &context, true); kfree(ctx); } + mutex_unlock(&dev->rss_lock); } /** @@ -10917,6 +10920,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->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 fa0a3de1e9fb..3d1190e3abb3 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -1257,6 +1257,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->rss_lock taken */ if (!ops->get_rxnfc || !ops->set_rxfh) return -EOPNOTSUPP; @@ -1336,6 +1337,10 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, } } + if (rxfh.rss_context) { + mutex_lock(&dev->rss_lock); + locked = true; + } if (create) { if (delete) { ret = -EINVAL; @@ -1438,6 +1443,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, } out: + if (locked) + mutex_unlock(&dev->rss_lock); kfree(rss_config); return ret; }