ethtool: track custom RSS contexts in the core
Subsystems (8)
v3 cover ethtool: track custom RSS contexts in the core
From: Edward Cree <[email protected]>
Make the core responsible for tracking the set of custom RSS contexts,
their IDs, indirection tables, hash keys, and hash functions; this lets us get rid of duplicative code in drivers, and will allow us to support netlink dumps later.
This series only moves the sfc EF10 & EF100 driver over to the new API; if
the design is approved of, I plan to post a follow-up series to convert the other drivers (mvpp2, octeontx2, mlx5, sfc/siena) and remove the legacy API.
However, I don't have hardware for the drivers besides sfc, so I won't be
able to test those myself. Maintainers of those drivers (on CC), your comments on the API design would be appreciated, in particular whether the rss_ctx_max_id limit (corresponding to your fixed-size arrays of contexts) is actually needed.
Changes in v3:
* Added WangXun ngbe to patch #1, not sure if they've added WoL support since v2 or if I just missed it last time around
* Re-ordered struct ethtool_netdev_state to avoid hole (Andrew Lunn)
* Fixed some resource leaks in error handling paths (kuba)
* Added maintainers of other context-using drivers to CC
Edward Cree (7):
net: move ethtool-related netdev state into its own struct
net: ethtool: attach an IDR of custom RSS contexts to a netdevice
net: ethtool: record custom RSS contexts in the IDR
net: ethtool: let the core choose RSS context IDs
net: ethtool: add an extack parameter to new rxfh_context APIs
net: ethtool: add a mutex protecting RSS contexts
sfc: use new rxfh_context API
drivers/net/ethernet/realtek/r8169_main.c | 4 +- drivers/net/ethernet/sfc/ef10.c | 2 +- drivers/net/ethernet/sfc/ef100_ethtool.c | 5 +- drivers/net/ethernet/sfc/efx.c | 2 +- drivers/net/ethernet/sfc/efx.h | 2 +- drivers/net/ethernet/sfc/efx_common.c | 10 +- drivers/net/ethernet/sfc/ethtool.c | 5 +- drivers/net/ethernet/sfc/ethtool_common.c | 147 ++++++++++-------- drivers/net/ethernet/sfc/ethtool_common.h | 18 ++- drivers/net/ethernet/sfc/mcdi_filters.c | 133 ++++++++-------- drivers/net/ethernet/sfc/mcdi_filters.h | 8 +- drivers/net/ethernet/sfc/net_driver.h | 28 ++-- drivers/net/ethernet/sfc/rx_common.c | 64 ++------ drivers/net/ethernet/sfc/rx_common.h | 8 +- .../net/ethernet/wangxun/ngbe/ngbe_ethtool.c | 4 +- drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 2 +- drivers/net/phy/phy.c | 2 +- drivers/net/phy/phy_device.c | 5 +- drivers/net/phy/phylink.c | 2 +- include/linux/ethtool.h | 109 ++++++++++++- include/linux/netdevice.h | 7 +- net/core/dev.c | 38 +++++ net/ethtool/ioctl.c | 127 ++++++++++++++- net/ethtool/wol.c | 2 +- 24 files changed, 491 insertions(+), 243 deletions(-)
RFC PATCH v3 1/7 net: move ethtool-related netdev state into its own struct
From: Edward Cree <[email protected]>
net_dev->ethtool is a pointer to new struct ethtool_netdev_state, which
currently contains only the wol_enabled field.
---
RealTek and WangXun maintainers, I only CCed you on this patch rather than
the whole series, to avoid spamming you with a discussion that's likely not to be relevant to you. Hope that's okay.
drivers/net/ethernet/realtek/r8169_main.c | 4 ++-- drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c | 4 ++-- drivers/net/ethernet/wangxun/ngbe/ngbe_main.c | 2 +- drivers/net/phy/phy.c | 2 +- drivers/net/phy/phy_device.c | 5 +++-- drivers/net/phy/phylink.c | 2 +- include/linux/ethtool.h | 8 ++++++++ include/linux/netdevice.h | 7 ++++--- net/core/dev.c | 4 ++++ net/ethtool/ioctl.c | 2 +- net/ethtool/wol.c | 2 +- 11 files changed, 28 insertions(+), 14 deletions(-)
drivers/net/ethernet/realtek/r8169_main.c +2 -2
@@ -1455,7 +1455,7 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)if (tp->dash_type ==RTL_DASH_NONE ) {rtl_set_d3_pll_down (tp, !wolopts); - tp->dev->wol_enabled = wolopts ?1 :0 ; + tp->dev->ethtool->wol_enabled = wolopts ?1 :0 ; } }
@@ -5321,7 +5321,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)rtl_set_d3_pll_down (tp, true); }else {rtl_set_d3_pll_down (tp, false); - dev->wol_enabled =1 ; + dev->ethtool->wol_enabled =1 ; } jumbo_max =rtl_jumbo_max (tp);
drivers/net/ethernet/wangxun/ngbe/ngbe_ethtool.c +2 -2
@@ -34,9 +34,9 @@ static int ngbe_set_wol(struct net_device *netdev, wx->wol =0 ;if (wol->wolopts &WAKE_MAGIC ) wx->wol =WX_PSR_WKUP_CTL_MAG ; - netdev->wol_enabled = !!(wx->wol); + netdev->ethtool->wol_enabled = !!(wx->wol);wr32 (wx,WX_PSR_WKUP_CTL , wx->wol); -device_set_wakeup_enable (&pdev->dev, netdev->wol_enabled); +device_set_wakeup_enable (&pdev->dev, netdev->ethtool->wol_enabled);return 0 ; }
drivers/net/ethernet/wangxun/ngbe/ngbe_main.c +1 -1
@@ -636,7 +636,7 @@ static int ngbe_probe(struct pci_dev *pdev,if (wx->wol_hw_supported) wx->wol =NGBE_PSR_WKUP_CTL_MAG ; - netdev->wol_enabled = !!(wx->wol); + netdev->ethtool->wol_enabled = !!(wx->wol);wr32 (wx,NGBE_PSR_WKUP_CTL , wx->wol);device_set_wakeup_enable (&pdev->dev, wx->wol);
drivers/net/phy/phy.c +1 -1
@@ -1283,7 +1283,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)if (netdev) {struct device *parent = netdev->dev.parent; -if (netdev->wol_enabled) +if (netdev->ethtool->wol_enabled)pm_system_wakeup ();else if (device_may_wakeup (&netdev->dev))pm_wakeup_dev_event (&netdev->dev,0 , true);
drivers/net/phy/phy_device.c +3 -2
@@ -285,7 +285,7 @@ static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)if (!netdev) goto out; -if (netdev->wol_enabled) +if (netdev->ethtool->wol_enabled)return false;/* As long as not all affected network drivers support the
@@ -1859,7 +1859,8 @@ int phy_suspend(struct phy_device *phydev)return 0 ;phy_ethtool_get_wol (phydev, &wol); - phydev->wol_enabled = wol.wolopts || (netdev && netdev->wol_enabled); + phydev->wol_enabled = wol.wolopts || + (netdev && netdev->ethtool->wol_enabled);/* If the device has WOL enabled, we cannot suspend the PHY */ if (phydev->wol_enabled && !(phydrv->flags &PHY_ALWAYS_CALL_SUSPEND ))return -EBUSY ;
drivers/net/phy/phylink.c +1 -1
@@ -2172,7 +2172,7 @@ void phylink_suspend(struct phylink *pl, bool mac_wol) {ASSERT_RTNL (); -if (mac_wol && (!pl->netdev || pl->netdev->wol_enabled)) { +if (mac_wol && (!pl->netdev || pl->netdev->ethtool->wol_enabled)) {/* Wake-on-Lan enabled, MAC handling */ mutex_lock (&pl->state_mutex);
include/linux/ethtool.h +8 -0
@@ -935,6 +935,14 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,const struct ethtool_link_ksettings *cmd, u32 *dev_speed, u8 *dev_duplex); +/** +* struct ethtool_netdev_state - per-netdevice state for ethtool features +* @wol_enabled: Wake-on-LAN is enabled +*/ +struct ethtool_netdev_state { +unsigned wol_enabled:1 ; +}; +struct phy_device ;struct phy_tdr_config ;struct phy_plca_cfg ;
include/linux/netdevice.h +4 -3
@@ -79,6 +79,7 @@ struct xdp_buff;struct xdp_frame ;struct xdp_metadata_ops ;struct xdp_md ; +struct ethtool_netdev_state ;typedef u32 xdp_features_t ;
@@ -2011,8 +2012,6 @@ enum netdev_ml_priv_type {* switch driver and used to set the phys state of the * switch port. * -* @wol_enabled: Wake-on-LAN is enabled -* * @threaded: napi threaded mode is enabled * * @net_notifier_list: List of per-net netdev notifier block
@@ -2024,6 +2023,7 @@ enum netdev_ml_priv_type {* @udp_tunnel_nic_info: static structure describing the UDP tunnel * offload capabilities of the device * @udp_tunnel_nic: UDP tunnel offload state +* @ethtool: ethtool related state * @xdp_state: stores info on attached XDP BPF programs * * @nested_level: Used as a parameter of spin_lock_nested() of
@@ -2383,7 +2383,6 @@ struct net_device {struct sfp_bus *sfp_bus;struct lock_class_key *qdisc_tx_busylock;bool proto_down; - unsigned wol_enabled:1; unsigned threaded:1;struct list_head net_notifier_list;
@@ -2395,6 +2394,8 @@ struct net_device {const struct udp_tunnel_nic_info *udp_tunnel_nic_info;struct udp_tunnel_nic *udp_tunnel_nic; +struct ethtool_netdev_state *ethtool; +/* protected by rtnl_lock */ struct bpf_xdp_entity xdp_state[__MAX_XDP_MODE];
net/core/dev.c +4 -0
@@ -10747,6 +10747,9 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, dev->real_num_rx_queues = rxqs;if (netif_alloc_rx_queues (dev)) goto free_all; + dev->ethtool =kzalloc (sizeof (*dev->ethtool),GFP_KERNEL_ACCOUNT ); +if (!dev->ethtool) + goto free_all;strcpy (dev->name, name); dev->name_assign_type = name_assign_type;
@@ -10797,6 +10800,7 @@ void free_netdev(struct net_device *dev)return ; } +kfree (dev->ethtool);netif_free_tx_queues (dev);netif_free_rx_queues (dev);
net/ethtool/ioctl.c +1 -1
@@ -1461,7 +1461,7 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)if (ret)return ret; - dev->wol_enabled = !!wol.wolopts; + dev->ethtool->wol_enabled = !!wol.wolopts;ethtool_notify (dev,ETHTOOL_MSG_WOL_NTF ,NULL );return 0 ;
net/ethtool/wol.c +1 -1
@@ -137,7 +137,7 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info) ret = dev->ethtool_ops->set_wol (dev, &wol);if (ret)return ret; - dev->wol_enabled = !!wol.wolopts; + dev->ethtool->wol_enabled = !!wol.wolopts;return 1 ; }
RFC PATCH v3 2/7 net: ethtool: attach an IDR of custom RSS contexts to a netdevice
In reply to [email protected]From: Edward Cree <[email protected]>
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.
--- include/linux/ethtool.h | 43 ++++++++++++++++++++++++++++++++++++++++- net/core/dev.c | 23 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-)
include/linux/ethtool.h +42 -1
@@ -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 ;
net/core/dev.c +23 -0
@@ -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)
RFC PATCH v3 3/7 net: ethtool: record custom RSS contexts in the IDR
In reply to [email protected]From: Edward Cree <[email protected]>
Since drivers are still choosing the context IDs, we have to force the
IDR to use the ID they've chosen rather than picking one ourselves.
--- include/linux/ethtool.h | 14 ++++++++ net/ethtool/ioctl.c | 71 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 2 deletions(-)
include/linux/ethtool.h +14 -0
@@ -194,6 +194,17 @@ static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)return (u8 *)(ethtool_rxfh_context_indir (ctx) + ctx->indir_size); } +static inline size_t ethtool_rxfh_context_size (u32 indir_size,u32 key_size, +u16 priv_size) +{ +size_t indir_bytes =array_size (indir_size,sizeof (u32)); +size_t flex_len; + + flex_len =size_add (size_add (indir_bytes, key_size), +ALIGN (priv_size,sizeof (u32))); +return struct_size ((struct ethtool_rxfh_context *)0 , data, flex_len); +} +/* declare a link mode bitmap */ #define __ETHTOOL_DECLARE_LINK_MODE_MASK (name) \ DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
@@ -731,6 +742,8 @@ struct ethtool_mm_stats {* will remain unchanged. * Returns a negative error code or zero. An error code must be returned * if at least one unsupported change was requested. +* @rxfh_priv_size: size of the driver private data area the core should +* allocate for an RSS context. * @get_rxfh_context: Get the contents of the RX flow hash indirection table, * hash key, and/or hash function assiciated to the given rss context. * Returns a negative error code or zero.
@@ -824,6 +837,7 @@ struct ethtool_ops { u32 cap_link_lanes_supported:1;u32 supported_coalesce_params;u32 supported_ring_params; +u16 rxfh_priv_size;void (*get_drvinfo)(struct net_device *,struct ethtool_drvinfo *);int (*get_regs_len)(struct net_device *);void (*get_regs)(struct net_device *,struct ethtool_regs *,void *);
net/ethtool/ioctl.c +69 -2
107 lines not on this page · raw message
RFC PATCH v3 4/7 net: ethtool: let the core choose RSS context IDs
In reply to [email protected]From: Edward Cree <[email protected]>
Add a new API to create/modify/remove RSS contexts, that passes in the
newly-chosen context ID (not as a pointer) rather than leaving the driver to choose it on create. Also pass in the ctx, allowing drivers to easily use its private data area to store their hardware-specific state.
Keep the existing .set_rxfh_context API for now as a fallback, but
deprecate it.
--- include/linux/ethtool.h | 40 ++++++++++++++++++++++++-- net/core/dev.c | 11 +++++-- net/ethtool/ioctl.c | 64 +++++++++++++++++++++++++++++++---------- 3 files changed, 94 insertions(+), 21 deletions(-)
include/linux/ethtool.h +37 -3
@@ -747,10 +747,33 @@ struct ethtool_mm_stats {* @get_rxfh_context: Get the contents of the RX flow hash indirection table, * hash key, and/or hash function assiciated to the given rss context. * Returns a negative error code or zero. -* @set_rxfh_context: Create, remove and configure RSS contexts. Allows setting +* @create_rxfh_context: Create a new RSS context with the specified RX flow +* hash indirection table, hash key, and hash function. +* Arguments which are set to %NULL or zero will be populated to +* appropriate defaults by the driver. +* The &struct ethtool_rxfh_context for this context is passed in @ctx; +* note that the indir table, hkey and hfunc are not yet populated as +* of this call. The driver does not need to update these; the core +* will do so if this op succeeds. +* If the driver provides this method, it must also provide +* @modify_rxfh_context and @remove_rxfh_context. +* Returns a negative error code or zero. +* @modify_rxfh_context: Reconfigure the specified RSS context. Allows setting * the contents of the RX flow hash indirection table, hash key, and/or -* hash function associated to the given context. Arguments which are set -* to %NULL or zero will remain unchanged. +* hash function associated with the given context. +* Arguments which are set to %NULL or zero will remain unchanged. +* The &struct ethtool_rxfh_context for this context is passed in @ctx; +* note that it will still contain the *old* settings. The driver does +* not need to update these; the core will do so if this op succeeds. +* Returns a negative error code or zero. An error code must be returned +* if at least one unsupported change was requested. +* @remove_rxfh_context: Remove the specified RSS context. +* The &struct ethtool_rxfh_context for this context is passed in @ctx. +* Returns a negative error code or zero. +* @set_rxfh_context: Deprecated API to create, remove and configure RSS +* contexts. Allows setting the contents of the RX flow hash indirection +* table, hash key, and/or hash function associated to the given context. +* Arguments which are set to %NULL or zero will remain unchanged. * Returns a negative error code or zero. An error code must be returned * if at least one unsupported change was requested. * @get_channels: Get number of channels.
@@ -901,6 +924,17 @@ struct ethtool_ops { const u8 *key, const u8 hfunc);int (*get_rxfh_context)(struct net_device *,u32 *indir,u8 *key,u8 *hfunc,u32 rss_context); +int (*create_rxfh_context)(struct net_device *, +struct ethtool_rxfh_context *ctx, +const u32 *indir,const u8 *key, +const u8 hfunc,u32 rss_context); +int (*modify_rxfh_context)(struct net_device *, +struct ethtool_rxfh_context *ctx, +const u32 *indir,const u8 *key, +const u8 hfunc,u32 rss_context); +int (*remove_rxfh_context)(struct net_device *, +struct ethtool_rxfh_context *ctx, +u32 rss_context);int (*set_rxfh_context)(struct net_device *,const u32 *indir,const u8 *key,const u8 hfunc,u32 *rss_context,bool delete);
net/core/dev.c +8 -3
@@ -10860,15 +10860,20 @@ static void netdev_rss_contexts_free(struct net_device *dev)struct ethtool_rxfh_context *ctx;u32 context; -if (!dev->ethtool_ops->set_rxfh_context) +if (!dev->ethtool_ops->create_rxfh_context && + !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); +if (dev->ethtool_ops->create_rxfh_context) + dev->ethtool_ops->remove_rxfh_context (dev, ctx, context); +else + dev->ethtool_ops->set_rxfh_context (dev, indir, key, + ctx->hfunc, + &context, true);kfree (ctx); } }
net/ethtool/ioctl.c +49 -15
99 lines not on this page · raw message
RFC PATCH v3 5/7 net: ethtool: add an extack parameter to new rxfh_context APIs
In reply to [email protected]From: Edward Cree <[email protected]>
Currently passed as NULL, but will allow drivers to report back errors
when ethnl support for these ops is added.
--- include/linux/ethtool.h | 9 ++++++--- net/core/dev.c | 3 ++- net/ethtool/ioctl.c | 9 ++++++--- 3 files changed, 14 insertions(+), 7 deletions(-)
include/linux/ethtool.h +6 -3
@@ -927,14 +927,17 @@ struct ethtool_ops {int (*create_rxfh_context)(struct net_device *,struct ethtool_rxfh_context *ctx,const u32 *indir,const u8 *key, -const u8 hfunc,u32 rss_context); +const u8 hfunc,u32 rss_context, +struct netlink_ext_ack *extack);int (*modify_rxfh_context)(struct net_device *,struct ethtool_rxfh_context *ctx,const u32 *indir,const u8 *key, -const u8 hfunc,u32 rss_context); +const u8 hfunc,u32 rss_context, +struct netlink_ext_ack *extack);int (*remove_rxfh_context)(struct net_device *,struct ethtool_rxfh_context *ctx, -u32 rss_context); +u32 rss_context, +struct netlink_ext_ack *extack);int (*set_rxfh_context)(struct net_device *,const u32 *indir,const u8 *key,const u8 hfunc,u32 *rss_context,bool delete);
net/core/dev.c +2 -1
@@ -10869,7 +10869,8 @@ static void netdev_rss_contexts_free(struct net_device *dev)idr_remove (&dev->ethtool->rss_ctx, context);if (dev->ethtool_ops->create_rxfh_context) - dev->ethtool_ops->remove_rxfh_context (dev, ctx, context); + dev->ethtool_ops->remove_rxfh_context (dev, ctx, context, +NULL );else dev->ethtool_ops->set_rxfh_context(dev, indir, key, ctx->hfunc,
net/ethtool/ioctl.c +6 -3
@@ -1385,14 +1385,17 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,if (create) ret = ops->create_rxfh_context (dev, ctx, indir, hkey, rxfh.hfunc, - rxfh.rss_context); + rxfh.rss_context, +NULL );else if (delete) ret = ops->remove_rxfh_context (dev, ctx, - rxfh.rss_context); + rxfh.rss_context, +NULL );else ret = ops->modify_rxfh_context (dev, ctx, indir, hkey, rxfh.hfunc, - rxfh.rss_context); + rxfh.rss_context, +NULL ); }else { ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
RFC PATCH v3 6/7 net: ethtool: add a mutex protecting RSS contexts
In reply to [email protected]From: Edward Cree <[email protected]>
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.
--- include/linux/ethtool.h | 3 +++ net/core/dev.c | 5 +++++ net/ethtool/ioctl.c | 7 +++++++ 3 files changed, 15 insertions(+)
include/linux/ethtool.h +3 -0
@@ -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 ; };
net/core/dev.c +5 -0
@@ -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);
net/ethtool/ioctl.c +7 -0
@@ -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; }
RFC PATCH v3 7/7 sfc: use new rxfh_context API
In reply to [email protected]From: Edward Cree <[email protected]>
The core is now responsible for allocating IDs and a memory region for
us to store our state (struct efx_rss_context_priv), so we no longer need efx_alloc_rss_context_entry() and friends.
Since the contexts are now maintained by the core, use the core's lock
(net_dev->ethtool->rss_lock), rather than our own mutex (efx->rss_lock), to serialise access against changes; and remove the now-unused efx->rss_lock from struct efx_nic.
--- drivers/net/ethernet/sfc/ef10.c | 2 +- drivers/net/ethernet/sfc/ef100_ethtool.c | 5 +- drivers/net/ethernet/sfc/efx.c | 2 +- drivers/net/ethernet/sfc/efx.h | 2 +- drivers/net/ethernet/sfc/efx_common.c | 10 +- drivers/net/ethernet/sfc/ethtool.c | 5 +- drivers/net/ethernet/sfc/ethtool_common.c | 147 +++++++++++++--------- drivers/net/ethernet/sfc/ethtool_common.h | 18 ++- drivers/net/ethernet/sfc/mcdi_filters.c | 133 ++++++++++---------- drivers/net/ethernet/sfc/mcdi_filters.h | 8 +- drivers/net/ethernet/sfc/net_driver.h | 28 ++--- drivers/net/ethernet/sfc/rx_common.c | 64 ++-------- drivers/net/ethernet/sfc/rx_common.h | 8 +- 13 files changed, 213 insertions(+), 219 deletions(-)
drivers/net/ethernet/sfc/ef10.c +1 -1
@@ -1396,7 +1396,7 @@ static void efx_ef10_table_reset_mc_allocations(struct efx_nic *efx)efx_mcdi_filter_table_reset_mc_allocations (efx); nic_data->must_restore_piobufs = true;efx_ef10_forget_old_piobufs (efx); - efx->rss_context.context_id =EFX_MCDI_RSS_CONTEXT_INVALID ; + efx->rss_context.priv.context_id =EFX_MCDI_RSS_CONTEXT_INVALID ;/* Driver-created vswitches and vports must be re-created */ nic_data->must_probe_vswitching = true;
drivers/net/ethernet/sfc/ef100_ethtool.c +4 -1
@@ -58,10 +58,13 @@ const struct ethtool_ops ef100_ethtool_ops = { .get_rxfh_indir_size = efx_ethtool_get_rxfh_indir_size, .get_rxfh_key_size = efx_ethtool_get_rxfh_key_size, + .rxfh_priv_size = sizeof(struct efx_rss_context_priv), .get_rxfh = efx_ethtool_get_rxfh, .set_rxfh = efx_ethtool_set_rxfh, .get_rxfh_context = efx_ethtool_get_rxfh_context, - .set_rxfh_context = efx_ethtool_set_rxfh_context, + .create_rxfh_context = efx_ethtool_create_rxfh_context, + .modify_rxfh_context = efx_ethtool_modify_rxfh_context, + .remove_rxfh_context = efx_ethtool_remove_rxfh_context, .get_module_info = efx_ethtool_get_module_info, .get_module_eeprom = efx_ethtool_get_module_eeprom,
drivers/net/ethernet/sfc/efx.c +1 -1
@@ -299,7 +299,7 @@ static int efx_probe_nic(struct efx_nic *efx)if (efx->n_channels >1 )netdev_rss_key_fill (efx->rss_context.rx_hash_key,sizeof (efx->rss_context.rx_hash_key)); -efx_set_default_rx_indir_table (efx, &efx->rss_context); +efx_set_default_rx_indir_table (efx, efx->rss_context.rx_indir_table);/* Initialise the interrupt moderation settings */ efx->irq_mod_step_us =DIV_ROUND_UP (efx->timer_quantum_ns,1000 );
drivers/net/ethernet/sfc/efx.h +1 -1
@@ -158,7 +158,7 @@ static inline s32 efx_filter_get_rx_ids(struct efx_nic *efx, }/* RSS contexts */ -static inline bool efx_rss_active (struct efx_rss_context *ctx) +static inline bool efx_rss_active (struct efx_rss_context_priv *ctx) {return ctx->context_id !=EFX_MCDI_RSS_CONTEXT_INVALID ; }
drivers/net/ethernet/sfc/efx_common.c +4 -6
@@ -714,7 +714,7 @@ void efx_reset_down(struct efx_nic *efx, enum reset_type method)mutex_lock (&efx->mac_lock);down_write (&efx->filter_sem); -mutex_lock (&efx->rss_lock); +mutex_lock (&efx->net_dev->ethtool->rss_lock); efx->type->fini (efx); }
@@ -777,7 +777,7 @@ int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok)if (efx->type->rx_restore_rss_contexts) efx->type->rx_restore_rss_contexts (efx); -mutex_unlock (&efx->rss_lock); +mutex_unlock (&efx->net_dev->ethtool->rss_lock); efx->type->filter_table_restore (efx);up_write (&efx->filter_sem);
@@ -793,7 +793,7 @@ int efx_reset_up(struct efx_nic *efx, enum reset_type method, bool ok) fail: efx->port_initialized = false; -mutex_unlock (&efx->rss_lock); +mutex_unlock (&efx->net_dev->ethtool->rss_lock);up_write (&efx->filter_sem);mutex_unlock (&efx->mac_lock);
@@ -1000,9 +1000,7 @@ int efx_init_struct(struct efx_nic *efx, struct pci_dev *pci_dev) efx->type->rx_hash_offset - efx->type->rx_prefix_size; efx->rx_packet_ts_offset = efx->type->rx_ts_offset - efx->type->rx_prefix_size; -INIT_LIST_HEAD (&efx->rss_context.list); - efx->rss_context.context_id =EFX_MCDI_RSS_CONTEXT_INVALID ; -mutex_init (&efx->rss_lock); + efx->rss_context.priv.context_id =EFX_MCDI_RSS_CONTEXT_INVALID ; efx->vport_id =EVB_PORT_ID_ASSIGNED ;spin_lock_init (&efx->stats_lock); efx->vi_stride =EFX_DEFAULT_VI_STRIDE ;
drivers/net/ethernet/sfc/ethtool.c +4 -1
@@ -267,10 +267,13 @@ const struct ethtool_ops efx_ethtool_ops = { .set_rxnfc = efx_ethtool_set_rxnfc, .get_rxfh_indir_size = efx_ethtool_get_rxfh_indir_size, .get_rxfh_key_size = efx_ethtool_get_rxfh_key_size, + .rxfh_priv_size = sizeof(struct efx_rss_context_priv), .get_rxfh = efx_ethtool_get_rxfh, .set_rxfh = efx_ethtool_set_rxfh, .get_rxfh_context = efx_ethtool_get_rxfh_context, - .set_rxfh_context = efx_ethtool_set_rxfh_context, + .create_rxfh_context = efx_ethtool_create_rxfh_context, + .modify_rxfh_context = efx_ethtool_modify_rxfh_context, + .remove_rxfh_context = efx_ethtool_remove_rxfh_context, .get_ts_info = efx_ethtool_get_ts_info, .get_module_info = efx_ethtool_get_module_info, .get_module_eeprom = efx_ethtool_get_module_eeprom,
drivers/net/ethernet/sfc/ethtool_common.c +86 -61
199 lines not on this page · raw message
drivers/net/ethernet/sfc/ethtool_common.h +14 -4
@@ -50,10 +50,20 @@ int efx_ethtool_set_rxfh(struct net_device *net_dev, const u32 *indir, const u8 *key, const u8 hfunc);int efx_ethtool_get_rxfh_context (struct net_device *net_dev,u32 *indir,u8 *key,u8 *hfunc,u32 rss_context); -int efx_ethtool_set_rxfh_context (struct net_device *net_dev, -const u32 *indir,const u8 *key, -const u8 hfunc,u32 *rss_context, -bool delete); +int efx_ethtool_create_rxfh_context (struct net_device *net_dev, +struct ethtool_rxfh_context *ctx, +const u32 *indir,const u8 *key, +const u8 hfunc,u32 rss_context, +struct netlink_ext_ack *extack); +int efx_ethtool_modify_rxfh_context (struct net_device *net_dev, +struct ethtool_rxfh_context *ctx, +const u32 *indir,const u8 *key, +const u8 hfunc,u32 rss_context, +struct netlink_ext_ack *extack); +int efx_ethtool_remove_rxfh_context (struct net_device *net_dev, +struct ethtool_rxfh_context *ctx, +u32 rss_context, +struct netlink_ext_ack *extack);int efx_ethtool_reset (struct net_device *net_dev,u32 *flags); int efx_ethtool_get_module_eeprom(struct net_device *net_dev, struct ethtool_eeprom *ee,
drivers/net/ethernet/sfc/mcdi_filters.c +69 -64
354 lines not on this page · raw message
drivers/net/ethernet/sfc/mcdi_filters.h +2 -6
@@ -145,9 +145,9 @@ void efx_mcdi_filter_del_vlan(struct efx_nic *efx, u16 vid);void efx_mcdi_rx_free_indir_table (struct efx_nic *efx);int efx_mcdi_rx_push_rss_context_config (struct efx_nic *efx, -struct efx_rss_context *ctx, +struct efx_rss_context_priv *ctx,const u32 *rx_indir_table, -const u8 *key); +const u8 *key,bool delete);int efx_mcdi_pf_rx_push_rss_config (struct efx_nic *efx,bool user,const u32 *rx_indir_table,const u8 *key);
@@ -161,10 +161,6 @@ int efx_mcdi_push_default_indir_table(struct efx_nic *efx,int efx_mcdi_rx_pull_rss_config (struct efx_nic *efx);int efx_mcdi_rx_pull_rss_context_config (struct efx_nic *efx,struct efx_rss_context *ctx); -int efx_mcdi_get_rss_context_flags (struct efx_nic *efx,u32 context, -u32 *flags); -void efx_mcdi_set_rss_context_flags (struct efx_nic *efx, -struct efx_rss_context *ctx);void efx_mcdi_rx_restore_rss_contexts (struct efx_nic *efx);static inline void efx_mcdi_update_rx_scatter (struct efx_nic *efx)
drivers/net/ethernet/sfc/net_driver.h +14 -14
@@ -737,21 +737,24 @@ struct vfdi_status;/* The reserved RSS context value */ #define EFX_MCDI_RSS_CONTEXT_INVALID 0xffffffff/** -* struct efx_rss_context - A user-defined RSS context for filtering -* @list: node of linked list on which this struct is stored +* struct efx_rss_context_priv - driver private data for an RSS context * @context_id: the RSS_CONTEXT_ID returned by MC firmware, or * %EFX_MCDI_RSS_CONTEXT_INVALID if this context is not present on the NIC. -* For Siena, 0 if RSS is active, else %EFX_MCDI_RSS_CONTEXT_INVALID. -* @user_id: the rss_context ID exposed to userspace over ethtool. * @rx_hash_udp_4tuple: UDP 4-tuple hashing enabled +*/ +struct efx_rss_context_priv { +u32 context_id; +bool rx_hash_udp_4tuple; +}; + +/** +* struct efx_rss_context - an RSS context +* @priv: hardware-specific state * @rx_hash_key: Toeplitz hash key for this RSS context * @indir_table: Indirection table for this RSS context */ struct efx_rss_context { -struct list_head list; -u32 context_id; -u32 user_id; -bool rx_hash_udp_4tuple; +struct efx_rss_context_priv priv;u8 rx_hash_key[40 ];u32 rx_indir_table[128 ]; };
@@ -883,9 +886,7 @@ struct efx_mae;* @rx_packet_ts_offset: Offset of timestamp from start of packet data * (valid only if channel->sync_timestamps_enabled; always negative) * @rx_scatter: Scatter mode enabled for receives -* @rss_context: Main RSS context. Its @list member is the head of the list of -* RSS contexts created by user requests -* @rss_lock: Protects custom RSS context software state in @rss_context.list +* @rss_context: Main RSS context. * @vport_id: The function's vport ID, only relevant for PFs * @int_error_count: Number of internal errors seen recently * @int_error_expire: Time at which error count will be expired
@@ -1052,7 +1053,6 @@ struct efx_nic {int rx_packet_ts_offset;bool rx_scatter;struct efx_rss_context rss_context; -struct mutex rss_lock;u32 vport_id;unsigned int_error_count;
@@ -1416,9 +1416,9 @@ struct efx_nic_type { const u32 *rx_indir_table, const u8 *key);int (*rx_pull_rss_config)(struct efx_nic *efx);int (*rx_push_rss_context_config)(struct efx_nic *efx, -struct efx_rss_context *ctx, +struct efx_rss_context_priv *ctx,const u32 *rx_indir_table, -const u8 *key); +const u8 *key,bool delete);int (*rx_pull_rss_context_config)(struct efx_nic *efx,struct efx_rss_context *ctx);void (*rx_restore_rss_contexts)(struct efx_nic *efx);
drivers/net/ethernet/sfc/rx_common.c +10 -54
79 lines not on this page · raw message
drivers/net/ethernet/sfc/rx_common.h +3 -5
@@ -84,11 +84,9 @@ void efx_rx_packet_gro(struct efx_channel *channel, struct efx_rx_buffer *rx_buf, unsigned int n_frags, u8 *eh, __wsum csum); -struct efx_rss_context *efx_alloc_rss_context_entry (struct efx_nic *efx); -struct efx_rss_context *efx_find_rss_context_entry (struct efx_nic *efx,u32 id); -void efx_free_rss_context_entry (struct efx_rss_context *ctx); -void efx_set_default_rx_indir_table (struct efx_nic *efx, -struct efx_rss_context *ctx); +struct efx_rss_context_priv *efx_find_rss_context_entry (struct efx_nic *efx, +u32 id); +void efx_set_default_rx_indir_table (struct efx_nic *efx,u32 *indir);bool efx_filter_is_mc_recipient (const struct efx_filter_spec *spec); bool efx_filter_spec_equal(const struct efx_filter_spec *left,
Russell King (Oracle)
maintainerRe: [RFC PATCH v3 net-next 2/7] net: ethtool: attach an IDR of custom RSS contexts to a netdevice
In reply to [email protected]Quoted 61 lines · On Tue, Sep 12, 2023 at 03:21:37PM +0100, [email protected] wrote:
From: Edward Cree <[email protected]>
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 <[email protected]>--- include/linux/ethtool.h | 43 ++++++++++++++++++++++++++++++++++++++++- net/core/dev.c | 23 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-)include/linux/ethtool.h +37
-0@@ -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)
include/linux/ethtool.h int ethtool_virtdev_set_link_ksettings(struct net_device *dev, · line 982
Quoted 10 lines
include/linux/ethtool.h +3
-1@@ -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."
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Russell King (Oracle)
maintainerRe: [RFC PATCH v3 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts
In reply to [email protected]Quoted 38 lines · On Tue, Sep 12, 2023 at 03:21:41PM +0100, [email protected] wrote:
From: Edward Cree <[email protected]>
While this is not needed to serialise the ethtool entry points (which
are all under RTNL), drivers may have cause to asynchronously accessdev->ethtool->rss_ctx; takingdev->ethtool->rss_lockallows them to do this safely without needing to take the RTNL.Signed-off-by Edward Cree <[email protected]>--- include/linux/ethtool.h | 3 +++ net/core/dev.c | 5 +++++ net/ethtool/ioctl.c | 7 +++++++ 3 files changed, 15 insertions(+)include/linux/ethtool.h +3
-0@@ -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 ; };net/core/dev.c +0
-0
net/core/dev.c int register_netdevice(struct net_device *dev) · line 10057
Quoted 4 lines
net/core/dev.c +1
-0@@ -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?
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Edward Cree
maintainerRe: [RFC PATCH v3 net-next 2/7] net: ethtool: attach an IDR of custom RSS contexts to a netdevice
In reply to Russell King (Oracle)Quoted 6 lines · Russell King (Oracle)
include/linux/ethtool.h int ethtool_virtdev_set_link_ksettings(struct net_device *dev, · line 982
Quoted 1 line · On Tue, Sep 12, 2023 at 03:21:37PM +0100, [email protected] wrote:
include/linux/ethtool.h +1
-0+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
Edward Cree
maintainerRe: [RFC PATCH v3 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts
In reply to Russell King (Oracle)Quoted 8 lines · Russell King (Oracle)
Quoted 7 lines · On Tue, Sep 12, 2023 at 03:21:41PM +0100, [email protected] wrote:
net/core/dev.c +0
-0@@ -10054,6 +10054,7 @@ int register_netdevice(struct net_device *dev)idr_init_base (&dev->ethtool->rss_ctx,1 );
Quoted 6 lines
net/core/dev.c int register_netdevice(struct net_device *dev) · line 10057
Quoted 2 lines
net/core/dev.c +1
-0spin_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
Re: [RFC PATCH v3 net-next 2/7] net: ethtool: attach an IDR of custom RSS contexts to a netdevice
In reply to Edward CreeQuoted 9 lines · Edward Cree
Quoted 6 lines · Russell King (Oracle)
include/linux/ethtool.h int ethtool_virtdev_set_link_ksettings(struct net_device *dev, · line 982
Quoted 1 line · On Tue, Sep 12, 2023 at 03:21:37PM +0100, [email protected] wrote:
include/linux/ethtool.h +1
-0+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.
Quoted 2 lines
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!
Quoted 2 lines
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.
Re: [RFC PATCH v3 net-next 4/7] net: ethtool: let the core choose RSS context IDs
In reply to [email protected]Quoted 60 lines · On Tue, Sep 12, 2023 at 03:21:39PM +0100, [email protected] wrote:
From: Edward Cree <[email protected]>
Add a new API to create/modify/remove RSS contexts, that passes in the
newly-chosen context ID (not as a pointer) rather than leaving the driver to choose it on create. Also pass in the ctx, allowing drivers to easily use its private data area to store their hardware-specific state.
Keep the existing .set_rxfh_context API for now as a fallback, but
deprecate it.Signed-off-by Edward Cree <[email protected]>--- include/linux/ethtool.h | 40 ++++++++++++++++++++++++-- net/core/dev.c | 11 +++++-- net/ethtool/ioctl.c | 64 +++++++++++++++++++++++++++++++---------- 3 files changed, 94 insertions(+), 21 deletions(-)include/linux/ethtool.h +26
-3@@ -747,10 +747,33 @@ struct ethtool_mm_stats {* @get_rxfh_context: Get the contents of the RX flow hash indirection table, * hash key, and/or hash function assiciated to the given rss context. * Returns a negative error code or zero. -* @set_rxfh_context: Create, remove and configure RSS contexts. Allows setting +* @create_rxfh_context: Create a new RSS context with the specified RX flow +* hash indirection table, hash key, and hash function. +* Arguments which are set to %NULL or zero will be populated to +* appropriate defaults by the driver. +* The &struct ethtool_rxfh_context for this context is passed in @ctx; +* note that the indir table, hkey and hfunc are not yet populated as +* of this call. The driver does not need to update these; the core +* will do so if this op succeeds. +* If the driver provides this method, it must also provide +* @modify_rxfh_context and @remove_rxfh_context. +* Returns a negative error code or zero. +* @modify_rxfh_context: Reconfigure the specified RSS context. Allows setting * the contents of the RX flow hash indirection table, hash key, and/or -* hash function associated to the given context. Arguments which are set -* to %NULL or zero will remain unchanged. +* hash function associated with the given context. +* Arguments which are set to %NULL or zero will remain unchanged. +* The &struct ethtool_rxfh_context for this context is passed in @ctx; +* note that it will still contain the *old* settings. The driver does +* not need to update these; the core will do so if this op succeeds. +* Returns a negative error code or zero. An error code must be returned +* if at least one unsupported change was requested. +* @remove_rxfh_context: Remove the specified RSS context. +* The &struct ethtool_rxfh_context for this context is passed in @ctx. +* Returns a negative error code or zero. +* @set_rxfh_context: Deprecated API to create, remove and configure RSS +* contexts. Allows setting the contents of the RX flow hash indirection +* table, hash key, and/or hash function associated to the given context. +* Arguments which are set to %NULL or zero will remain unchanged. * Returns a negative error code or zero. An error code must be returned * if at least one unsupported change was requested. * @get_channels: Get number of channels.
include/linux/ethtool.h struct ethtool_ops { · line 930
Quoted 7 lines
include/linux/ethtool.h +4
-0@@ -901,6 +924,17 @@ struct ethtool_ops { const u8 *key, const u8 hfunc);int (*get_rxfh_context)(struct net_device *,u32 *indir,u8 *key,u8 *hfunc,u32 rss_context); +int (*create_rxfh_context)(struct net_device *, +struct ethtool_rxfh_context *ctx, +const u32 *indir,const u8 *key, +const u8 hfunc,u32 rss_context);
To return the rss_context this creates shouldn't it use a pointer to rss_context here?
Matin
Quoted 146 lines
include/linux/ethtool.h +7
-0+int (*modify_rxfh_context)(struct net_device *, +struct ethtool_rxfh_context *ctx, +const u32 *indir,const u8 *key, +const u8 hfunc,u32 rss_context); +int (*remove_rxfh_context)(struct net_device *, +struct ethtool_rxfh_context *ctx, +u32 rss_context);int (*set_rxfh_context)(struct net_device *,const u32 *indir,const u8 *key,const u8 hfunc,u32 *rss_context,bool delete);net/core/dev.c +8
-3@@ -10860,15 +10860,20 @@ static void netdev_rss_contexts_free(struct net_device *dev)struct ethtool_rxfh_context *ctx;u32 context; -if (!dev->ethtool_ops->set_rxfh_context) +if (!dev->ethtool_ops->create_rxfh_context && + !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); +if (dev->ethtool_ops->create_rxfh_context) + dev->ethtool_ops->remove_rxfh_context (dev, ctx, context); +else + dev->ethtool_ops->set_rxfh_context (dev, indir, key, + ctx->hfunc, + &context, true);kfree (ctx); } }net/ethtool/ioctl.c +49
-15@@ -1274,7 +1274,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,if (rxfh.rsvd8[0 ] || rxfh.rsvd8[1 ] || rxfh.rsvd8[2 ] || rxfh.rsvd32)return -EINVAL ;/* Most drivers don't handle rss_context, check it's 0 as well */ -if (rxfh.rss_context && !ops->set_rxfh_context) +if (rxfh.rss_context && !(ops->create_rxfh_context || + ops->set_rxfh_context))return -EOPNOTSUPP ; create = rxfh.rss_context ==ETH_RXFH_CONTEXT_ALLOC ;@@ -1349,8 +1350,28 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, } ctx->indir_size = dev_indir_size; ctx->key_size = dev_key_size; - ctx->hfunc = rxfh.hfunc; ctx->priv_size = ops->rxfh_priv_size; +/* Initialise to an empty context */ + ctx->indir_no_change = ctx->key_no_change =1 ; + ctx->hfunc =ETH_RSS_HASH_NO_CHANGE ; +if (ops->create_rxfh_context) { +int ctx_id; + +/* driver uses new API, core allocates ID */ +/* if rss_ctx_max_id is not specified (left as 0), it is +* treated as INT_MAX + 1 by idr_alloc +*/ + ctx_id =idr_alloc (&dev->ethtool->rss_ctx, ctx,1 , + dev->ethtool->rss_ctx_max_id, +GFP_KERNEL_ACCOUNT ); +/* 0 is not allowed, so treat it like an error here */ +if (ctx_id <=0 ) { +kfree (ctx); + ret = -ENOMEM ; + goto out; + } + rxfh.rss_context = ctx_id; + } }else if (rxfh.rss_context) { ctx =idr_find (&dev->ethtool->rss_ctx, rxfh.rss_context);if (!ctx) {@@ -1359,15 +1380,34 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, } } -if (rxfh.rss_context) - ret = ops->set_rxfh_context (dev, indir, hkey, rxfh.hfunc, - &rxfh.rss_context, delete); -else +if (rxfh.rss_context) { +if (ops->create_rxfh_context) { +if (create) + ret = ops->create_rxfh_context (dev, ctx, indir, + hkey, rxfh.hfunc, + rxfh.rss_context); +else if (delete) + ret = ops->remove_rxfh_context (dev, ctx, + rxfh.rss_context); +else + ret = ops->modify_rxfh_context (dev, ctx, indir, + hkey, rxfh.hfunc, + rxfh.rss_context); + }else { + ret = ops->set_rxfh_context (dev, indir, hkey, + rxfh.hfunc, + &rxfh.rss_context, delete); + } + }else { ret = ops->set_rxfh (dev, indir, hkey, rxfh.hfunc); + }if (ret) { -if (create) +if (create) {/* failed to create, free our new tracking entry */ +if (ops->create_rxfh_context) +idr_remove (&dev->ethtool->rss_ctx, rxfh.rss_context);kfree (ctx); + } goto out; }@@ -1383,12 +1423,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, dev->priv_flags |=IFF_RXFH_CONFIGURED ; }/* Update rss_ctx tracking */ -if (create) { -/* Ideally this should happen before calling the driver, -* so that we can fail more cleanly; but we don't have the -* context ID until the driver picks it, so we have to -* wait until after. -*/ +if (create && !ops->create_rxfh_context) { +/* driver uses old API, it chose context ID */ if (WARN_ON (idr_find (&dev->ethtool->rss_ctx, rxfh.rss_context))) {/* context ID reused, our tracking is screwed */ kfree (ctx);@@ -1398,8 +1434,6 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,WARN_ON (idr_alloc (&dev->ethtool->rss_ctx, ctx, rxfh.rss_context, rxfh.rss_context +1 ,GFP_KERNEL ) != rxfh.rss_context); - ctx->indir_no_change = rxfh.indir_size ==ETH_RXFH_INDIR_NO_CHANGE ; - ctx->key_no_change = !rxfh.key_size; }if (delete) {WARN_ON (idr_remove (&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);
Edward Cree
maintainerRe: [RFC PATCH v3 net-next 4/7] net: ethtool: let the core choose RSS context IDs
Quoted 8 lines · Martin Habets
include/linux/ethtool.h struct ethtool_ops { · line 930
Quoted 4 lines · On Tue, Sep 12, 2023 at 03:21:39PM +0100, [email protected] wrote:
include/linux/ethtool.h +4
-0+int (*create_rxfh_context)(struct net_device *, +struct ethtool_rxfh_context *ctx, +const u32 *indir,const u8 *key, +const u8 hfunc,u32 rss_context);To return the
rss_contextthis creates shouldn't it use a pointer torss_contexthere?
No, the whole point of this new API is that the core, not the
driver, chooses the value of rss_context. Does the commit message not explain that sufficiently?
(If you look at Patch #7 you'll see that sfc doesn't even use the
value, though other drivers might if their HW has a fixed set of slots for RSS configs.)
-ed
Re: [RFC PATCH v3 net-next 4/7] net: ethtool: let the core choose RSS context IDs
Quoted 11 lines · Edward Cree
Quoted 8 lines · Martin Habets
include/linux/ethtool.h struct ethtool_ops { · line 930
Quoted 4 lines · On Tue, Sep 12, 2023 at 03:21:39PM +0100, [email protected] wrote:
include/linux/ethtool.h +4
-0+int (*create_rxfh_context)(struct net_device *, +struct ethtool_rxfh_context *ctx, +const u32 *indir,const u8 *key, +const u8 hfunc,u32 rss_context);To return the
rss_contextthis creates shouldn't it use a pointer torss_contexthere?
Quoted 3 lines
No, the whole point of this new API is that the core, not the
driver, chooses the value ofrss_context. Does the commit message not explain that sufficiently?
Your commit describes it correct, but I had my brain wired the wrong way around. My mistake.
Martin
Quoted 5 lines
(If you look at Patch #7 you'll see that sfc doesn't even use the
value, though other drivers might if their HW has a fixed set of slots for RSS configs.)-ed