Skip to contentSkip to conversation

ethtool: track custom RSS contexts in the core

PATCH Superseded [email protected] 4 participants 21 messages Jun 18, 2024 – Jun 20, 2024

Subsystems (8)

v5 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v5 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;
other drivers (mvpp2, octeontx2, mlx5, sfc/siena) can be converted afterwards and the legacy API removed.

Changes in v5:
* Rebased on top of Ahmed Zaki's struct ethtool_rxfh_param API
* Moved rxfh_max_context_id to the ethtool ops struct

Changes in v4:
* replaced IDR with XArray
* grouped initialisations together in patch 6
* dropped RFC tags

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 XArray of custom RSS contexts to a netdevice
net: ethtool: record custom RSS contexts in the XArray
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      |   4 +
 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            |   4 +
 drivers/net/ethernet/sfc/ethtool_common.c     | 148 ++++++++++--------
 drivers/net/ethernet/sfc/ethtool_common.h     |  12 ++
 drivers/net/ethernet/sfc/mcdi_filters.c       | 135 ++++++++--------
 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                       | 107 +++++++++++++
 include/linux/netdevice.h                     |   7 +-
 net/core/dev.c                                |  43 +++++
 net/ethtool/ioctl.c                           | 110 ++++++++++++-
 net/ethtool/wol.c                             |   2 +-
 24 files changed, 480 insertions(+), 235 deletions(-)
#1 lore raw

PATCH v5 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.

---
 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
index 9246ea2118ff..714d2e804694 100644
@@ -1608,7 +1608,7 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)
 
 	if (!tp->dash_enabled) {
 		rtl_set_d3_pll_down(tp, !wolopts);
-		tp->dev->wol_enabled = wolopts ? 1 : 0;
+		tp->dev->ethtool->wol_enabled = wolopts ? 1 : 0;
 	}
 }
 
@@ -5478,7 +5478,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
index 46a5a3e95202..e868f7ef4920 100644
@@ -37,9 +37,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
index e894e01d030d..a8119de60deb 100644
@@ -650,7 +650,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
index c4236564c1cd..785182fa5fe0 100644
@@ -1309,7 +1309,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
index 6c6ec9475709..473cbc1d497b 100644
@@ -296,7 +296,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
@@ -1984,7 +1984,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
index 02427378acfd..38e4e7c0f7d5 100644
@@ -2275,7 +2275,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
index 6fd9107d3cc0..8cd6b3c993f1 100644
@@ -998,6 +998,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
index 85111502cf8f..bd88ecb61598 100644
@@ -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;
 
@@ -1985,8 +1986,6 @@ enum netdev_reg_state {
  *			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
@@ -1998,6 +1997,7 @@ enum netdev_reg_state {
  *	@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
@@ -2372,7 +2372,6 @@ struct net_device {
 	struct lock_class_key	*qdisc_tx_busylock;
 	bool			proto_down;
 	bool			threaded;
-	unsigned		wol_enabled:1;
 
 	struct list_head	net_notifier_list;
 
@@ -2383,6 +2382,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
index c361a7b69da8..29351bbea803 100644
@@ -11065,6 +11065,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;
@@ -11115,6 +11118,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
index e645d751a5e8..998571f05deb 100644
@@ -1503,7 +1503,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
index 0ed56c9ac1bc..a39d8000d808 100644
@@ -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;
 }
 
#2 lore raw

Re: [PATCH v5 net-next 1/7] net: move ethtool-related netdev state into its own struct

net/core/dev.c struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, · line 11068

Quoted 9 lines · On 2024-06-18 15:44, [email protected] wrote:
net/core/dev.c +1 -0
index c361a7b69da8..29351bbea803 100644
@@ -11065,6 +11065,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);

Why GFP_KERNEL_ACCOUNT instead of just GFP_KERNEL?

net/core/dev.c void free_netdev(struct net_device *dev) · line 11121

Quoted 10 lines
net/core/dev.c +3 -0
+	if (!dev->ethtool)
+		goto free_all;
 
 	strcpy(dev->name, name);
 	dev->name_assign_type = name_assign_type;
@@ -11115,6 +11118,7 @@ void free_netdev(struct net_device *dev)
 		return;
 	}
 
+	kfree(dev->ethtool);

dev->ethtool = NULL?

maintainer
#3 lore raw

Re: [PATCH v5 net-next 1/7] net: move ethtool-related netdev state into its own struct

Quoted 7 lines · On Tue, 18 Jun 2024 16:05:13 -0700 David Wei wrote:

net/core/dev.c struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, · line 11068

Quoted 5 lines
net/core/dev.c +1 -0
@@ -11065,6 +11065,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);  

Why GFP_KERNEL_ACCOUNT instead of just GFP_KERNEL?

netdevs can be created by a user, think veth getting created in a container. So we need to account the allocated memory towards the memory limit of the current user.

Quoted 9 lines
Quoted 9 lines
net/core/dev.c +2 -0
+	if (!dev->ethtool)
+		goto free_all;
 
 	strcpy(dev->name, name);
 	dev->name_assign_type = name_assign_type;
@@ -11115,6 +11118,7 @@ void free_netdev(struct net_device *dev)
 		return;
 	}
 
Quoted 3 lines

net/core/dev.c void free_netdev(struct net_device *dev) · line 11121

Quoted 1 line
net/core/dev.c +1 -0
+	kfree(dev->ethtool);  

dev->ethtool = NULL?

defensive programming is sometimes permitted by not encouraged :)

#4 lore raw

Re: [PATCH v5 net-next 1/7] net: move ethtool-related netdev state into its own struct

Quoted 9 lines · Jakub Kicinski
Quoted 7 lines · On Tue, 18 Jun 2024 16:05:13 -0700 David Wei wrote:

net/core/dev.c struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, · line 11068

Quoted 5 lines
net/core/dev.c +1 -0
@@ -11065,6 +11065,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);  

Why GFP_KERNEL_ACCOUNT instead of just GFP_KERNEL?

Quoted 3 lines

netdevs can be created by a user, think veth getting created in a container. So we need to account the allocated memory towards the memory limit of the current user.

(Y)

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
net/core/dev.c +2 -0
+	if (!dev->ethtool)
+		goto free_all;
 
 	strcpy(dev->name, name);
 	dev->name_assign_type = name_assign_type;
@@ -11115,6 +11118,7 @@ void free_netdev(struct net_device *dev)
 		return;
 	}
 
Quoted 5 lines
Quoted 3 lines

net/core/dev.c void free_netdev(struct net_device *dev) · line 11121

Quoted 1 line
net/core/dev.c +1 -0
+	kfree(dev->ethtool);  

dev->ethtool = NULL?

defensive programming is sometimes permitted by not encouraged :)

I've been here enough to know this bit!

But, kfree(foo) followed by foo = NULL is a common pattern I see in kernel code. free_netdev() does it a bit further down. Is this pattern deprecated, then?

maintainer
#5 lore raw

Re: [PATCH v5 net-next 1/7] net: move ethtool-related netdev state into its own struct

Quoted 9 lines · On Tue, 18 Jun 2024 16:45:56 -0700 David Wei wrote:
Quoted 3 lines
Quoted 1 line

dev->ethtool = NULL?

defensive programming is sometimes permitted by not encouraged :)

I've been here enough to know this bit!

But, kfree(foo) followed by foo = NULL is a common pattern I see in kernel code. free_netdev() does it a bit further down. Is this pattern deprecated, then?

I wouldn't say its deprecated. But I'd certainly let the author choose not to do this, especially on a straightforward path like free_netdev()

#6 lore raw

PATCH v5 2/7 net: ethtool: attach an XArray 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 | 42 +++++++++++++++++++++++++++++++++++++++++
 net/core/dev.c          | 31 ++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+)
include/linux/ethtool.h +42 -0
index 8cd6b3c993f1..a68b83a6d61f 100644
@@ -159,6 +159,46 @@ 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
+ * @priv_size: Size of driver private data, in bytes
+ * @hfunc: RSS hash function identifier.  One of the %ETH_RSS_HASH_*
+ * @input_xfrm: Defines how the input data is transformed. Valid values are one
+ *	of %RXH_XFRM_*.
+ * @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;
+	u16 priv_size;
+	u8 hfunc;
+	u8 input_xfrm;
+	u8 indir_configured:1;
+	u8 key_configured: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)
@@ -1000,9 +1040,11 @@ 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
  * @wol_enabled:	Wake-on-LAN is enabled
  */
 struct ethtool_netdev_state {
+	struct xarray		rss_ctx;
 	unsigned		wol_enabled:1;
 };
 
net/core/dev.c +31 -0
index 29351bbea803..cc85baa3624b 100644
@@ -10285,6 +10285,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);
 
@@ -11184,6 +11187,32 @@ 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;
+
+	xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
+		struct ethtool_rxfh_param rxfh;
+
+		rxfh.indir = ethtool_rxfh_context_indir(ctx);
+		rxfh.key = ethtool_rxfh_context_key(ctx);
+		rxfh.hfunc = ctx->hfunc;
+		rxfh.input_xfrm = ctx->input_xfrm;
+		rxfh.rss_context = context;
+		rxfh.rss_delete = true;
+
+		xa_erase(&dev->ethtool->rss_ctx, context);
+		if (dev->ethtool_ops->cap_rss_ctx_supported)
+			dev->ethtool_ops->set_rxfh(dev, &rxfh, NULL);
+		else /* can't happen */
+			pr_warn_once("No callback to remove RSS context from %s\n",
+				     netdev_name(dev));
+		kfree(ctx);
+	}
+	xa_destroy(&dev->ethtool->rss_ctx);
+}
+
 /**
  *	unregister_netdevice_queue - remove device from the kernel
  *	@dev: device
@@ -11287,6 +11316,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)
maintainer
#7 lore raw

Re: [PATCH v5 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

net/core/dev.c void synchronize_net(void) · line 11210

Quoted 6 lines · On Tue, 18 Jun 2024 23:44:22 +0100 [email protected] wrote:
net/core/dev.c +6 -0
+		xa_erase(&dev->ethtool->rss_ctx, context);
+		if (dev->ethtool_ops->cap_rss_ctx_supported)
+			dev->ethtool_ops->set_rxfh(dev, &rxfh, NULL);
+		else /* can't happen */
+			pr_warn_once("No callback to remove RSS context from %s\n",
+				     netdev_name(dev));

nit: I'd be tempted to call dev->ethtool_ops->set_rxfh()
unconditionally. Is there a legit scenario where context may exist but there's no set_rxfh callback?

If you prefer to keep the check - netdev_warn_once() ?

maintainer
#8 lore raw

Re: [PATCH v5 net-next 2/7] net: ethtool: attach an XArray of custom RSS contexts to a netdevice

In reply to [email protected]

include/linux/ethtool.h static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) · line 180

Quoted 5 lines · On Tue, 18 Jun 2024 23:44:22 +0100 [email protected] wrote:

+ * @indir_no_change: indir was not specified at create time
+ * @key_no_change: hkey was not specified at create time

include/linux/ethtool.h +2 -0
+	u8 indir_configured:1;
+	u8 key_configured:1;

looks like names in the kdoc also need to be updated

#9 lore raw

PATCH v5 3/7 net: ethtool: record custom RSS contexts in the XArray

In reply to [email protected]

From: Edward Cree <[email protected]>

Since drivers are still choosing the context IDs, we have to force the
XArray to use the ID they've chosen rather than picking one ourselves, and handle the case where they give us an ID that's already in use.

---
 include/linux/ethtool.h | 14 ++++++++
 net/ethtool/ioctl.c     | 74 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 1 deletion(-)
include/linux/ethtool.h +14 -0
index a68b83a6d61f..5bef46fdcb94 100644
@@ -199,6 +199,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)
@@ -709,6 +720,8 @@ struct ethtool_rxfh_param {
  *	contexts.
  * @cap_rss_sym_xor_supported: indicates if the driver supports symmetric-xor
  *	RSS.
+ * @rxfh_priv_size: size of the driver private data area the core should
+ *	allocate for an RSS context (in &struct ethtool_rxfh_context).
  * @supported_coalesce_params: supported types of interrupt coalescing.
  * @supported_ring_params: supported ring params.
  * @get_drvinfo: Report driver/device information. Modern drivers no
@@ -892,6 +905,7 @@ struct ethtool_ops {
 	u32     cap_link_lanes_supported:1;
 	u32     cap_rss_ctx_supported:1;
 	u32	cap_rss_sym_xor_supported:1;
+	u16	rxfh_priv_size;
 	u32	supported_coalesce_params;
 	u32	supported_ring_params;
 	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
net/ethtool/ioctl.c +73 -1

108 lines not on this page · raw message

#10 lore raw

Re: [PATCH v5 net-next 3/7] net: ethtool: record custom RSS contexts in the XArray

Quoted 19 lines · On 2024-06-18 15:44, [email protected] wrote:

From: Edward Cree <[email protected]>

Since drivers are still choosing the context IDs, we have to force the
XArray to use the ID they've chosen rather than picking one ourselves, and handle the case where they give us an ID that's already in use.

---
 include/linux/ethtool.h | 14 ++++++++
 net/ethtool/ioctl.c     | 74 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 1 deletion(-)
include/linux/ethtool.h +0 -0
index a68b83a6d61f..5bef46fdcb94 100644
@@ -199,6 +199,17 @@ static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
 	return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size);
 }

include/linux/ethtool.h static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx) · line 210

Quoted 10 lines
include/linux/ethtool.h +9 -0
 
+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);

ctx->data is [ priv | indir_tbl | key ] but only priv and indir_tbl are aligned to sizeof(u32). Why does key not need to be aligned? Is it guaranteed to be 40 bytes?

bnxt has key_size = HW_HASH_KEY_SIZE = 40

mlx5 has key_size = mlx5e_rss_params_hash::toeplitz_hash_key = u8[40]

maintainer
#11 lore raw

Re: [PATCH v5 net-next 3/7] net: ethtool: record custom RSS contexts in the XArray

Quoted 21 lines · David Wei
Quoted 20 lines · On 2024-06-18 15:44, [email protected] wrote:

From: Edward Cree <[email protected]>

Since drivers are still choosing the context IDs, we have to force the
XArray to use the ID they've chosen rather than picking one ourselves, and handle the case where they give us an ID that's already in use.

---
 include/linux/ethtool.h | 14 ++++++++
 net/ethtool/ioctl.c     | 74 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 1 deletion(-)
include/linux/ethtool.h +0 -0
index a68b83a6d61f..5bef46fdcb94 100644
@@ -199,6 +199,17 @@ static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
 	return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size);
 }
 
Quoted 12 lines

include/linux/ethtool.h static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx) · line 210

Quoted 9 lines
include/linux/ethtool.h +9 -0
+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);

ctx->data is [ priv | indir_tbl | key ] but only priv and indir_tbl are aligned to sizeof(u32). Why does key not need to be aligned?

Because it's a u8[], whereas indir_tbl is a u32[]. (And priv is aligned to sizeof(void *), so that drivers can put
whatever struct they like there and have proper alignment.)

Quoted 1 line

Is it guaranteed to be 40 bytes?

Not AFAIK, though that certainly is a popular size.

-ed

#12 lore raw

PATCH v5 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 API for now as a fallback, but deprecate it
for custom contexts (rss_context != 0).

---
 include/linux/ethtool.h | 37 +++++++++++++++++++++++++++++++++
 net/core/dev.c          |  5 ++++-
 net/ethtool/ioctl.c     | 46 ++++++++++++++++++++++++++++++-----------
 3 files changed, 75 insertions(+), 13 deletions(-)
include/linux/ethtool.h +37 -0
index 5bef46fdcb94..4fec3c2876aa 100644
@@ -722,6 +722,10 @@ struct ethtool_rxfh_param {
  *	RSS.
  * @rxfh_priv_size: size of the driver private data area the core should
  *	allocate for an RSS context (in &struct ethtool_rxfh_context).
+ * @rxfh_max_context_id: maximum (exclusive) supported RSS context ID.  If this
+ *	is zero then the core may choose any (nonzero) ID, otherwise the core
+ *	will only use IDs strictly less than this value, as the @rss_context
+ *	argument to @create_rxfh_context and friends.
  * @supported_coalesce_params: supported types of interrupt coalescing.
  * @supported_ring_params: supported ring params.
  * @get_drvinfo: Report driver/device information. Modern drivers no
@@ -818,6 +822,29 @@ struct ethtool_rxfh_param {
  *	will remain unchanged.
  *	Returns a negative error code or zero. An error code must be returned
  *	if at least one unsupported change was requested.
+ * @create_rxfh_context: Create a new RSS context with the specified RX flow
+ *	hash indirection table, hash key, and hash function.
+ *	Parameters 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 with the given context.
+ *	Parameters 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.
  * @get_channels: Get number of channels.
  * @set_channels: Set number of channels.  Returns a negative error code or
  *	zero.
@@ -906,6 +933,7 @@ struct ethtool_ops {
 	u32     cap_rss_ctx_supported:1;
 	u32	cap_rss_sym_xor_supported:1;
 	u16	rxfh_priv_size;
+	u32	rxfh_max_context_id;
 	u32	supported_coalesce_params;
 	u32	supported_ring_params;
 	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
@@ -968,6 +996,15 @@ struct ethtool_ops {
 	int	(*get_rxfh)(struct net_device *, struct ethtool_rxfh_param *);
 	int	(*set_rxfh)(struct net_device *, struct ethtool_rxfh_param *,
 			    struct netlink_ext_ack *extack);
+	int	(*create_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       const struct ethtool_rxfh_param *rxfh);
+	int	(*modify_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       const struct ethtool_rxfh_param *rxfh);
+	int	(*remove_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       u32 rss_context);
 	void	(*get_channels)(struct net_device *, struct ethtool_channels *);
 	int	(*set_channels)(struct net_device *, struct ethtool_channels *);
 	int	(*get_dump_flag)(struct net_device *, struct ethtool_dump *);
net/core/dev.c +4 -1
index cc85baa3624b..c4e880397c07 100644
@@ -11203,7 +11203,10 @@ static void netdev_rss_contexts_free(struct net_device *dev)
 		rxfh.rss_delete = true;
 
 		xa_erase(&dev->ethtool->rss_ctx, context);
-		if (dev->ethtool_ops->cap_rss_ctx_supported)
+		if (dev->ethtool_ops->create_rxfh_context)
+			dev->ethtool_ops->remove_rxfh_context(dev, ctx,
+							      context);
+		else if (dev->ethtool_ops->cap_rss_ctx_supported)
 			dev->ethtool_ops->set_rxfh(dev, &rxfh, NULL);
 		else /* can't happen */
 			pr_warn_once("No callback to remove RSS context from %s\n",
net/ethtool/ioctl.c +34 -12

74 lines not on this page · raw message

maintainer
#13 lore raw

Re: [PATCH v5 net-next 4/7] net: ethtool: let the core choose RSS context IDs

Quoted 4 lines · On Tue, 18 Jun 2024 23:44:24 +0100 [email protected] wrote:

+ * @create_rxfh_context: Create a new RSS context with the specified RX flow
+ * hash indirection table, hash key, and hash function.
+ * Parameters which are set to %NULL or zero will be populated to
+ * appropriate defaults by the driver.

The defaults will most likely "inherit" whatever is set in context 0. So the driver _may_ init the values according to its preferences but they will not be used by the core (specifically not reported to user space via ethtool netlink)

Does that match your thinking?

Quoted 33 lines

+ * 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 with the given context.
+ * Parameters 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.
* @get_channels: Get number of channels.
* @set_channels: Set number of channels. Returns a negative error code or
* zero.

include/linux/ethtool.h +1 -0
@@ -906,6 +933,7 @@ struct ethtool_ops {
 	u32     cap_rss_ctx_supported:1;
 	u32	cap_rss_sym_xor_supported:1;
 	u16	rxfh_priv_size;
+	u32	rxfh_max_context_id;
 	u32	supported_coalesce_params;
 	u32	supported_ring_params;
 	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
@@ -968,6 +996,15 @@ struct ethtool_ops {
 	int	(*get_rxfh)(struct net_device *, struct ethtool_rxfh_param *);
 	int	(*set_rxfh)(struct net_device *, struct ethtool_rxfh_param *,

include/linux/ethtool.h struct ethtool_ops { · line 1007

Quoted 10 lines
include/linux/ethtool.h +9 -0
 			    struct netlink_ext_ack *extack);
+	int	(*create_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       const struct ethtool_rxfh_param *rxfh);
+	int	(*modify_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       const struct ethtool_rxfh_param *rxfh);
+	int	(*remove_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       u32 rss_context);

Can we make remove void? It's sort of a cleanup, cleanups which can fail make life hard.

maintainer
#14 lore raw

Re: [PATCH v5 net-next 4/7] net: ethtool: let the core choose RSS context IDs

Quoted 6 lines · On Wed, 19 Jun 2024 10:24:35 -0700 Jakub Kicinski wrote:
Quoted 4 lines · On Tue, 18 Jun 2024 23:44:24 +0100 [email protected] wrote:

+ * @create_rxfh_context: Create a new RSS context with the specified RX flow
+ * hash indirection table, hash key, and hash function.
+ * Parameters which are set to %NULL or zero will be populated to
+ * appropriate defaults by the driver.

Quoted 6 lines

The defaults will most likely "inherit" whatever is set in context 0. So the driver _may_ init the values according to its preferences but they will not be used by the core (specifically not reported to user space via ethtool netlink)

Does that match your thinking?

I was thinking about the key, hfunc and xfrm. Indirection table needs to get reported.

maintainer
#15 lore raw

Re: [PATCH v5 net-next 4/7] net: ethtool: let the core choose RSS context IDs

In reply to Jakub Kicinski
Quoted 6 lines · Jakub Kicinski
Quoted 4 lines · On Tue, 18 Jun 2024 23:44:24 +0100 [email protected] wrote:

+ * @create_rxfh_context: Create a new RSS context with the specified RX flow
+ * hash indirection table, hash key, and hash function.
+ * Parameters which are set to %NULL or zero will be populated to
+ * appropriate defaults by the driver.

Quoted 6 lines

The defaults will most likely "inherit" whatever is set in context 0. So the driver _may_ init the values according to its preferences but they will not be used by the core (specifically not reported to user space via ethtool netlink)

Does that match your thinking?

Yes, that was what I had in mind.

Quoted 1 line

Indirection table needs to get reported.

Okay, I'll alter the documentation to say so, that notwithstanding
this bit...

Quoted 4 lines
Quoted 4 lines

+ * 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.

... at least indir MUST, and the others MAY, be filled in by the
driver if they weren't specified in params.
(sfc does this already, because it uses the ctx as a place to store
the new table and/or key if it has to generate them.)

Quoted 6 lines

include/linux/ethtool.h struct ethtool_ops { · line 1007

Quoted 3 lines
include/linux/ethtool.h +3 -0
+	int	(*remove_rxfh_context)(struct net_device *,
+				       struct ethtool_rxfh_context *ctx,
+				       u32 rss_context);

Can we make remove void? It's sort of a cleanup, cleanups which can fail make life hard.

At least on sfc it's possible for it to fail. Apart from anything
else, I don't think there's anything in the core to catch the case of trying to remove a context while there are still ntuple filters targeting it; I believe that gets all the way down to the firmware, which responds EBUSY or something. If that happens, we don't want to pretend it worked and delete the context from the xarray.

-ed

#16 lore raw

PATCH v5 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          | 2 +-
 net/ethtool/ioctl.c     | 9 ++++++---
 3 files changed, 13 insertions(+), 7 deletions(-)
include/linux/ethtool.h +6 -3
index 4fec3c2876aa..f781894a7cb2 100644
@@ -998,13 +998,16 @@ struct ethtool_ops {
 			    struct netlink_ext_ack *extack);
 	int	(*create_rxfh_context)(struct net_device *,
 				       struct ethtool_rxfh_context *ctx,
-				       const struct ethtool_rxfh_param *rxfh);
+				       const struct ethtool_rxfh_param *rxfh,
+				       struct netlink_ext_ack *extack);
 	int	(*modify_rxfh_context)(struct net_device *,
 				       struct ethtool_rxfh_context *ctx,
-				       const struct ethtool_rxfh_param *rxfh);
+				       const struct ethtool_rxfh_param *rxfh,
+				       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);
 	void	(*get_channels)(struct net_device *, struct ethtool_channels *);
 	int	(*set_channels)(struct net_device *, struct ethtool_channels *);
 	int	(*get_dump_flag)(struct net_device *, struct ethtool_dump *);
net/core/dev.c +1 -1
index c4e880397c07..09d01b49a414 100644
@@ -11205,7 +11205,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
 		xa_erase(&dev->ethtool->rss_ctx, context);
 		if (dev->ethtool_ops->create_rxfh_context)
 			dev->ethtool_ops->remove_rxfh_context(dev, ctx,
-							      context);
+							      context, NULL);
 		else if (dev->ethtool_ops->cap_rss_ctx_supported)
 			dev->ethtool_ops->set_rxfh(dev, &rxfh, NULL);
 		else /* can't happen */
net/ethtool/ioctl.c +6 -3
index 2b75d84c3078..244e565e1365 100644
@@ -1423,12 +1423,15 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 
 	if (rxfh.rss_context && ops->create_rxfh_context) {
 		if (create)
-			ret = ops->create_rxfh_context(dev, ctx, &rxfh_dev);
+			ret = ops->create_rxfh_context(dev, ctx, &rxfh_dev,
+						       extack);
 		else if (rxfh_dev.rss_delete)
 			ret = ops->remove_rxfh_context(dev, ctx,
-						       rxfh.rss_context);
+						       rxfh.rss_context,
+						       extack);
 		else
-			ret = ops->modify_rxfh_context(dev, ctx, &rxfh_dev);
+			ret = ops->modify_rxfh_context(dev, ctx, &rxfh_dev,
+						       extack);
 	} else {
 		ret = ops->set_rxfh(dev, &rxfh_dev, extack);
 	}
#17 lore raw

PATCH v5 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
index f781894a7cb2..73481a69c17d 100644
@@ -1095,10 +1095,13 @@ 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_lock:		Protects entries in @rss_ctx.  May be taken from
+ *			within RTNL.
  * @wol_enabled:	Wake-on-LAN is enabled
  */
 struct ethtool_netdev_state {
 	struct xarray		rss_ctx;
+	struct mutex		rss_lock;
 	unsigned		wol_enabled:1;
 };
 
net/core/dev.c +5 -0
index 09d01b49a414..6679834be301 100644
@@ -10287,6 +10287,7 @@ int register_netdevice(struct net_device *dev)
 
 	/* rss ctx ID 0 is reserved for the default context, start from 1 */
 	xa_init_flags(&dev->ethtool->rss_ctx, XA_FLAGS_ALLOC1);
+	mutex_init(&dev->ethtool->rss_lock);
 
 	spin_lock_init(&dev->addr_list_lock);
 	netdev_set_addr_lockdep_class(dev);
@@ -11192,6 +11193,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
 	struct ethtool_rxfh_context *ctx;
 	unsigned long context;
 
+	mutex_lock(&dev->ethtool->rss_lock);
 	xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
 		struct ethtool_rxfh_param rxfh;
 
@@ -11214,6 +11216,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
 		kfree(ctx);
 	}
 	xa_destroy(&dev->ethtool->rss_ctx);
+	mutex_unlock(&dev->ethtool->rss_lock);
 }
 
 /**
@@ -11326,6 +11329,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
index 244e565e1365..9d2d677770db 100644
@@ -1282,6 +1282,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	struct netlink_ext_ack *extack = NULL;
 	struct ethtool_rxnfc rx_rings;
 	struct ethtool_rxfh rxfh;
+	bool locked = false; /* dev->ethtool->rss_lock taken */
 	u32 indir_bytes = 0;
 	bool create = false;
 	u8 *rss_config;
@@ -1377,6 +1378,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 (rxfh_dev.rss_delete) {
 			ret = -EINVAL;
@@ -1492,6 +1497,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;
 }
#18 lore raw

PATCH v5 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  |   4 +
 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        |   4 +
 drivers/net/ethernet/sfc/ethtool_common.c | 148 ++++++++++++----------
 drivers/net/ethernet/sfc/ethtool_common.h |  12 ++
 drivers/net/ethernet/sfc/mcdi_filters.c   | 135 ++++++++++----------
 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, 208 insertions(+), 219 deletions(-)
drivers/net/ethernet/sfc/ef10.c +1 -1
index 8fa6c0e9195b..7d69302ffa0a 100644
@@ -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 -0
index cf55202b3a7b..896ffca4aee2 100644
@@ -59,8 +59,12 @@ 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,
+	.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
index e9d9de8e648a..6f1a01ded7d4 100644
@@ -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
index 48d3623735ba..7a6cab883d66 100644
@@ -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
index 4ebd5ae23eca..13cf647051af 100644
@@ -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 -0
index 37c69c8d90b1..0f5c68b8bab7 100644
@@ -268,8 +268,12 @@ 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,
+	.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 +82 -66

206 lines not on this page · raw message

drivers/net/ethernet/sfc/ethtool_common.h +12 -0
index a680e5980213..fc52e891637d 100644
@@ -49,6 +49,18 @@ int efx_ethtool_get_rxfh(struct net_device *net_dev,
 int efx_ethtool_set_rxfh(struct net_device *net_dev,
 			 struct ethtool_rxfh_param *rxfh,
 			 struct netlink_ext_ack *extack);
+int efx_ethtool_create_rxfh_context(struct net_device *net_dev,
+				    struct ethtool_rxfh_context *ctx,
+				    const struct ethtool_rxfh_param *rxfh,
+				    struct netlink_ext_ack *extack);
+int efx_ethtool_modify_rxfh_context(struct net_device *net_dev,
+				    struct ethtool_rxfh_context *ctx,
+				    const struct ethtool_rxfh_param *rxfh,
+				    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 +70 -65

355 lines not on this page · raw message

drivers/net/ethernet/sfc/mcdi_filters.h +2 -6
index c0d6558b9fd2..11b9f87ed9e1 100644
@@ -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
index f2dd7feb0e0c..b85c51cbe7f9 100644
@@ -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
index fbd2769307f9..75fa84192362 100644
@@ -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,
maintainer
#19 lore raw
In reply to [email protected]
Quoted 8 lines · On Tue, 18 Jun 2024 23:44:20 +0100 [email protected] wrote:

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;
other drivers (mvpp2, octeontx2, mlx5, sfc/siena) can be converted afterwards and the legacy API removed.

LGTM!

I'll take a stab at bnxt conversion tomorrow. Do you have any tests or should I try to produce some as well?

maintainer
#20 lore raw
Quoted 2 lines · Jakub Kicinski

I'll take a stab at bnxt conversion tomorrow. Do you have any tests or should I try to produce some as well?

I don't have any tests, no; that would be great.

-ed