| Message ID | 4a41069859105d8c669fe26171248aad7f88d1e9.1695838185.git.ecree.xilinx@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: track custom RSS contexts in the core | expand |
On 9/27/2023 11:13 AM, 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 | 25 ++++++++++++++++++++++++ > 2 files changed, 67 insertions(+), 1 deletion(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 8aeefc0b4e10..bb11cb2f477d 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 *)); > +}; Is it not feasible to use container_of to get to private data for the drivers? I guess this change in particular doesn't actually include any users yet...
> -----Original Message----- > From: edward.cree@amd.com <edward.cree@amd.com> > Sent: Wednesday, September 27, 2023 11:14 AM > > +/** > + * 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 { nit: */ and struct definition alignment.
On Wed, Sep 27, 2023 at 07:13:33PM +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 | 25 ++++++++++++++++++++++++ > 2 files changed, 67 insertions(+), 1 deletion(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 8aeefc0b4e10..bb11cb2f477d 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; On 32-bit architectures this has a hole after hfunc and 3 empty bytes here. Move these 2 1-bit fields before priv_size to avoid that. Martin > + /* 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: XArray of custom RSS contexts > + * @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 xarray 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 9e85a71e33ed..05e95abdfd17 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -10072,6 +10072,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 */ > + xa_init_flags(&dev->ethtool->rss_ctx, XA_FLAGS_ALLOC1); > + > spin_lock_init(&dev->addr_list_lock); > netdev_set_addr_lockdep_class(dev); > > @@ -10874,6 +10877,26 @@ void synchronize_net(void) > } > EXPORT_SYMBOL(synchronize_net); > > +static void netdev_rss_contexts_free(struct net_device *dev) > +{ > + struct ethtool_rxfh_context *ctx; > + unsigned long context; > + > + if (dev->ethtool_ops->set_rxfh_context) > + xa_for_each(&dev->ethtool->rss_ctx, context, ctx) { > + u32 *indir = ethtool_rxfh_context_indir(ctx); > + u8 *key = ethtool_rxfh_context_key(ctx); > + u32 concast = context; > + > + xa_erase(&dev->ethtool->rss_ctx, context); > + dev->ethtool_ops->set_rxfh_context(dev, indir, key, > + ctx->hfunc, &concast, > + true); > + kfree(ctx); > + } > + xa_destroy(&dev->ethtool->rss_ctx); > +} > + > /** > * unregister_netdevice_queue - remove device from the kernel > * @dev: device > @@ -10978,6 +11001,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)
On Fri, 29 Sep 2023 11:17:53 -0700 Jacob Keller wrote: > > +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 *)); > > +}; > > Is it not feasible to use container_of to get to private data for the > drivers? I guess this change in particular doesn't actually include any > users yet... That could work in general but here specifically there are also 2 variable-size arrays hiding in data[].
On Wed, 27 Sep 2023 19:13:33 +0100 edward.cree@amd.com wrote: > + struct ethtool_rxfh_context *ctx; > + unsigned long context; > + > + if (dev->ethtool_ops->set_rxfh_context) Can there be contexts if there's no callback to create them? Perhaps you need this for later patches but would be good to mention "why" in the commit message. > + xa_for_each(&dev->ethtool->rss_ctx, context, ctx) { > + u32 *indir = ethtool_rxfh_context_indir(ctx); > + u8 *key = ethtool_rxfh_context_key(ctx); > + u32 concast = context; > + > + xa_erase(&dev->ethtool->rss_ctx, context); > + dev->ethtool_ops->set_rxfh_context(dev, indir, key, > + ctx->hfunc, &concast, > + true); > + kfree(ctx); > + }
On Wed, 27 Sep 2023 19:13:33 +0100 edward.cree@amd.com wrote: > /** > * struct ethtool_netdev_state - per-netdevice state for ethtool features > + * @rss_ctx: XArray of custom RSS contexts > + * @rss_ctx_max_id: maximum (exclusive) supported RSS context ID Is this one set by the driver? How would it be set? It'd be good if drivers didn't access ethtool state directly. Makes core easier to refactor if the API is constrained. > * @wol_enabled: Wake-on-LAN is enabled > */ > struct ethtool_netdev_state { > - unsigned wol_enabled:1; > + struct xarray rss_ctx; > + u32 rss_ctx_max_id; > + u32 wol_enabled:1;
On 05/10/2023 00:00, Jakub Kicinski wrote: > On Wed, 27 Sep 2023 19:13:33 +0100 edward.cree@amd.com wrote: >> + struct ethtool_rxfh_context *ctx; >> + unsigned long context; >> + >> + if (dev->ethtool_ops->set_rxfh_context) > > Can there be contexts if there's no callback to create them? I don't believe so. But maybe making that load-bearing isn't great... > Perhaps you need this for later patches but would be good to > mention "why" in the commit message. Well, the loop below tries to call ->set_rxfh_context, which wouldn't go too well if there's no callback. But I guess the code makes more sense to read if this just guards the actual call and not the kfree. -ed >> + xa_for_each(&dev->ethtool->rss_ctx, context, ctx) { >> + u32 *indir = ethtool_rxfh_context_indir(ctx); >> + u8 *key = ethtool_rxfh_context_key(ctx); >> + u32 concast = context; >> + >> + xa_erase(&dev->ethtool->rss_ctx, context); >> + dev->ethtool_ops->set_rxfh_context(dev, indir, key, >> + ctx->hfunc, &concast, >> + true); >> + kfree(ctx); >> + }
On 05/10/2023 00:10, Jakub Kicinski wrote: > On Wed, 27 Sep 2023 19:13:33 +0100 edward.cree@amd.com wrote: >> /** >> * struct ethtool_netdev_state - per-netdevice state for ethtool features >> + * @rss_ctx: XArray of custom RSS contexts >> + * @rss_ctx_max_id: maximum (exclusive) supported RSS context ID > > Is this one set by the driver? How would it be set? I was thinking drivers would just assign this directly in their probe routine. > It'd be good if drivers didn't access ethtool state directly. > Makes core easier to refactor if the API is constrained. Would you prefer it as a getter in the ethtool ops? The core would call it every time a new context is being allocated. (In any case, arguably I shouldn't add it in this patch as it's not used until patch #4; I'll fix that in v5 either way.)
On Thu, 5 Oct 2023 19:43:36 +0100 Edward Cree wrote: > > Is this one set by the driver? How would it be set? > > I was thinking drivers would just assign this directly in their > probe routine. > > > It'd be good if drivers didn't access ethtool state directly. > > Makes core easier to refactor if the API is constrained. > > Would you prefer it as a getter in the ethtool ops? The core > would call it every time a new context is being allocated. If ops work that'd be simplest. I was worried that the exact limit may need to be established at runtime based on FW/HW gen, and that's why you place it here. But that's a more general problem with the current simplistic approach of sticking all the capabilities into ops. If you only need a static cofing we can go with ops and add runtime corrections for all caps later on.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 8aeefc0b4e10..bb11cb2f477d 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: XArray of custom RSS contexts + * @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 xarray 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 9e85a71e33ed..05e95abdfd17 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -10072,6 +10072,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 */ + xa_init_flags(&dev->ethtool->rss_ctx, XA_FLAGS_ALLOC1); + spin_lock_init(&dev->addr_list_lock); netdev_set_addr_lockdep_class(dev); @@ -10874,6 +10877,26 @@ void synchronize_net(void) } EXPORT_SYMBOL(synchronize_net); +static void netdev_rss_contexts_free(struct net_device *dev) +{ + struct ethtool_rxfh_context *ctx; + unsigned long context; + + if (dev->ethtool_ops->set_rxfh_context) + xa_for_each(&dev->ethtool->rss_ctx, context, ctx) { + u32 *indir = ethtool_rxfh_context_indir(ctx); + u8 *key = ethtool_rxfh_context_key(ctx); + u32 concast = context; + + xa_erase(&dev->ethtool->rss_ctx, context); + dev->ethtool_ops->set_rxfh_context(dev, indir, key, + ctx->hfunc, &concast, + true); + kfree(ctx); + } + xa_destroy(&dev->ethtool->rss_ctx); +} + /** * unregister_netdevice_queue - remove device from the kernel * @dev: device @@ -10978,6 +11001,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)