diff mbox series

[RFC,net-next,2/6] net: ethtool: record custom RSS contexts in the IDR

Message ID 57c0a5a7d41e1341e8a7b0256ca8ed6f3e3ea9c0.1680538846.git.ecree.xilinx@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ethtool: track custom RSS contexts in the core | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1896 this patch: 1896
netdev/cc_maintainers warning 3 maintainers not CCed: vladimir.oltean@nxp.com d-tatianin@yandex-team.ru andrew@lunn.ch
netdev/build_clang success Errors and warnings before: 567 this patch: 567
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2005 this patch: 2005
netdev/checkpatch warning WARNING: function definition argument 'struct net_device *' should also have an identifier name WARNING: line length of 83 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

edward.cree@amd.com April 3, 2023, 4:32 p.m. UTC
From: Edward Cree <ecree.xilinx@gmail.com>

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.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 include/linux/ethtool.h | 17 +++++++++++
 net/ethtool/ioctl.c     | 67 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 82 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski April 3, 2023, 9:48 p.m. UTC | #1
On Mon, 3 Apr 2023 17:32:59 +0100 edward.cree@amd.com wrote:
> @@ -880,6 +896,7 @@ struct ethtool_ops {
>  			    u8 *hfunc);
>  	int	(*set_rxfh)(struct net_device *, const u32 *indir,
>  			    const u8 *key, const u8 hfunc);
> +	u16	(*get_rxfh_priv_size)(struct net_device *);
>  	int	(*get_rxfh_context)(struct net_device *, u32 *indir, u8 *key,
>  				    u8 *hfunc, u32 rss_context);
>  	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,

Would a static value not do for most drivers?
We already have a handful of data fields in the "ops" structure.

> @@ -1331,6 +1335,31 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  	}
>  
> +	if (create) {
> +		if (delete) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		ctx = kzalloc(ethtool_rxfh_context_size(dev_indir_size,
> +							dev_key_size,
> +							dev_priv_size),
> +			      GFP_USER);

GFP_USER? Do you mean it for accounting? GFP_KERNEL_ACCOUNT?
Edward Cree April 4, 2023, 11:49 a.m. UTC | #2
On 03/04/2023 22:48, Jakub Kicinski wrote:
> On Mon, 3 Apr 2023 17:32:59 +0100 edward.cree@amd.com wrote:
>> @@ -880,6 +896,7 @@ struct ethtool_ops {
>>  			    u8 *hfunc);
>>  	int	(*set_rxfh)(struct net_device *, const u32 *indir,
>>  			    const u8 *key, const u8 hfunc);
>> +	u16	(*get_rxfh_priv_size)(struct net_device *);
>>  	int	(*get_rxfh_context)(struct net_device *, u32 *indir, u8 *key,
>>  				    u8 *hfunc, u32 rss_context);
>>  	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
> 
> Would a static value not do for most drivers?

Yes, it would.

> We already have a handful of data fields in the "ops" structure.

I didn't notice that / realise it was an option.  Will do.

>> @@ -1331,6 +1335,31 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>>  		}
>>  	}
>>  
>> +	if (create) {
>> +		if (delete) {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +		ctx = kzalloc(ethtool_rxfh_context_size(dev_indir_size,
>> +							dev_key_size,
>> +							dev_priv_size),
>> +			      GFP_USER);
> 
> GFP_USER? Do you mean it for accounting? GFP_KERNEL_ACCOUNT?

It's an allocation triggerable by userland; I was under the
 impression that those were supposed to use GFP_USER for some
 reason; the rss_config alloc further up this function does.
Jakub Kicinski April 4, 2023, 11:40 p.m. UTC | #3
On Tue, 4 Apr 2023 12:49:18 +0100 Edward Cree wrote:
> > GFP_USER? Do you mean it for accounting? GFP_KERNEL_ACCOUNT?  
> 
> It's an allocation triggerable by userland; I was under the
>  impression that those were supposed to use GFP_USER for some
>  reason; the rss_config alloc further up this function does.

That's what I thought, too, and that it implies memory accounting.
But then someone from MM told me that that's not the case, and
that GFP_USER is supposed to be mmap()able. Or maybe the latter
part I got from the kdoc in gfp_types.h.

I think we should make sure the memory is accounted.
Edward Cree April 5, 2023, 9:34 a.m. UTC | #4
On 05/04/2023 00:40, Jakub Kicinski wrote:
> On Tue, 4 Apr 2023 12:49:18 +0100 Edward Cree wrote:
>>> GFP_USER? Do you mean it for accounting? GFP_KERNEL_ACCOUNT?  
>>
>> It's an allocation triggerable by userland; I was under the
>>  impression that those were supposed to use GFP_USER for some
>>  reason; the rss_config alloc further up this function does.
> 
> That's what I thought, too, and that it implies memory accounting.
> But then someone from MM told me that that's not the case, and
> that GFP_USER is supposed to be mmap()able. Or maybe the latter
> part I got from the kdoc in gfp_types.h.
> 
> I think we should make sure the memory is accounted.

Okay.  Presumably this doesn't apply to `rss_config` because it's
 short-lived (freed on the way out of the function)?
(In which case I think `rss_config` should just be GFP_KERNEL;
 we don't try to map it to userspace, we just copy_{from,to}_user
 between it and the ioctl data.  But that's unrelated cleanup
 which we can worry about later, if at all.)
diff mbox series

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 1898f4446666..a16580a8e9d7 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -163,12 +163,16 @@  static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings)
  * @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: indirection table, hash key, and driver private data are
 	 * stored sequentially in @data area.  Use below helpers to access
 	 */
@@ -190,6 +194,16 @@  static inline void *ethtool_rxfh_context_priv(struct ethtool_rxfh_context *ctx)
 	return ethtool_rxfh_context_key(ctx) + ctx->key_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), priv_size);
+	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)
@@ -727,6 +741,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.
+ * @get_rxfh_priv_size: Get the 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.
@@ -880,6 +896,7 @@  struct ethtool_ops {
 			    u8 *hfunc);
 	int	(*set_rxfh)(struct net_device *, const u32 *indir,
 			    const u8 *key, const u8 hfunc);
+	u16	(*get_rxfh_priv_size)(struct net_device *);
 	int	(*get_rxfh_context)(struct net_device *, u32 *indir, u8 *key,
 				    u8 *hfunc, u32 rss_context);
 	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 59adc4e6e9ee..c8f11ac343c9 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1248,14 +1248,15 @@  static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 {
 	int ret;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_rxfh_context *ctx;
 	struct ethtool_rxnfc rx_rings;
 	struct ethtool_rxfh rxfh;
-	u32 dev_indir_size = 0, dev_key_size = 0, i;
+	u32 dev_indir_size = 0, dev_key_size = 0, dev_priv_size = 0, i;
 	u32 *indir = NULL, indir_bytes = 0;
 	u8 *hkey = NULL;
 	u8 *rss_config;
 	u32 rss_cfg_offset = offsetof(struct ethtool_rxfh, rss_config[0]);
-	bool delete = false;
+	bool create = false, delete = false;
 
 	if (!ops->get_rxnfc || !ops->set_rxfh)
 		return -EOPNOTSUPP;
@@ -1274,6 +1275,9 @@  static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	/* Most drivers don't handle rss_context, check it's 0 as well */
 	if (rxfh.rss_context && !ops->set_rxfh_context)
 		return -EOPNOTSUPP;
+	create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
+	if (create && ops->get_rxfh_priv_size)
+		dev_priv_size = ops->get_rxfh_priv_size(dev);
 
 	/* If either indir, hash key or function is valid, proceed further.
 	 * Must request at least one change: indir size, hash key or function.
@@ -1331,6 +1335,31 @@  static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		}
 	}
 
+	if (create) {
+		if (delete) {
+			ret = -EINVAL;
+			goto out;
+		}
+		ctx = kzalloc(ethtool_rxfh_context_size(dev_indir_size,
+							dev_key_size,
+							dev_priv_size),
+			      GFP_USER);
+		if (!ctx) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		ctx->indir_size = dev_indir_size;
+		ctx->key_size = dev_key_size;
+		ctx->hfunc = rxfh.hfunc;
+		ctx->priv_size = dev_priv_size;
+	} else if (rxfh.rss_context) {
+		ctx = idr_find(&dev->rss_ctx, rxfh.rss_context);
+		if (!ctx) {
+			ret = -ENOENT;
+			goto out;
+		}
+	}
+
 	if (rxfh.rss_context)
 		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
 					    &rxfh.rss_context, delete);
@@ -1350,6 +1379,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;
 	}
+	/* 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->rss_ctx, rxfh.rss_context)))
+			/* context ID reused, our tracking is screwed */
+			goto out;
+		/* Allocate the exact ID the driver gave us */
+		WARN_ON(idr_alloc(&dev->rss_ctx, ctx, rxfh.rss_context,
+				  rxfh.rss_context + 1, GFP_KERNEL) !=
+			rxfh.rss_context);
+		ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
+		ctx->key_no_change = !rxfh.key_size;
+	}
+	if (delete) {
+		WARN_ON(idr_remove(&dev->rss_ctx, rxfh.rss_context) != ctx);
+		kfree(ctx);
+	} else if (ctx) {
+		if (indir) {
+			for (i = 0; i < dev_indir_size; i++)
+				ethtool_rxfh_context_indir(ctx)[i] = indir[i];
+			ctx->indir_no_change = 0;
+		}
+		if (hkey) {
+			memcpy(ethtool_rxfh_context_key(ctx), hkey,
+			       dev_key_size);
+			ctx->key_no_change = 0;
+		}
+		if (rxfh.hfunc != ETH_RSS_HASH_NO_CHANGE)
+			ctx->hfunc = rxfh.hfunc;
+	}
 
 out:
 	kfree(rss_config);