diff mbox series

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

Message ID 7552f2ab4cf66232baf03d3bc3a47fc1341761f9.1718750587.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/ynl success Generated files up to date; no warnings/errors; no diff in generated;
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: 853 this patch: 853
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 914 this patch: 914
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: 2659 this patch: 2659
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/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 88 this patch: 88
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-06-19--03-00 (tests: 656)

Commit Message

edward.cree@amd.com June 18, 2024, 10:44 p.m. UTC 
From: Edward Cree <ecree.xilinx@gmail.com>

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

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 include/linux/ethtool.h | 37 +++++++++++++++++++++++++++++++++
 net/core/dev.c          |  5 ++++-
 net/ethtool/ioctl.c     | 46 ++++++++++++++++++++++++++++++-----------
 3 files changed, 75 insertions(+), 13 deletions(-)

Comments

Jakub Kicinski June 19, 2024, 5:24 p.m. UTC | #1 
On Tue, 18 Jun 2024 23:44:24 +0100 edward.cree@amd.com 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?

> + *	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);

Can we make remove void? It's sort of a cleanup, cleanups which can
fail make life hard.
Jakub Kicinski June 19, 2024, 8:08 p.m. UTC | #2 
On Wed, 19 Jun 2024 10:24:35 -0700 Jakub Kicinski wrote:
> On Tue, 18 Jun 2024 23:44:24 +0100 edward.cree@amd.com 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?

I was thinking about the key, hfunc and xfrm. Indirection table
needs to get reported.
Edward Cree June 20, 2024, 4:42 a.m. UTC | #3 
On 19/06/2024 18:24, Jakub Kicinski wrote:
> On Tue, 18 Jun 2024 23:44:24 +0100 edward.cree@amd.com 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?

Yes, that was what I had in mind.

> Indirection table needs to get reported.

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

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

>> +	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
diff mbox series

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 5bef46fdcb94..4fec3c2876aa 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -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 *);
diff --git a/net/core/dev.c b/net/core/dev.c
index cc85baa3624b..c4e880397c07 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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",
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index f879deb6ac4e..2b75d84c3078 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1392,9 +1392,24 @@  static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		}
 		ctx->indir_size = dev_indir_size;
 		ctx->key_size = dev_key_size;
-		ctx->hfunc = rxfh.hfunc;
-		ctx->input_xfrm = rxfh.input_xfrm;
 		ctx->priv_size = ops->rxfh_priv_size;
+		/* Initialise to an empty context */
+		ctx->hfunc = ETH_RSS_HASH_NO_CHANGE;
+		ctx->input_xfrm = RXH_XFRM_NO_CHANGE;
+		if (ops->create_rxfh_context) {
+			u32 limit = ops->rxfh_max_context_id ?: U32_MAX;
+			u32 ctx_id;
+
+			/* driver uses new API, core allocates ID */
+			ret = xa_alloc(&dev->ethtool->rss_ctx, &ctx_id, ctx,
+				       XA_LIMIT(1, limit), GFP_KERNEL_ACCOUNT);
+			if (ret < 0) {
+				kfree(ctx);
+				goto out;
+			}
+			WARN_ON(!ctx_id); /* can't happen */
+			rxfh.rss_context = ctx_id;
+		}
 	} else if (rxfh.rss_context) {
 		ctx = xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context);
 		if (!ctx) {
@@ -1406,11 +1421,24 @@  static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	rxfh_dev.rss_context = rxfh.rss_context;
 	rxfh_dev.input_xfrm = rxfh.input_xfrm;
 
-	ret = ops->set_rxfh(dev, &rxfh_dev, extack);
-	if (ret) {
+	if (rxfh.rss_context && ops->create_rxfh_context) {
 		if (create)
+			ret = ops->create_rxfh_context(dev, ctx, &rxfh_dev);
+		else if (rxfh_dev.rss_delete)
+			ret = ops->remove_rxfh_context(dev, ctx,
+						       rxfh.rss_context);
+		else
+			ret = ops->modify_rxfh_context(dev, ctx, &rxfh_dev);
+	} else {
+		ret = ops->set_rxfh(dev, &rxfh_dev, extack);
+	}
+	if (ret) {
+		if (create) {
 			/* failed to create, free our new tracking entry */
+			if (ops->create_rxfh_context)
+				xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context);
 			kfree(ctx);
+		}
 		goto out;
 	}
 
@@ -1426,12 +1454,8 @@  static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 			dev->priv_flags |= IFF_RXFH_CONFIGURED;
 	}
 	/* Update rss_ctx tracking */
-	if (create) {
-		/* Ideally this should happen before calling the driver,
-		 * so that we can fail more cleanly; but we don't have the
-		 * context ID until the driver picks it, so we have to
-		 * wait until after.
-		 */
+	if (create && !ops->create_rxfh_context) {
+		/* driver uses old API, it chose context ID */
 		if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) {
 			/* context ID reused, our tracking is screwed */
 			kfree(ctx);
@@ -1443,8 +1467,6 @@  static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 			kfree(ctx);
 			goto out;
 		}
-		ctx->indir_configured = rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE;
-		ctx->key_configured = !!rxfh.key_size;
 	}
 	if (rxfh_dev.rss_delete) {
 		WARN_ON(xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);