diff mbox series

[RFC,net-next,3/6] net: ethtool: let the core choose RSS context IDs

Message ID 00a28ff573df347ba0762004bc8c7aa8dfcf31f6.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 12 maintainers not CCed: leon@kernel.org hkelam@marvell.com sbhatta@marvell.com d-tatianin@yandex-team.ru linux@armlinux.org.uk sgoutham@marvell.com gakula@marvell.com linux-rdma@vger.kernel.org mw@semihalf.com saeedm@nvidia.com vladimir.oltean@nxp.com andrew@lunn.ch
netdev/build_clang fail Errors and warnings before: 567 this patch: 578
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 fail Errors and warnings before: 2005 this patch: 1788
netdev/checkpatch warning CHECK: multiple assignments should be avoided WARNING: function definition argument 'struct net_device *' should also have an identifier name WARNING: line length of 81 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:33 p.m. UTC 
From: Edward Cree <ecree.xilinx@gmail.com>

Rename the existing .set_rxfh_context API to .set_rxfh_context_old, and
 add a new .set_rxfh_context API that passes in the newly-chosen context
 ID (not as a pointer) rather than leaving the driver to choose it.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |  2 +-
 .../marvell/octeontx2/nic/otx2_ethtool.c      |  2 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  2 +-
 drivers/net/ethernet/sfc/ef100_ethtool.c      |  2 +-
 drivers/net/ethernet/sfc/ethtool.c            |  2 +-
 drivers/net/ethernet/sfc/siena/ethtool.c      |  2 +-
 include/linux/ethtool.h                       |  8 ++-
 net/core/dev.c                                | 13 +++--
 net/ethtool/ioctl.c                           | 49 +++++++++++++------
 9 files changed, 58 insertions(+), 24 deletions(-)

Comments

Jakub Kicinski April 3, 2023, 9:54 p.m. UTC | #1 
On Mon, 3 Apr 2023 17:33:00 +0100 edward.cree@amd.com wrote:
>  	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
>  				    const u8 *key, const u8 hfunc,
> -				    u32 *rss_context, bool delete);
> +				    u32 rss_context, bool delete);

Would it be easier to pass struct ethtool_rxfh_context instead of
doing it field by field?  Otherwise Intel will need to add more
arguments and touch all drivers. Or are you thinking that they should
use a separate callback for the "RR RSS" or whatever their thing is?

And maybe separate op for create / change / delete?

And an extack on top... :)
Edward Cree April 4, 2023, 12:14 p.m. UTC | #2 
On 03/04/2023 22:54, Jakub Kicinski wrote:
> On Mon, 3 Apr 2023 17:33:00 +0100 edward.cree@amd.com wrote:
>>  	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
>>  				    const u8 *key, const u8 hfunc,
>> -				    u32 *rss_context, bool delete);
>> +				    u32 rss_context, bool delete);
> 
> Would it be easier to pass struct ethtool_rxfh_context instead of
> doing it field by field?  Otherwise Intel will need to add more
> arguments and touch all drivers. Or are you thinking that they should
> use a separate callback for the "RR RSS" or whatever their thing is?

Initially I tried to just pass in ctx with the new values already
 filled in.  But that breaks if the op fails; we have to leave the
 old values in ctx.  We maybe could create a second, ephemeral
 struct ethtool_rxfh_context to pass the new values in, but then
 we have to worry about which one's priv the driver uses.
(We can't e.g. just pass in the ephemeral one, and copy its priv
 across when we update the real ctx after the op returns, because
 what if the driver stores, say, a list_head in its priv?)

And if we did pass a struct wrapping indir, key and hfunc, then
 any patch adding more fields to it would need existing drivers
 to check the new fields were unused / set to NO_CHANGE.

So I think we just have to accept that new fields will mean
 changing all drivers.  (There's only half a dozen, anyway.)
And doing that through the op arguments means the compiler will
 catch any driver that hasn't been updated, rather than the
 driver potentially silently ignoring the new field.

> And maybe separate op for create / change / delete?

Good idea, that would also elide renaming the legacy op.

> And an extack on top... :)

Sure.
Jakub Kicinski April 4, 2023, 11:42 p.m. UTC | #3 
On Tue, 4 Apr 2023 13:14:39 +0100 Edward Cree wrote:
> > Would it be easier to pass struct ethtool_rxfh_context instead of
> > doing it field by field?  Otherwise Intel will need to add more
> > arguments and touch all drivers. Or are you thinking that they should
> > use a separate callback for the "RR RSS" or whatever their thing is?  
> 
> Initially I tried to just pass in ctx with the new values already
>  filled in.  But that breaks if the op fails; we have to leave the
>  old values in ctx.  We maybe could create a second, ephemeral
>  struct ethtool_rxfh_context to pass the new values in, but then
>  we have to worry about which one's priv the driver uses.
> (We can't e.g. just pass in the ephemeral one, and copy its priv
>  across when we update the real ctx after the op returns, because
>  what if the driver stores, say, a list_head in its priv?)
> 
> And if we did pass a struct wrapping indir, key and hfunc, then
>  any patch adding more fields to it would need existing drivers
>  to check the new fields were unused / set to NO_CHANGE.
> 
> So I think we just have to accept that new fields will mean
>  changing all drivers.  (There's only half a dozen, anyway.)
> And doing that through the op arguments means the compiler will
>  catch any driver that hasn't been updated, rather than the
>  driver potentially silently ignoring the new field.

Fair point with needing to copy in case of error, okay :(
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index adc953611913..8d626c753f8e 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -5758,7 +5758,7 @@  static const struct ethtool_ops mvpp2_eth_tool_ops = {
 	.get_rxfh		= mvpp2_ethtool_get_rxfh,
 	.set_rxfh		= mvpp2_ethtool_set_rxfh,
 	.get_rxfh_context	= mvpp2_ethtool_get_rxfh_context,
-	.set_rxfh_context	= mvpp2_ethtool_set_rxfh_context,
+	.set_rxfh_context_old	= mvpp2_ethtool_set_rxfh_context,
 };
 
 /* Used for PPv2.1, or PPv2.2 with the old Device Tree binding that
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
index 0f8d1a69139f..67310434cb18 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
@@ -1325,7 +1325,7 @@  static const struct ethtool_ops otx2_ethtool_ops = {
 	.get_rxfh		= otx2_get_rxfh,
 	.set_rxfh		= otx2_set_rxfh,
 	.get_rxfh_context	= otx2_get_rxfh_context,
-	.set_rxfh_context	= otx2_set_rxfh_context,
+	.set_rxfh_context_old	= otx2_set_rxfh_context,
 	.get_msglevel		= otx2_get_msglevel,
 	.set_msglevel		= otx2_set_msglevel,
 	.get_pauseparam		= otx2_get_pauseparam,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 1f5a2110d31f..edf099d64fdb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -2425,7 +2425,7 @@  const struct ethtool_ops mlx5e_ethtool_ops = {
 	.get_rxfh          = mlx5e_get_rxfh,
 	.set_rxfh          = mlx5e_set_rxfh,
 	.get_rxfh_context  = mlx5e_get_rxfh_context,
-	.set_rxfh_context  = mlx5e_set_rxfh_context,
+	.set_rxfh_context_old = mlx5e_set_rxfh_context,
 	.get_rxnfc         = mlx5e_get_rxnfc,
 	.set_rxnfc         = mlx5e_set_rxnfc,
 	.get_tunable       = mlx5e_get_tunable,
diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
index 702abbe59b76..ec210ad77b21 100644
--- a/drivers/net/ethernet/sfc/ef100_ethtool.c
+++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
@@ -61,7 +61,7 @@  const struct ethtool_ops ef100_ethtool_ops = {
 	.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,
+	.set_rxfh_context_old	= efx_ethtool_set_rxfh_context,
 
 	.get_module_info	= efx_ethtool_get_module_info,
 	.get_module_eeprom	= efx_ethtool_get_module_eeprom,
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 364323599f7b..6c421cb1a9cf 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -270,7 +270,7 @@  const struct ethtool_ops efx_ethtool_ops = {
 	.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,
+	.set_rxfh_context_old	= efx_ethtool_set_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,
diff --git a/drivers/net/ethernet/sfc/siena/ethtool.c b/drivers/net/ethernet/sfc/siena/ethtool.c
index e4ec589216c1..1378d1cfc5e2 100644
--- a/drivers/net/ethernet/sfc/siena/ethtool.c
+++ b/drivers/net/ethernet/sfc/siena/ethtool.c
@@ -270,7 +270,7 @@  const struct ethtool_ops efx_siena_ethtool_ops = {
 	.get_rxfh		= efx_siena_ethtool_get_rxfh,
 	.set_rxfh		= efx_siena_ethtool_set_rxfh,
 	.get_rxfh_context	= efx_siena_ethtool_get_rxfh_context,
-	.set_rxfh_context	= efx_siena_ethtool_set_rxfh_context,
+	.set_rxfh_context_old	= efx_siena_ethtool_set_rxfh_context,
 	.get_ts_info		= efx_ethtool_get_ts_info,
 	.get_module_info	= efx_siena_ethtool_get_module_info,
 	.get_module_eeprom	= efx_siena_ethtool_get_module_eeprom,
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index a16580a8e9d7..0c7df2e043b2 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -752,6 +752,9 @@  struct ethtool_mm_stats {
  *	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.
+ * @set_rxfh_context_old: Legacy version of @set_rxfh_context, where driver
+ *	chooses the new context ID in the %ETH_RXFH_CONTEXT_ALLOC case.
+ *	Arguments and return otherwise the same.
  * @get_channels: Get number of channels.
  * @set_channels: Set number of channels.  Returns a negative error code or
  *	zero.
@@ -901,7 +904,10 @@  struct ethtool_ops {
 				    u8 *hfunc, u32 rss_context);
 	int	(*set_rxfh_context)(struct net_device *, const u32 *indir,
 				    const u8 *key, const u8 hfunc,
-				    u32 *rss_context, bool delete);
+				    u32 rss_context, bool delete);
+	int	(*set_rxfh_context_old)(struct net_device *, const u32 *indir,
+					const u8 *key, const u8 hfunc,
+					u32 *rss_context, bool delete);
 	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 d0a936d4e532..0600945a6810 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10785,15 +10785,22 @@  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->set_rxfh_context &&
+	    !dev->ethtool_ops->set_rxfh_context_old)
 		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);
+		if (dev->ethtool_ops->set_rxfh_context)
+			dev->ethtool_ops->set_rxfh_context(dev, indir, key,
+							   ctx->hfunc, context,
+							   true);
+		else
+			dev->ethtool_ops->set_rxfh_context_old(dev, indir, key,
+							       ctx->hfunc,
+							       &context, true);
 		kfree(ctx);
 	}
 }
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index c8f11ac343c9..9e41dc9151d2 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1273,7 +1273,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->set_rxfh_context ||
+				  ops->set_rxfh_context_old))
 		return -EOPNOTSUPP;
 	create = rxfh.rss_context == ETH_RXFH_CONTEXT_ALLOC;
 	if (create && ops->get_rxfh_priv_size)
@@ -1350,8 +1351,27 @@  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 = dev_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->set_rxfh_context) {
+			int ctx_id;
+
+			/* driver uses new API, core allocates ID */
+			/* if rss_ctx_max_id is not specified (left as 0), it is
+			 * treated as INT_MAX + 1 by idr_alloc
+			 */
+			ctx_id = idr_alloc(&dev->rss_ctx, ctx, 1,
+					   dev->rss_ctx_max_id, GFP_KERNEL);
+			/* 0 is not allowed, so treat it like an error here */
+			if (ctx_id <= 0) {
+				kfree(ctx);
+				ret = -ENOMEM;
+				goto out;
+			}
+			rxfh.rss_context = ctx_id;
+		}
 	} else if (rxfh.rss_context) {
 		ctx = idr_find(&dev->rss_ctx, rxfh.rss_context);
 		if (!ctx) {
@@ -1360,11 +1380,18 @@  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->set_rxfh_context)
+			ret = ops->set_rxfh_context(dev, indir, hkey, rxfh.hfunc,
+						    rxfh.rss_context, delete);
+		else
+			ret = ops->set_rxfh_context_old(dev, indir, hkey,
+							rxfh.hfunc,
+							&rxfh.rss_context,
+							delete);
+	} else {
 		ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
+	}
 	if (ret)
 		goto out;
 
@@ -1380,12 +1407,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->set_rxfh_context) {
+		/* driver uses old API, it chose context ID */
 		if (WARN_ON(idr_find(&dev->rss_ctx, rxfh.rss_context)))
 			/* context ID reused, our tracking is screwed */
 			goto out;
@@ -1393,8 +1416,6 @@  static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		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);