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

[Martin Habets <habetsm.xilinx@gmail.com>]

On Wed, Sep 27, 2023 at 07:13:35PM +0100, edward.cree@amd.com wrote:
> 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_context API for now as a fallback, but
>  deprecate it.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>

> ---
>  include/linux/ethtool.h | 40 ++++++++++++++++++++++++---
>  net/core/dev.c          | 15 ++++++++---
>  net/ethtool/ioctl.c     | 60 ++++++++++++++++++++++++++++++-----------
>  3 files changed, 93 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 229a23571008..975fda7218f8 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -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.
> @@ -901,6 +924,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);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 05e95abdfd17..637218adca22 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10882,16 +10882,23 @@ static void netdev_rss_contexts_free(struct net_device *dev)
>  	struct ethtool_rxfh_context *ctx;
>  	unsigned long context;
>  
> -	if (dev->ethtool_ops->set_rxfh_context)
> +	if (dev->ethtool_ops->create_rxfh_context ||
> +	    dev->ethtool_ops->set_rxfh_context)
>  		xa_for_each(&dev->ethtool->rss_ctx, context, ctx) {
>  			u32 *indir = ethtool_rxfh_context_indir(ctx);
>  			u8 *key = ethtool_rxfh_context_key(ctx);
>  			u32 concast = context;
>  
>  			xa_erase(&dev->ethtool->rss_ctx, context);
> -			dev->ethtool_ops->set_rxfh_context(dev, indir, key,
> -							   ctx->hfunc, &concast,
> -							   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,
> +								   &concast,
> +								   true);
>  			kfree(ctx);
>  		}
>  	xa_destroy(&dev->ethtool->rss_ctx);
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 1d13bc8fbb75..c23d2bd3cd2a 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -1274,7 +1274,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd8[2] || rxfh.rsvd32)
>  		return -EINVAL;
>  	/* Most drivers don't handle rss_context, check it's 0 as well */
> -	if (rxfh.rss_context && !ops->set_rxfh_context)
> +	if (rxfh.rss_context && !(ops->create_rxfh_context ||
> +				  ops->set_rxfh_context))
>  		return -EOPNOTSUPP;
>  	create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
>  
> @@ -1349,8 +1350,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->priv_size = ops->rxfh_priv_size;
> +		/* Initialise to an empty context */
> +		ctx->indir_no_change = ctx->key_no_change = 1;
> +		ctx->hfunc = ETH_RSS_HASH_NO_CHANGE;
> +		if (ops->create_rxfh_context) {
> +			u32 limit = dev->ethtool->rss_ctx_max_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) {
> @@ -1359,15 +1376,34 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  		}
>  	}
>  
> -	if (rxfh.rss_context)
> -		ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
> -					    &rxfh.rss_context, delete);
> -	else
> +	if (rxfh.rss_context) {
> +		if (ops->create_rxfh_context) {
> +			if (create)
> +				ret = ops->create_rxfh_context(dev, ctx, indir,
> +							       hkey, rxfh.hfunc,
> +							       rxfh.rss_context);
> +			else if (delete)
> +				ret = ops->remove_rxfh_context(dev, ctx,
> +							       rxfh.rss_context);
> +			else
> +				ret = ops->modify_rxfh_context(dev, ctx, indir,
> +							       hkey, rxfh.hfunc,
> +							       rxfh.rss_context);
> +		} else {
> +			ret = ops->set_rxfh_context(dev, indir, hkey,
> +						    rxfh.hfunc,
> +						    &rxfh.rss_context, delete);
> +		}
> +	} else {
>  		ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
> +	}
>  	if (ret) {
> -		if (create)
> +		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;
>  	}
>  
> @@ -1383,12 +1419,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);
> @@ -1400,8 +1432,6 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
>  			kfree(ctx);
>  			goto out;
>  		}
> -		ctx->indir_no_change = rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE;
> -		ctx->key_no_change = !rxfh.key_size;
>  	}
>  	if (delete) {
>  		WARN_ON(xa_erase(&dev->ethtool->rss_ctx, rxfh.rss_context) != ctx);