| Message ID | 9c71d5168e1ee22b40625eec53a8bb00456d60ed.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:37PM +0100, edward.cree@amd.com wrote: > From: Edward Cree <ecree.xilinx@gmail.com> > > Each context stores the RXFH settings (indir, key, and hfunc) as well > as optionally some driver private data. > Delete any still-existing contexts at netdev unregister time. > > Signed-off-by: Edward Cree <ecree.xilinx@gmail.com> > --- > include/linux/ethtool.h | 43 ++++++++++++++++++++++++++++++++++++++++- > net/core/dev.c | 23 ++++++++++++++++++++++ > 2 files changed, 65 insertions(+), 1 deletion(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 8aeefc0b4e10..c770e32d79e6 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -157,6 +157,43 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) > return index % n_rx_rings; > } > > +/** > + * struct ethtool_rxfh_context - a custom RSS context configuration > + * @indir_size: Number of u32 entries in indirection table > + * @key_size: Size of hash key, in bytes > + * @hfunc: RSS hash function identifier. One of the %ETH_RSS_HASH_* > + * @priv_size: Size of driver private data, in bytes > + * @indir_no_change: indir was not specified at create time > + * @key_no_change: hkey was not specified at create time > + */ > +struct ethtool_rxfh_context { > + u32 indir_size; > + u32 key_size; > + u8 hfunc; > + u16 priv_size; > + u8 indir_no_change:1; > + u8 key_no_change:1; > + /* private: driver private data, indirection table, and hash key are > + * stored sequentially in @data area. Use below helpers to access. > + */ > + u8 data[] __aligned(sizeof(void *)); > +}; > + > +static inline void *ethtool_rxfh_context_priv(struct ethtool_rxfh_context *ctx) > +{ > + return ctx->data; > +} > + > +static inline u32 *ethtool_rxfh_context_indir(struct ethtool_rxfh_context *ctx) > +{ > + return (u32 *)(ctx->data + ALIGN(ctx->priv_size, sizeof(u32))); > +} > + > +static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx) > +{ > + return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size); > +} > + > /* declare a link mode bitmap */ > #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name) \ > DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS) > @@ -937,10 +974,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_ctx_max_id: maximum (exclusive) supported RSS context ID > * @wol_enabled: Wake-on-LAN is enabled > */ > struct ethtool_netdev_state { > - unsigned wol_enabled:1; > + struct idr rss_ctx; https://docs.kernel.org/core-api/idr.html "The IDR interface is deprecated; please use the XArray instead."
On 12/09/2023 17:36, Russell King (Oracle) wrote: > On Tue, Sep 12, 2023 at 03:21:37PM +0100, edward.cree@amd.com wrote: >> + struct idr rss_ctx; > > https://docs.kernel.org/core-api/idr.html > > "The IDR interface is deprecated; please use the XArray instead." IDR is a wrapper around XArray these days, right? When I looked into the equivalent to use XArray directly it looked much more complicated for flexibility that really isn't needed here. Is there an explanation you can point me at of why this extremely convenient wrapper is deprecated? -e
On Wed, Sep 13, 2023 at 12:22:03PM +0100, Edward Cree wrote: > On 12/09/2023 17:36, Russell King (Oracle) wrote: > > On Tue, Sep 12, 2023 at 03:21:37PM +0100, edward.cree@amd.com wrote: > >> + struct idr rss_ctx; > > > > https://docs.kernel.org/core-api/idr.html > > > > "The IDR interface is deprecated; please use the XArray instead." > > IDR is a wrapper around XArray these days, right? Yes, but a bad one. > When I looked into the equivalent to use XArray directly it looked much > more complicated for flexibility that really isn't needed here. No, it's no more complex to use. There are a lot of _other_ things you can do with it, but every IDR call has an equivalent XArray call. And as a bonus you get a spinlock protecting you! > Is there an explanation you can point me at of why this extremely > convenient wrapper is deprecated? Because why have two APIs for the same thing? One day, I will be finished with important projects and then I'll go back to eradicating the users of the IDR.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 8aeefc0b4e10..c770e32d79e6 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -157,6 +157,43 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) return index % n_rx_rings; } +/** + * struct ethtool_rxfh_context - a custom RSS context configuration + * @indir_size: Number of u32 entries in indirection table + * @key_size: Size of hash key, in bytes + * @hfunc: RSS hash function identifier. One of the %ETH_RSS_HASH_* + * @priv_size: Size of driver private data, in bytes + * @indir_no_change: indir was not specified at create time + * @key_no_change: hkey was not specified at create time + */ +struct ethtool_rxfh_context { + u32 indir_size; + u32 key_size; + u8 hfunc; + u16 priv_size; + u8 indir_no_change:1; + u8 key_no_change:1; + /* private: driver private data, indirection table, and hash key are + * stored sequentially in @data area. Use below helpers to access. + */ + u8 data[] __aligned(sizeof(void *)); +}; + +static inline void *ethtool_rxfh_context_priv(struct ethtool_rxfh_context *ctx) +{ + return ctx->data; +} + +static inline u32 *ethtool_rxfh_context_indir(struct ethtool_rxfh_context *ctx) +{ + return (u32 *)(ctx->data + ALIGN(ctx->priv_size, sizeof(u32))); +} + +static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx) +{ + return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size); +} + /* declare a link mode bitmap */ #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name) \ DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS) @@ -937,10 +974,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_ctx_max_id: maximum (exclusive) supported RSS context ID * @wol_enabled: Wake-on-LAN is enabled */ struct ethtool_netdev_state { - unsigned wol_enabled:1; + struct idr rss_ctx; + u32 rss_ctx_max_id; + u32 wol_enabled:1; }; struct phy_device; diff --git a/net/core/dev.c b/net/core/dev.c index bb3841371349..4bbb6bda7b7e 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10050,6 +10050,9 @@ int register_netdevice(struct net_device *dev) if (ret) return ret; + /* rss ctx ID 0 is reserved for the default context, start from 1 */ + idr_init_base(&dev->ethtool->rss_ctx, 1); + spin_lock_init(&dev->addr_list_lock); netdev_set_addr_lockdep_class(dev); @@ -10852,6 +10855,24 @@ void synchronize_net(void) } EXPORT_SYMBOL(synchronize_net); +static void netdev_rss_contexts_free(struct net_device *dev) +{ + struct ethtool_rxfh_context *ctx; + u32 context; + + if (!dev->ethtool_ops->set_rxfh_context) + return; + idr_for_each_entry(&dev->ethtool->rss_ctx, ctx, context) { + u32 *indir = ethtool_rxfh_context_indir(ctx); + u8 *key = ethtool_rxfh_context_key(ctx); + + idr_remove(&dev->ethtool->rss_ctx, context); + dev->ethtool_ops->set_rxfh_context(dev, indir, key, ctx->hfunc, + &context, true); + kfree(ctx); + } +} + /** * unregister_netdevice_queue - remove device from the kernel * @dev: device @@ -10956,6 +10977,8 @@ void unregister_netdevice_many_notify(struct list_head *head, netdev_name_node_alt_flush(dev); netdev_name_node_free(dev->name_node); + netdev_rss_contexts_free(dev); + call_netdevice_notifiers(NETDEV_PRE_UNINIT, dev); if (dev->netdev_ops->ndo_uninit)