[RFC,net-next,1/6] net: ethtool: attach an IDR of custom RSS contexts to a netdevice

Commit Message

[e-mail] April 3, 2023, 4:32 p.m. UTC

From: Edward Cree <[e-mail]>

Each context stores the RXFH settings (indir, key, and hfunc) as well
 as optionally some driver private data.
Delete any still-existing contexts at netdev unregister time.

Signed-off-by: Edward Cree <[e-mail]>
---
 include/linux/ethtool.h   | 33 +++++++++++++++++++++++++++++++++
 include/linux/netdevice.h |  5 +++++
 net/core/dev.c            | 23 +++++++++++++++++++++++
 3 files changed, 61 insertions(+)

Comments

Jakub Kicinski April 3, 2023, 9:43 p.m. UTC | #1

On Mon, 3 Apr 2023 17:32:58 +0100 [e-mail] wrote:
> From: Edward Cree <[e-mail]>
> 
> 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.

> +/**
> + * 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
> + */
> +struct ethtool_rxfh_context {
> +	u32 indir_size;
> +	u32 key_size;
> +	u8 hfunc;
> +	u16 priv_size;
> +	/* private: indirection table, hash key, and driver private data are
> +	 * stored sequentially in @data area.  Use below helpers to access
> +	 */
> +	u8 data[];

I think that something needs to get aligned here...
Driver priv needs to guarantee ulong alignment in case someone puts 
a pointer in it.

> +};
> +
> +static inline u32 *ethtool_rxfh_context_indir(struct ethtool_rxfh_context *ctx)
> +{
> +	return (u32 *)&ctx->data;
> +}
> +
> +static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
> +{
> +	return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size);
> +}
> +
> +static inline void *ethtool_rxfh_context_priv(struct ethtool_rxfh_context *ctx)
> +{
> +	return ethtool_rxfh_context_key(ctx) + ctx->key_size;

ALIGN_PTR() ... ?
Or align data[] and reorder..

> +}
> +
>  /* declare a link mode bitmap */
>  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
>  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)

> +	u32			rss_ctx_max_id;
> +	struct idr		rss_ctx;

noob question, why not xarray?
Isn't IDR just a legacy wrapper around xarray anyway?

Edward Cree April 4, 2023, 11:30 a.m. UTC | #2

On 03/04/2023 22:43, Jakub Kicinski wrote:
> On Mon, 3 Apr 2023 17:32:58 +0100 [e-mail] wrote:
>> +	/* private: indirection table, hash key, and driver private data are
>> +	 * stored sequentially in @data area.  Use below helpers to access
>> +	 */
>> +	u8 data[];
> 
> I think that something needs to get aligned here...
> Driver priv needs to guarantee ulong alignment in case someone puts 
> a pointer in it.
> 
>> +static inline void *ethtool_rxfh_context_priv(struct ethtool_rxfh_context *ctx)
>> +{
>> +	return ethtool_rxfh_context_key(ctx) + ctx->key_size;
> 
> ALIGN_PTR() ... ?
> Or align data[] and reorder..

Very good points.  Indir also needs 4-byte alignment.
Will fix in next version.

>> +	u32			rss_ctx_max_id;
>> +	struct idr		rss_ctx;
> 
> noob question, why not xarray?

Because I know how to use the IDR API, but have never used
 xarray directly, so would need to learn it.

> Isn't IDR just a legacy wrapper around xarray anyway?

I see it as a *convenience* wrapper.  Is it deprecated?

Jakub Kicinski April 4, 2023, 11:36 p.m. UTC | #3

On Tue, 4 Apr 2023 12:30:42 +0100 Edward Cree wrote:
> > Isn't IDR just a legacy wrapper around xarray anyway?  
> 
> I see it as a *convenience* wrapper.  Is it deprecated?

Hm, I'm not so sure what the relation between idr and xarray
is after glancing at the code.
I'm more used to seeing xarray these days but if you prefer 
to keep the IDR it's fine.

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 798d35890118..1898f4446666 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -157,6 +157,39 @@  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
+ */
+struct ethtool_rxfh_context {
+	u32 indir_size;
+	u32 key_size;
+	u8 hfunc;
+	u16 priv_size;
+	/* private: indirection table, hash key, and driver private data are
+	 * stored sequentially in @data area.  Use below helpers to access
+	 */
+	u8 data[];
+};
+
+static inline u32 *ethtool_rxfh_context_indir(struct ethtool_rxfh_context *ctx)
+{
+	return (u32 *)&ctx->data;
+}
+
+static inline u8 *ethtool_rxfh_context_key(struct ethtool_rxfh_context *ctx)
+{
+	return (u8 *)(ethtool_rxfh_context_indir(ctx) + ctx->indir_size);
+}
+
+static inline void *ethtool_rxfh_context_priv(struct ethtool_rxfh_context *ctx)
+{
+	return ethtool_rxfh_context_key(ctx) + ctx->key_size;
+}
+
 /* declare a link mode bitmap */
 #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
 	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a740be3bb911..91f7dad070bd 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2028,6 +2028,8 @@  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
+ *	@rss_ctx_max_id:	maximum (exclusive) supported RSS context ID
+ *	@rss_ctx:	IDR storing custom RSS context state
  *	@xdp_state:		stores info on attached XDP BPF programs
  *
  *	@nested_level:	Used as a parameter of spin_lock_nested() of
@@ -2397,6 +2399,9 @@  struct net_device {
 	const struct udp_tunnel_nic_info	*udp_tunnel_nic_info;
 	struct udp_tunnel_nic	*udp_tunnel_nic;
 
+	u32			rss_ctx_max_id;
+	struct idr		rss_ctx;
+
 	/* protected by rtnl_lock */
 	struct bpf_xdp_entity	xdp_state[__MAX_XDP_MODE];
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 7ce5985be84b..d0a936d4e532 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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->rss_ctx, 1);
+
 	spin_lock_init(&dev->addr_list_lock);
 	netdev_set_addr_lockdep_class(dev);
 
@@ -10777,6 +10780,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->rss_ctx, ctx, context) {
+		u32 *indir = ethtool_rxfh_context_indir(ctx);
+		u8 *key = ethtool_rxfh_context_key(ctx);
+
+		idr_remove(&dev->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
@@ -10881,6 +10902,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)