Skip to contentSkip to conversation

ethtool: track custom RSS contexts in the core

RFC PATCH Superseded [email protected] 4 participants 24 messages Apr 11, 2023 – Jun 9, 2023

Subsystems (7)

v2 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v2 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 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.)

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 +-
 drivers/net/phy/phy.c                     |   2 +-
 drivers/net/phy/phy_device.c              |   4 +-
 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                       | 124 ++++++++++++++++--
 net/ethtool/wol.c                         |   2 +-
 22 files changed, 484 insertions(+), 240 deletions(-)
#1 lore raw

RFC PATCH v2 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.

---
Changes in v2: New patch.

---
 drivers/net/ethernet/realtek/r8169_main.c | 4 ++--
 drivers/net/phy/phy.c                     | 2 +-
 drivers/net/phy/phy_device.c              | 4 ++--
 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 +-
 9 files changed, 24 insertions(+), 11 deletions(-)
drivers/net/ethernet/realtek/r8169_main.c +2 -2
index 9f8357bbc8a4..356f43fac74f 100644
@@ -1451,7 +1451,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;
 	}
 }
 
@@ -5330,7 +5330,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/phy/phy.c +1 -1
index 0c0df38cd1ab..2d8307e9c351 100644
@@ -1245,7 +1245,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 +2 -2
index 917ba84105fc..535002e75dc5 100644
@@ -281,7 +281,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,7 @@ int phy_suspend(struct phy_device *phydev)
 
 	/* If the device has WOL enabled, we cannot suspend the PHY */
 	phy_ethtool_get_wol(phydev, &wol);
-	if (wol.wolopts || (netdev && netdev->wol_enabled))
+	if (wol.wolopts || (netdev && netdev->ethtool->wol_enabled))
 		return -EBUSY;
 
 	if (!phydrv || !phydrv->suspend)
drivers/net/phy/phylink.c +1 -1
index f7da96f0c75b..c332d8950f01 100644
@@ -2005,7 +2005,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 798d35890118..c73b28df301c 100644
@@ -934,6 +934,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 a740be3bb911..1915a6221096 100644
@@ -77,6 +77,7 @@ struct udp_tunnel_nic;
 struct bpf_prog;
 struct xdp_buff;
 struct xdp_md;
+struct ethtool_netdev_state;
 
 void synchronize_net(void);
 void netdev_set_default_ethtool_ops(struct net_device *dev,
@@ -2015,8 +2016,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
@@ -2028,6 +2027,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
@@ -2385,7 +2385,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;
@@ -2397,6 +2396,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 7ce5985be84b..93960861a11f 100644
@@ -10676,6 +10676,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;
@@ -10726,6 +10729,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 59adc4e6e9ee..0effaca4ff9e 100644
@@ -1449,7 +1449,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 a4a43d9e6e9d..820578b70073 100644
@@ -136,7 +136,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;
 }
 
maintainer Reviewed-by on v2 1/7
#2 lore raw

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

Quoted 7 lines · On Tue, Apr 11, 2023 at 07:26:09PM +0100, [email protected] wrote:

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.

Andrew

maintainer Reviewed-by on v2 1/7
#3 lore raw

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

In reply to [email protected]
Quoted 7 lines · On Tue, 11 Apr 2023 19:26:09 +0100 [email protected] wrote:

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.

#4 lore raw

RFC PATCH v2 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.

---
Changes in v2: fix data area to ensure proper alignment (kuba)

---
 include/linux/ethtool.h | 41 +++++++++++++++++++++++++++++++++++++++++
 net/core/dev.c          | 23 +++++++++++++++++++++++
 2 files changed, 64 insertions(+)
include/linux/ethtool.h +41 -0
index c73b28df301c..7963b06da484 100644
@@ -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)
@@ -936,9 +973,13 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
 
 /**
  * struct ethtool_netdev_state - per-netdevice state for ethtool features
+ * @rss_ctx_max_id:	maximum (exclusive) supported RSS context ID
+ * @rss_ctx:		IDR storing custom RSS context state
  * @wol_enabled:	Wake-on-LAN is enabled
  */
 struct ethtool_netdev_state {
+	u32			rss_ctx_max_id;
+	struct idr		rss_ctx;
 	unsigned		wol_enabled:1;
 };
 
net/core/dev.c +23 -0
index 93960861a11f..c9ed9f6ea695 100644
@@ -9983,6 +9983,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);
 
@@ -10781,6 +10784,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
@@ -10885,6 +10906,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
#5 lore raw

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

include/linux/ethtool.h int ethtool_virtdev_set_link_ksettings(struct net_device *dev, · line 984

Quoted 11 lines

/**
* struct ethtool_netdev_state - per-netdevice state for ethtool features
+ * @rss_ctx_max_id: maximum (exclusive) supported RSS context ID
+ * @rss_ctx: IDR storing custom RSS context state
* @wol_enabled: Wake-on-LAN is enabled
*/
struct ethtool_netdev_state {

include/linux/ethtool.h +2 -0
+	u32			rss_ctx_max_id;
+	struct idr		rss_ctx;
 	unsigned		wol_enabled:1;
 };

A nitpick. On 64 bit systems, you have a hole between rss_ctx_max_id and rss_ctx. If you swap those around, and change wol_enabled to also be a u32 bitfield, the compiler can probably do without the hole.

Andrew

maintainer
#6 lore raw

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

Quoted 7 lines · Andrew Lunn
Quoted 7 lines

/**
* struct ethtool_netdev_state - per-netdevice state for ethtool features
+ * @rss_ctx_max_id: maximum (exclusive) supported RSS context ID
+ * @rss_ctx: IDR storing custom RSS context state
* @wol_enabled: Wake-on-LAN is enabled
*/
struct ethtool_netdev_state {

Quoted 8 lines

include/linux/ethtool.h int ethtool_virtdev_set_link_ksettings(struct net_device *dev, · line 984

Quoted 4 lines
include/linux/ethtool.h +2 -0
+	u32			rss_ctx_max_id;
+	struct idr		rss_ctx;
 	unsigned		wol_enabled:1;
 };

A nitpick. On 64 bit systems, you have a hole between rss_ctx_max_id and rss_ctx. If you swap those around, and change wol_enabled to also be a u32 bitfield, the compiler can probably do without the hole.

Sure, makes sense.

maintainer
#7 lore raw

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

In reply to [email protected]
Quoted 7 lines · On Tue, 11 Apr 2023 19:26:10 +0100 [email protected] wrote:
net/core/dev.c +7 -0
+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;

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

Quoted 10 lines
net/core/dev.c +10 -0
+	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);
+	}
+}

nit: maybe move the ethtool related code out to a new net/ethtool/netdev.c ? We can probably put forward declarations in net/core/dev.h or a new header among sources?

#8 lore raw

RFC PATCH v2 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.

---
Changes in v2:
* change .get_rxfh_priv_size op into rxfh_priv_size value member (kuba)
* use GFP_KERNEL_ACCOUNT rather than GFP_USER (kuba)
* adjust size calculation to allow for alignment padding from patch #2

---
 include/linux/ethtool.h | 14 +++++++++
 net/ethtool/ioctl.c     | 63 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 76 insertions(+), 1 deletion(-)
include/linux/ethtool.h +14 -0
index 7963b06da484..710d6a985347 100644
@@ -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.
@@ -823,6 +836,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 +62 -1

93 lines not on this page · raw message

maintainer
#9 lore raw

Re: [RFC PATCH v2 net-next 3/7] net: ethtool: record custom RSS contexts in the IDR

net/ethtool/ioctl.c static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, · line 1379

Quoted 7 lines · On Tue, 11 Apr 2023 19:26:11 +0100 [email protected] wrote:
net/ethtool/ioctl.c +0 -0
 	if (rxfh.rss_context)
 		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
 					    &rxfh.rss_context, delete);
@@ -1350,6 +1377,40 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		else if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
 			dev->priv_flags |= IFF_RXFH_CONFIGURED;
 	}

This is probably transient but I think we're potentially leaking @ctx in a goto out hiding inside the context here, and...

net/ethtool/ioctl.c static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, · line 1389

Quoted 10 lines
net/ethtool/ioctl.c +10 -0
+	/* 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 (WARN_ON(idr_find(&dev->ethtool->rss_ctx, rxfh.rss_context)))
+			/* context ID reused, our tracking is screwed */
+			goto out;

here.

maintainer
#10 lore raw

Re: [RFC PATCH v2 net-next 3/7] net: ethtool: record custom RSS contexts in the IDR

Quoted 12 lines · Jakub Kicinski

net/ethtool/ioctl.c static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, · line 1379

Quoted 7 lines · On Tue, 11 Apr 2023 19:26:11 +0100 [email protected] wrote:
net/ethtool/ioctl.c +0 -0
 	if (rxfh.rss_context)
 		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
 					    &rxfh.rss_context, delete);
@@ -1350,6 +1377,40 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		else if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
 			dev->priv_flags |= IFF_RXFH_CONFIGURED;
 	}

This is probably transient but I think we're potentially leaking @ctx in a goto out hiding inside the context here, and...

Quoted 12 lines

net/ethtool/ioctl.c static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, · line 1389

Quoted 10 lines
net/ethtool/ioctl.c +10 -0
+	/* 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 (WARN_ON(idr_find(&dev->ethtool->rss_ctx, rxfh.rss_context)))
+			/* context ID reused, our tracking is screwed */
+			goto out;

here.

Wasn't entirely transient. Fixed for v3.

#11 lore raw

RFC PATCH v2 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.

---
Changes in v2:
* split the new API into create/modify/remove ops (kuba). Also means we don't need to rename the old API and touch legacy drivers
* squash patch "net: ethtool: pass ctx_priv and create into .set_rxfh_context"

---
 include/linux/ethtool.h | 40 ++++++++++++++++++++++--
 net/core/dev.c          | 11 +++++--
 net/ethtool/ioctl.c     | 67 ++++++++++++++++++++++++++++++++---------
 3 files changed, 97 insertions(+), 21 deletions(-)
include/linux/ethtool.h +37 -3
index 710d6a985347..12ed3b79be68 100644
@@ -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.
@@ -900,6 +923,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
index c9ed9f6ea695..4feb58b0beb3 100644
@@ -10789,15 +10789,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 +52 -15

100 lines not on this page · raw message

maintainer Reviewed-by on v2 4/7
#12 lore raw

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

Quoted 11 lines · On Tue, 11 Apr 2023 19:26:12 +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.

#13 lore raw

RFC PATCH v2 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.

---
Changes in v2: New patch.

---
 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
index 12ed3b79be68..724da9234cf1 100644
@@ -926,14 +926,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
index 4feb58b0beb3..44668386f376 100644
@@ -10798,7 +10798,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
index 20154d6159a1..abd1cf50e681 100644
@@ -1384,14 +1384,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,
#14 lore raw

RFC PATCH v2 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 724da9234cf1..e8e88d5900d3 100644
@@ -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_max_id:	maximum (exclusive) supported RSS context ID
  * @rss_ctx:		IDR storing custom RSS context state
+ * @rss_lock:		Protects entries in @rss_ctx.  May be taken from
+ *			within RTNL.
  * @wol_enabled:	Wake-on-LAN is enabled
  */
 struct ethtool_netdev_state {
 	u32			rss_ctx_max_id;
 	struct idr		rss_ctx;
+	struct mutex		rss_lock;
 	unsigned		wol_enabled:1;
 };
 
net/core/dev.c +5 -0
index 44668386f376..60c844b372e3 100644
@@ -9987,6 +9987,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);
@@ -10792,6 +10793,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);
@@ -10806,6 +10808,7 @@ static void netdev_rss_contexts_free(struct net_device *dev)
 							   &context, true);
 		kfree(ctx);
 	}
+	mutex_unlock(&dev->ethtool->rss_lock);
 }
 
 /**
@@ -10919,6 +10922,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 abd1cf50e681..8b2e90ba03a1 100644
@@ -1257,6 +1257,7 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	u8 *rss_config;
 	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
 	bool create = false, delete = false;
+	bool locked = false; /* dev->ethtool->rss_lock taken */
 
 	if (!ops->get_rxnfc || !ops->set_rxfh)
 		return -EOPNOTSUPP;
@@ -1334,6 +1335,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;
@@ -1453,6 +1458,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;
 }
maintainer
#15 lore raw

Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

Quoted 6 lines · On Tue, Apr 11, 2023 at 07:26:14PM +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 access dev->ethtool->rss_ctx; taking dev->ethtool->rss_lock allows them to do this safely without needing to take the RTNL.

What is actually wrong with taking RTNL? KISS is often best, especially for locks.

Andrew

maintainer
#16 lore raw

Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

Quoted 8 lines · Andrew Lunn
Quoted 4 lines · On Tue, Apr 11, 2023 at 07:26:14PM +0100, [email protected] wrote:

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.

What is actually wrong with taking RTNL? KISS is often best, especially for locks.

The examples I have of driver code that needs to access rss_ctx (in the
sfc driver) are deep inside call chains where RTNL may or may not already be held.
1) filter insertion. E.g. ethtool -U is already holding RTNL, but other sources of filters (e.g. aRFS) aren't, and thus taking it if necessary might mean passing a 'bool rtnl_locked' all the way down the chain.
2) device reset handling (we have to restore the RSS contexts in the hardware after a reset). Again resets don't always happen under RTNL, and I don't fully understand the details (EFX_ASSERT_RESET_SERIALISED()).
So it makes life much simpler if we just have a finer-grained lock we can
just take when we need to.
Also, RTNL is a very big hammer that serialises all kinds of operations
across all the netdevs in the system, holding it for any length of time can cause annoying user-visible latency (e.g. iirc sshd accepting a new connection has to wait for it) so I prefer to avoid it if possible. If anything we want to be breaking up this BKL[1], not making it bigger.

-ed

[1]: https://legacy.netdevconf.info/2.2/slides/westphal-rtnlmutex-talk.pdf

maintainer
#17 lore raw

Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

Quoted 10 lines · Edward Cree
Quoted 8 lines · Andrew Lunn
Quoted 4 lines · On Tue, Apr 11, 2023 at 07:26:14PM +0100, [email protected] wrote:

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.

What is actually wrong with taking RTNL? KISS is often best, especially for locks.

Quoted 16 lines

The examples I have of driver code that needs to access rss_ctx (in the
sfc driver) are deep inside call chains where RTNL may or may not already be held.
1) filter insertion. E.g. ethtool -U is already holding RTNL, but other sources of filters (e.g. aRFS) aren't, and thus taking it if necessary might mean passing a 'bool rtnl_locked' all the way down the chain.
2) device reset handling (we have to restore the RSS contexts in the hardware after a reset). Again resets don't always happen under RTNL, and I don't fully understand the details (EFX_ASSERT_RESET_SERIALISED()).
So it makes life much simpler if we just have a finer-grained lock we can
just take when we need to.
Also, RTNL is a very big hammer that serialises all kinds of operations
across all the netdevs in the system, holding it for any length of time can cause annoying user-visible latency (e.g. iirc sshd accepting a new connection has to wait for it) so I prefer to avoid it if possible. If anything we want to be breaking up this BKL[1], not making it bigger.

Hi Ed

I have to wonder if your locking model is wrong. When i look at the next patch, i see the driver is also using this lock. And i generally find that is wrong.

As a rule of thumb, driver writes don't understand locking. Yes, there are some that do, but most don't. As core code developers, i find it good practice to have the locks in the core, and only in the core. Drivers writers should not need to worry about locking. The API into the driver will take the locks needed before entering the driver, and release them on exit.

So i don't really agree with 'it makes life much simpler if we just have a finer-grained lock'. It makes life more complex having to help driver writers find the corruption and deadlock bugs in their code because they got the locking wrong.

Please try to work on the abstraction so that drivers don't need this lock, just the core.

Andrew

maintainer
#18 lore raw

Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

Quoted 3 lines · Andrew Lunn

I have to wonder if your locking model is wrong. When i look at the next patch, i see the driver is also using this lock. And i generally find that is wrong.

...

Quoted 3 lines

Drivers writers should not need to worry about locking. The API into the driver will take the locks needed before entering the driver, and release them on exit.

I don't think that's possible without increasing driver complexity in
other ways. Essentially, for the driver to take advantage of the core tracking these contexts, and thus not need its own data structures to track them as well (like the efx->rss_context.list we remove in patch #7), it has to be able to access them on driver-initiated (not just core-initiated) events. (The central example of this being "oh, the NIC MCPU just rebooted, we have to reinstall all our filters".) And it needs to be able to exclude writes while it does that, not only for consistency but also because e.g. context deletion will free that memory (though I guess we could finesse that part with RCU?).
What I *could* do is add suitable wrapper functions for access to
dev->ethtool->rss_ctx (e.g. a core equivalent of efx_find_rss_context_entry() that wraps the idr_find()) and have them assert that the lock is held (like efx_find_rss_context_entry() does); that would at least validate the driver locking somewhat.
But having those helper functions perform the locking themselves would
mean going to a get/put model for managing the lifetime of the driver's reference (with a separate get_write for exclusive access), at which point it's probably harder for driver writers to understand than "any time you're touching rss_ctx you need to hold the rss_lock".

-ed

maintainer
#19 lore raw

Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

Quoted 20 lines · On Wed, 12 Apr 2023 18:42:16 +0100 Edward Cree wrote:
Quoted 3 lines · Andrew Lunn

I have to wonder if your locking model is wrong. When i look at the next patch, i see the driver is also using this lock. And i generally find that is wrong.

...

Quoted 3 lines

Drivers writers should not need to worry about locking. The API into the driver will take the locks needed before entering the driver, and release them on exit.

I don't think that's possible without increasing driver complexity in
other ways. Essentially, for the driver to take advantage of the core tracking these contexts, and thus not need its own data structures to track them as well (like the efx->rss_context.list we remove in patch #7), it has to be able to access them on driver-initiated (not just core-initiated) events. (The central example of this being "oh, the NIC MCPU just rebooted, we have to reinstall all our filters".) And it needs to be able to exclude writes while it does that, not only for consistency but also because e.g. context deletion will free that memory (though I guess we could finesse that part with RCU?).
What I *could* do is add suitable wrapper functions for access to
dev->ethtool->rss_ctx (e.g. a core equivalent of

Quoted 8 lines

efx_find_rss_context_entry() that wraps the idr_find()) and have them assert that the lock is held (like efx_find_rss_context_entry() does); that would at least validate the driver locking somewhat.
But having those helper functions perform the locking themselves would
mean going to a get/put model for managing the lifetime of the driver's reference (with a separate get_write for exclusive access), at which point it's probably harder for driver writers to understand than "any time you're touching rss_ctx you need to hold the rss_lock".

IMO the "MCPU has rebooted" case is a control path, taking rtnl seems like the right thing to do. Does that happen often enough or takes so long to recover that we need to be concerned about locking down rtnl? aRFS can't sleep IIUC so the mutex is does not seem like a great match.

IOW I had the same reaction as Andrew first time I saw this mutex :(

maintainer
#20 lore raw

Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

Quoted 3 lines · Jakub Kicinski

IMO the "MCPU has rebooted" case is a control path, taking rtnl seems like the right thing to do. Does that happen often enough or takes so long to recover that we need to be concerned about locking down rtnl?

Normally we *do* hold RTNL across the reset handling path, and all the
callers I can find take it. But the existence of a more complicated condition guarding the ASSERT_RTNL() in EFX_ASSERT_RESET_SERIALISED() implies that there is, or at least was, a call site that doesn't; that makes me nervous about assuming it.

Quoted 1 line

aRFS can't sleep IIUC so the mutex is does not seem like a great match.

sfc punts aRFS filter insertion into a workitem, because you can't do
MCDI without sleeping. And aRFS was just one example; there's lots of other sources of filters in the driver (PTP, sync_rx_mode (device UC/MC addresses), VLAN filtering (NETIF_F_HW_VLAN_CTAG_FILTER)...). Some of those filters can also have EFX_FILTER_FLAG_RX_RSS (though not custom contexts).

So while I *think* the filter insert code could carefully go
if (spec->flags & EFX_FILTER_FLAG_RX_RSS && spec->rss_context)

       ASSERT_RTNL();

it's kinda hairy. And if this is a *normal* level of hair for drivers to have in this area, then the "but driver writers don't understand locking!" argument doesn't really favour one solution over the other. After all, the driver will still have to hold *some* lock to access this stuff, whether it's rss_lock or RTNL.
(Idk, maybe sfc is just uniquely complex and messy. It wouldn't be
the first time.)

Quoted 1 line

IOW I had the same reaction as Andrew first time I saw this mutex :(

Well I seem to be outvoted, so I'll have another crack at getting it
to work with just RTNL (that's what I went for originally, actually, but one of the ASSERT_RTNL()s failed in test and I couldn't figure out why at the time. Possibly trying to argue the case has helped me to understand more of the details!).

maintainer
#21 lore raw

Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

Quoted 2 lines

(Idk, maybe sfc is just uniquely complex and messy. It wouldn't be
the first time.)

Hi Ed

Have you looked at other drivers? It would be bad to design an API around a messy driver. Maybe this is an opportunity to learn from other drivers and come up with something cleaner for SFC?

Andrew

maintainer
#22 lore raw

Re: [RFC PATCH v2 net-next 6/7] net: ethtool: add a mutex protecting RSS contexts

Quoted 7 lines · Andrew Lunn
Quoted 2 lines

(Idk, maybe sfc is just uniquely complex and messy. It wouldn't be
the first time.)

Hi Ed

Have you looked at other drivers? It would be bad to design an API around a messy driver.

I have; there's really not many that implement custom RSS contexts
(just Marvell's mvpp2 and octeontx2, and Mellanox's mlx5). The rss_ctx_max_id field is designed for those as they all have fixed- size arrays currently and idk whether that's a purely software limit or whether it reflects the hardware.
I couldn't find anything in any of them that looked like "restore
stuff after a device reboot"; maybe it's just not something those devices expect to experience normally.

I don't know enough about mlx5 hw to really understand their filter
code, but the rough equivalent of our efx_mcdi_filter_insert_locked() in that driver appears to be _mlx5_add_flow_rules(), which seems to be doing some kind of hand-over-hand locking. And no sign (whether in comments or in asserts) of whether the function expects callers to hold RTNL. Same goes for their functions operating on TIRs (whatever those are) which are called from all over (aRFS, tc, even kTLS!) in addition to the ethtool RSS/ntuple paths.

Anyway I'll cc maintainers of those drivers on v3 so they can chime
in on the API design. (Should've done that on v1 really, but I forgot. Mea culpa.)

-ed

#23 lore raw

RFC PATCH v2 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.

---
Changes in v2:
* actually hook up rxfh_priv_size in ethtool ops structs
* port to the updated API

---
 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
index d30459dbfe8f..6f12fcee8247 100644
@@ -1394,7 +1394,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
index 702abbe59b76..c5f82eb0e5b4 100644
@@ -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
index 746fd9164e30..1b2c281c1cc1 100644
@@ -298,7 +298,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 4239c7ece123..a077f648bbde 100644
@@ -160,7 +160,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 cc30524c2fe4..23e3778716b2 100644
@@ -717,7 +717,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);
 }
 
@@ -780,7 +780,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);
 	if (efx->type->sriov_reset)
@@ -798,7 +798,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);
 
@@ -1005,9 +1005,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
index 364323599f7b..f5fb7464e025 100644
@@ -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
index 659491932101..3df852eaab20 100644
@@ -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
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 fcd51d3992fa..bceeada24d6c 100644
@@ -770,21 +770,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];
 };
@@ -917,9 +920,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
@@ -1090,7 +1091,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;
@@ -1462,9 +1462,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,