[PATCH net-next v2 00/12] ethtool: rss: driver tweaks and netlink context dumps

[PATCH net-next v2 00/12] ethtool: rss: driver tweaks and netlink context dumps

[Jakub Kicinski]

This series is a semi-related collection of RSS patches.
Main point is supporting dumping RSS contexts via ethtool netlink.
At present additional RSS contexts can be queried one by one, and
assuming user know the right IDs. This series uses the XArray
added by Ed to provide netlink dump support for ETHTOOL_GET_RSS.

Patch 1 is a trivial selftest debug patch.
Patch 2 coverts mvpp2 for no real reason other than that I had
	a grand plan of converting all drivers at some stage.
Patch 3 removes a now moot check from mlx5 so that all tests
	can pass.
Patch 4 and 5 make a bit used for context support optional,
	for easier grepping of drivers which need converting
	if nothing else.
Patch 6 OTOH adds a new cap bit; some devices don't support
	using a different key per context and currently act
	in surprising ways.
Patch 7 and 8 update the RSS netlink code to use XArray.
Patch 9 and 10 add support for dumping contexts.
Patch 11 and 12 are small adjustments to spec and a new test.


I'm getting distracted with other work, so probably won't have
the time soon to complete next steps, but things which are missing
are (and some of these may be bad ideas):

 - better discovery

   Some sort of API to tell the user who many contexts the device
   can create. Upper bound, devices often share contexts between
   ports etc. so it's hard to tell exactly and upfront number of
   contexts for a netdev. But order of magnitude (4 vs 10s) may
   be enough for container management system to know whether to bother.

 - create/modify/delete via netlink
 
   The only question here is how to handle all the tricky IOCTL
   legacy. "No change" maps trivially to attribute not present.
   "reset" (indir_size = 0) probably needs to be a new NLA_FLAG?

 - better table size handling

   The current API assumes the LUT has fixed size, which isn't
   true for modern devices. We should have better APIs for the
   drivers to resize the tables, and in user facing API -
   the ability to specify pattern and min size rather than
   exact table expected (sort of like ethtool CLI already does).

 - recounted / socket-bound contexts

   Support for contexts which get "cleaned up" when their parent
   netlink socket gets closed. The major catch is that ntuple
   filters (which we don't currently track) depend on the context,
   so we need auto-removal for both.

v2:
 - fix bugs and build in mvpp2
v1: https://lore.kernel.org/20240802001801.565176-1-kuba@kernel.org

Jakub Kicinski (12):
  selftests: drv-net: rss_ctx: add identifier to traffic comments
  eth: mvpp2: implement new RSS context API
  eth: mlx5: allow disabling queues when RSS contexts exist
  ethtool: make ethtool_ops::cap_rss_ctx_supported optional
  eth: remove .cap_rss_ctx_supported from updated drivers
  ethtool: rss: don't report key if device doesn't support it
  ethtool: rss: move the device op invocation out of rss_prepare_data()
  ethtool: rss: report info about additional contexts from XArray
  ethtool: rss: support dumping RSS contexts
  ethtool: rss: support skipping contexts during dump
  netlink: specs: decode indirection table as u32 array
  selftests: drv-net: rss_ctx: test dumping RSS contexts

 Documentation/netlink/specs/ethtool.yaml      |  14 +-
 Documentation/networking/ethtool-netlink.rst  |  12 +-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |   2 +-
 drivers/net/ethernet/intel/ice/ice_ethtool.c  |   1 +
 .../net/ethernet/marvell/mvpp2/mvpp2_cls.c    |  18 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_cls.h    |   2 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 103 +++++---
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  13 +-
 drivers/net/ethernet/sfc/ef100_ethtool.c      |   2 +-
 drivers/net/ethernet/sfc/ethtool.c            |   2 +-
 drivers/net/ethernet/sfc/siena/ethtool.c      |   1 +
 include/linux/ethtool.h                       |   6 +-
 include/uapi/linux/ethtool_netlink.h          |   1 +
 net/ethtool/ioctl.c                           |  31 ++-
 net/ethtool/netlink.c                         |   2 +
 net/ethtool/netlink.h                         |   4 +-
 net/ethtool/rss.c                             | 231 ++++++++++++++++--
 .../selftests/drivers/net/hw/rss_ctx.py       |  74 +++++-
 tools/testing/selftests/net/lib/py/ksft.py    |   6 +
 19 files changed, 434 insertions(+), 91 deletions(-)

-- 
2.45.2

[PATCH net-next v2 01/12] selftests: drv-net: rss_ctx: add identifier to traffic comments

[Jakub Kicinski]

Include the "name" of the context in the comment for traffic
checks. Makes it easier to reason about which context failed
when we loop over 32 contexts (it may matter if we failed in
first vs last, for example).

Reviewed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/hw/rss_ctx.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
index 011508ca604b..1da6b214f4fe 100755
--- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
+++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
@@ -90,10 +90,10 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
     ksft_ge(directed, 20000, f"traffic on {name}: " + str(cnts))
     if params.get('noise'):
         ksft_lt(sum(cnts[i] for i in params['noise']), directed / 2,
-                "traffic on other queues:" + str(cnts))
+                f"traffic on other queues ({name})':" + str(cnts))
     if params.get('empty'):
         ksft_eq(sum(cnts[i] for i in params['empty']), 0,
-                "traffic on inactive queues: " + str(cnts))
+                f"traffic on inactive queues ({name}): " + str(cnts))
 
 
 def test_rss_key_indir(cfg):
-- 
2.45.2

Re: [PATCH net-next v2 01/12] selftests: drv-net: rss_ctx: add identifier to traffic comments

[Gal Pressman]

On 03/08/2024 7:26, Jakub Kicinski wrote:
> Include the "name" of the context in the comment for traffic
> checks. Makes it easier to reason about which context failed
> when we loop over 32 contexts (it may matter if we failed in
> first vs last, for example).
> 
> Reviewed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/hw/rss_ctx.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> index 011508ca604b..1da6b214f4fe 100755
> --- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> +++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> @@ -90,10 +90,10 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
>      ksft_ge(directed, 20000, f"traffic on {name}: " + str(cnts))
>      if params.get('noise'):
>          ksft_lt(sum(cnts[i] for i in params['noise']), directed / 2,
> -                "traffic on other queues:" + str(cnts))
> +                f"traffic on other queues ({name})':" + str(cnts))

You've already converted to an f-string, why not shove in the 'str(cnts)'?

Reviewed-by: Gal Pressman <gal@nvidia.com>

Re: [PATCH net-next v2 01/12] selftests: drv-net: rss_ctx: add identifier to traffic comments

[Jakub Kicinski]

On Sun, 4 Aug 2024 09:40:58 +0300 Gal Pressman wrote:
> > diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> > index 011508ca604b..1da6b214f4fe 100755
> > --- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> > +++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> > @@ -90,10 +90,10 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
> >      ksft_ge(directed, 20000, f"traffic on {name}: " + str(cnts))
> >      if params.get('noise'):
> >          ksft_lt(sum(cnts[i] for i in params['noise']), directed / 2,
> > -                "traffic on other queues:" + str(cnts))
> > +                f"traffic on other queues ({name})':" + str(cnts))  
> 
> You've already converted to an f-string, why not shove in the 'str(cnts)'?

For (possibly misguided) consistency, there are 2 other functions which
use the " + str(cnts)" format to append the per-queue stats to comment.

> Reviewed-by: Gal Pressman <gal@nvidia.com>

Thanks!

[PATCH net-next v2 02/12] eth: mvpp2: implement new RSS context API

[Jakub Kicinski]

Implement the separate create/modify/delete ops for RSS.

No problems with IDs - even tho RSS tables are per device
the driver already seems to allocate IDs linearly per port.
There's a translation table from per-port context ID
to device context ID.

mvpp2 doesn't have a key for the hash, it defaults to
an empty/previous indir table.

Compile-tested only.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - copy the default indir table
 - use ID from the core
 - fix build issues

CC: marcin.s.wojtas@gmail.com
CC: linux@armlinux.org.uk
---
 .../net/ethernet/marvell/mvpp2/mvpp2_cls.c    |  18 +---
 .../net/ethernet/marvell/mvpp2/mvpp2_cls.h    |   2 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 102 +++++++++++++-----
 3 files changed, 79 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c
index 40aeaa7bd739..1641791a2d5b 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c
@@ -1522,29 +1522,19 @@ static int mvpp22_rss_context_create(struct mvpp2_port *port, u32 *rss_ctx)
 	return 0;
 }
 
-int mvpp22_port_rss_ctx_create(struct mvpp2_port *port, u32 *port_ctx)
+int mvpp22_port_rss_ctx_create(struct mvpp2_port *port, u32 port_ctx)
 {
 	u32 rss_ctx;
-	int ret, i;
+	int ret;
 
 	ret = mvpp22_rss_context_create(port, &rss_ctx);
 	if (ret)
 		return ret;
 
-	/* Find the first available context number in the port, starting from 1.
-	 * Context 0 on each port is reserved for the default context.
-	 */
-	for (i = 1; i < MVPP22_N_RSS_TABLES; i++) {
-		if (port->rss_ctx[i] < 0)
-			break;
-	}
-
-	if (i == MVPP22_N_RSS_TABLES)
+	if (WARN_ON_ONCE(port->rss_ctx[port_ctx] >= 0))
 		return -EINVAL;
 
-	port->rss_ctx[i] = rss_ctx;
-	*port_ctx = i;
-
+	port->rss_ctx[port_ctx] = rss_ctx;
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.h
index 663157dc8062..85c9c6e80678 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.h
@@ -264,7 +264,7 @@ int mvpp22_port_rss_init(struct mvpp2_port *port);
 int mvpp22_port_rss_enable(struct mvpp2_port *port);
 int mvpp22_port_rss_disable(struct mvpp2_port *port);
 
-int mvpp22_port_rss_ctx_create(struct mvpp2_port *port, u32 *rss_ctx);
+int mvpp22_port_rss_ctx_create(struct mvpp2_port *port, u32 rss_ctx);
 int mvpp22_port_rss_ctx_delete(struct mvpp2_port *port, u32 rss_ctx);
 
 int mvpp22_port_rss_ctx_indir_set(struct mvpp2_port *port, u32 rss_ctx,
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 0d62a33afa80..90182f6fd9a1 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -5696,38 +5696,80 @@ static int mvpp2_ethtool_get_rxfh(struct net_device *dev,
 	return ret;
 }
 
+static bool mvpp2_ethtool_rxfh_okay(struct mvpp2_port *port,
+				    const struct ethtool_rxfh_param *rxfh)
+{
+	if (!mvpp22_rss_is_supported(port))
+		return false;
+
+	if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
+	    rxfh->hfunc != ETH_RSS_HASH_CRC32)
+		return false;
+
+	if (rxfh->key)
+		return false;
+
+	return true;
+}
+
+static int mvpp2_create_rxfh_context(struct net_device *dev,
+				     struct ethtool_rxfh_context *ctx,
+				     const struct ethtool_rxfh_param *rxfh,
+				     struct netlink_ext_ack *extack)
+{
+	struct mvpp2_port *port = netdev_priv(dev);
+	int ret = 0;
+
+	if (!mvpp2_ethtool_rxfh_okay(port, rxfh))
+		return -EOPNOTSUPP;
+
+	ctx->hfunc = ETH_RSS_HASH_CRC32;
+
+	ret = mvpp22_port_rss_ctx_create(port, rxfh->rss_context);
+	if (ret)
+		return ret;
+
+	if (!rxfh->indir)
+		ret = mvpp22_port_rss_ctx_indir_get(port, rxfh->rss_context,
+						    ethtool_rxfh_context_indir(ctx));
+	else
+		ret = mvpp22_port_rss_ctx_indir_set(port, rxfh->rss_context,
+						    rxfh->indir);
+	return ret;
+}
+
+static int mvpp2_modify_rxfh_context(struct net_device *dev,
+				     struct ethtool_rxfh_context *ctx,
+				     const struct ethtool_rxfh_param *rxfh,
+				     struct netlink_ext_ack *extack)
+{
+	struct mvpp2_port *port = netdev_priv(dev);
+	int ret = 0;
+
+	if (!mvpp2_ethtool_rxfh_okay(port, rxfh))
+		return -EOPNOTSUPP;
+
+	if (rxfh->indir)
+		ret = mvpp22_port_rss_ctx_indir_set(port, rxfh->rss_context,
+						    rxfh->indir);
+	return ret;
+}
+
+static int mvpp2_remove_rxfh_context(struct net_device *dev,
+				     struct ethtool_rxfh_context *ctx,
+				     u32 rss_context,
+				     struct netlink_ext_ack *extack)
+{
+	struct mvpp2_port *port = netdev_priv(dev);
+
+	return mvpp22_port_rss_ctx_delete(port, rss_context);
+}
+
 static int mvpp2_ethtool_set_rxfh(struct net_device *dev,
 				  struct ethtool_rxfh_param *rxfh,
 				  struct netlink_ext_ack *extack)
 {
-	struct mvpp2_port *port = netdev_priv(dev);
-	u32 *rss_context = &rxfh->rss_context;
-	int ret = 0;
-
-	if (!mvpp22_rss_is_supported(port))
-		return -EOPNOTSUPP;
-
-	if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
-	    rxfh->hfunc != ETH_RSS_HASH_CRC32)
-		return -EOPNOTSUPP;
-
-	if (rxfh->key)
-		return -EOPNOTSUPP;
-
-	if (*rss_context && rxfh->rss_delete)
-		return mvpp22_port_rss_ctx_delete(port, *rss_context);
-
-	if (*rss_context == ETH_RXFH_CONTEXT_ALLOC) {
-		ret = mvpp22_port_rss_ctx_create(port, rss_context);
-		if (ret)
-			return ret;
-	}
-
-	if (rxfh->indir)
-		ret = mvpp22_port_rss_ctx_indir_set(port, *rss_context,
-						    rxfh->indir);
-
-	return ret;
+	return mvpp2_modify_rxfh_context(dev, NULL, rxfh, extack);
 }
 
 /* Device ops */
@@ -5750,6 +5792,7 @@ static const struct net_device_ops mvpp2_netdev_ops = {
 
 static const struct ethtool_ops mvpp2_eth_tool_ops = {
 	.cap_rss_ctx_supported	= true,
+	.rxfh_max_context_id	= MVPP22_N_RSS_TABLES,
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES,
 	.nway_reset		= mvpp2_ethtool_nway_reset,
@@ -5772,6 +5815,9 @@ static const struct ethtool_ops mvpp2_eth_tool_ops = {
 	.get_rxfh_indir_size	= mvpp2_ethtool_get_rxfh_indir_size,
 	.get_rxfh		= mvpp2_ethtool_get_rxfh,
 	.set_rxfh		= mvpp2_ethtool_set_rxfh,
+	.create_rxfh_context	= mvpp2_create_rxfh_context,
+	.modify_rxfh_context	= mvpp2_modify_rxfh_context,
+	.remove_rxfh_context	= mvpp2_remove_rxfh_context,
 };
 
 /* Used for PPv2.1, or PPv2.2 with the old Device Tree binding that
-- 
2.45.2

Re: [PATCH net-next v2 02/12] eth: mvpp2: implement new RSS context API

[Edward Cree]

On 03/08/2024 05:26, Jakub Kicinski wrote:
> Implement the separate create/modify/delete ops for RSS.
> 
> No problems with IDs - even tho RSS tables are per device
> the driver already seems to allocate IDs linearly per port.
> There's a translation table from per-port context ID
> to device context ID.
> 
> mvpp2 doesn't have a key for the hash, it defaults to
> an empty/previous indir table.

Given that, should this be after patch #6?  So as to make it
 obviously correct not to populate ethtool_rxfh_context_key(ctx)
 with the default context's key.

> @@ -5750,6 +5792,7 @@ static const struct net_device_ops mvpp2_netdev_ops = {
>  
>  static const struct ethtool_ops mvpp2_eth_tool_ops = {
>  	.cap_rss_ctx_supported	= true,
> +	.rxfh_max_context_id	= MVPP22_N_RSS_TABLES,

Max ID is inclusive, not exclusive, so I think this should be
 MVPP22_N_RSS_TABLES - 1?

Re: [PATCH net-next v2 02/12] eth: mvpp2: implement new RSS context API

[Jakub Kicinski]

On Mon, 5 Aug 2024 12:25:28 +0100 Edward Cree wrote:
> > mvpp2 doesn't have a key for the hash, it defaults to
> > an empty/previous indir table.  
> 
> Given that, should this be after patch #6?  So as to make it
>  obviously correct not to populate ethtool_rxfh_context_key(ctx)
>  with the default context's key.

It's a bit different. Patch 6 is about devices which have a key but 
the same key is used for all contexts. mvpp2 has no key at all
even for context 0 (get_rxfh_key_size is not defined).

> > @@ -5750,6 +5792,7 @@ static const struct net_device_ops mvpp2_netdev_ops = {
> >  
> >  static const struct ethtool_ops mvpp2_eth_tool_ops = {
> >  	.cap_rss_ctx_supported	= true,
> > +	.rxfh_max_context_id	= MVPP22_N_RSS_TABLES,  
> 
> Max ID is inclusive, not exclusive, so I think this should be
>  MVPP22_N_RSS_TABLES - 1?

I totally did check this before sending:

 * @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.

But you're right, the code acts as if it was inclusive :S

Coincidentally, the default also appears exclusive:

	u32 limit = ops->rxfh_max_context_id ?: U32_MAX;

U32_MAX can't be used, it has special meaning:

#define ETH_RXFH_CONTEXT_ALLOC		0xffffffff

These seem like net-worthy fixes, no?

Re: [PATCH net-next v2 02/12] eth: mvpp2: implement new RSS context API

[Edward Cree]

On 05/08/2024 22:29, Jakub Kicinski wrote:
> On Mon, 5 Aug 2024 12:25:28 +0100 Edward Cree wrote:
>>> mvpp2 doesn't have a key for the hash, it defaults to
>>> an empty/previous indir table.  
>>
>> Given that, should this be after patch #6?  So as to make it
>>  obviously correct not to populate ethtool_rxfh_context_key(ctx)
>>  with the default context's key.
> 
> It's a bit different. Patch 6 is about devices which have a key but 
> the same key is used for all contexts. mvpp2 has no key at all
> even for context 0 (get_rxfh_key_size is not defined).

Oh, I see.  Clarify that in the commit message, perhaps?

>>> @@ -5750,6 +5792,7 @@ static const struct net_device_ops mvpp2_netdev_ops = {
>>>  
>>>  static const struct ethtool_ops mvpp2_eth_tool_ops = {
>>>  	.cap_rss_ctx_supported	= true,
>>> +	.rxfh_max_context_id	= MVPP22_N_RSS_TABLES,  
>>
>> Max ID is inclusive, not exclusive, so I think this should be
>>  MVPP22_N_RSS_TABLES - 1?
> 
> I totally did check this before sending:
> 
>  * @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.
> 
> But you're right, the code acts as if it was inclusive :S

Mea culpa, clearly when I was porting to XArray I must have
 confused myself over this.

> Coincidentally, the default also appears exclusive:
> 
> 	u32 limit = ops->rxfh_max_context_id ?: U32_MAX;
> 
> U32_MAX can't be used, it has special meaning:
> 
> #define ETH_RXFH_CONTEXT_ALLOC		0xffffffff

Given that both the default and drivers look more reasonable
 with an exclusive than an inclusive limit (I assume most
 drivers with a limit will have an N, like mvpp2 does, rather
 than a MAX), I guess we should change the code to match the
 doc rather than the other way around.

> These seem like net-worthy fixes, no?

Yep, agreed.  I'll send a patch.

Re: [PATCH net-next v2 02/12] eth: mvpp2: implement new RSS context API

[Jakub Kicinski]

On Tue, 6 Aug 2024 14:28:16 +0100 Edward Cree wrote:
> > Coincidentally, the default also appears exclusive:
> > 
> > 	u32 limit = ops->rxfh_max_context_id ?: U32_MAX;
> > 
> > U32_MAX can't be used, it has special meaning:
> > 
> > #define ETH_RXFH_CONTEXT_ALLOC		0xffffffff  
> 
> Given that both the default and drivers look more reasonable
>  with an exclusive than an inclusive limit (I assume most
>  drivers with a limit will have an N, like mvpp2 does, rather
>  than a MAX), I guess we should change the code to match the
>  doc rather than the other way around.

Somewhat unclear, because context 0 may not count, so to speak.
At least for bnxt using inclusive max context worked well.
But no preference, with the (obvious?) caveat that if we change 
the definition of the field to be exclusive we should rename it.

[PATCH net-next v2 03/12] eth: mlx5: allow disabling queues when RSS contexts exist

[Jakub Kicinski]

Since commit 24ac7e544081 ("ethtool: use the rss context XArray
in ring deactivation safety-check") core will prevent queues from
being disabled while being used by additional RSS contexts.
The safety check is no longer necessary, and core will do a more
accurate job of only rejecting changes which can actually break
things.

Reviewed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: saeedm@nvidia.com
CC: tariqt@nvidia.com
CC: leon@kernel.org
CC: linux-rdma@vger.kernel.org
---
 drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 36845872ae94..0b941482db30 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -445,7 +445,6 @@ int mlx5e_ethtool_set_channels(struct mlx5e_priv *priv,
 	unsigned int count = ch->combined_count;
 	struct mlx5e_params new_params;
 	bool arfs_enabled;
-	int rss_cnt;
 	bool opened;
 	int err = 0;
 
@@ -499,17 +498,6 @@ int mlx5e_ethtool_set_channels(struct mlx5e_priv *priv,
 		goto out;
 	}
 
-	/* Don't allow changing the number of channels if non-default RSS contexts exist,
-	 * the kernel doesn't protect against set_channels operations that break them.
-	 */
-	rss_cnt = mlx5e_rx_res_rss_cnt(priv->rx_res) - 1;
-	if (rss_cnt) {
-		err = -EINVAL;
-		netdev_err(priv->netdev, "%s: Non-default RSS contexts exist (%d), cannot change the number of channels\n",
-			   __func__, rss_cnt);
-		goto out;
-	}
-
 	/* Don't allow changing the number of channels if MQPRIO mode channel offload is active,
 	 * because it defines a partition over the channels queues.
 	 */
-- 
2.45.2

Re: [PATCH net-next v2 03/12] eth: mlx5: allow disabling queues when RSS contexts exist

[Gal Pressman]

On 03/08/2024 7:26, Jakub Kicinski wrote:
> Since commit 24ac7e544081 ("ethtool: use the rss context XArray
> in ring deactivation safety-check") core will prevent queues from
> being disabled while being used by additional RSS contexts.
> The safety check is no longer necessary, and core will do a more
> accurate job of only rejecting changes which can actually break
> things.

Very cool, thanks!
Reviewed-by: Gal Pressman <gal@nvidia.com>

[PATCH net-next v2 04/12] ethtool: make ethtool_ops::cap_rss_ctx_supported optional

[Jakub Kicinski]

cap_rss_ctx_supported was created because the API for creating
and configuring additional contexts is mux'ed with the normal
RSS API. Presence of ops does not imply driver can actually
support rss_context != 0 (in fact drivers mostly ignore that
field). cap_rss_ctx_supported lets core check that the driver
is context-aware before calling it.

Now that we have .create_rxfh_context, there is no such
ambiguity. We can depend on presence of the op.
Make setting the bit optional.

Reviewed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/ethtool.h | 3 ++-
 net/ethtool/ioctl.c     | 6 ++++--
 net/ethtool/rss.c       | 3 ++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 303fda54ef17..55c9f613ab64 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -727,7 +727,8 @@ struct kernel_ethtool_ts_info {
  * @cap_link_lanes_supported: indicates if the driver supports lanes
  *	parameter.
  * @cap_rss_ctx_supported: indicates if the driver supports RSS
- *	contexts.
+ *	contexts via legacy API, drivers implementing @create_rxfh_context
+ *	do not have to set this bit.
  * @cap_rss_sym_xor_supported: indicates if the driver supports symmetric-xor
  *	RSS.
  * @rxfh_indir_space: max size of RSS indirection tables, if indirection table
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 8ca13208d240..52dfb07393a6 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1227,7 +1227,8 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
 	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd32)
 		return -EINVAL;
 	/* Most drivers don't handle rss_context, check it's 0 as well */
-	if (rxfh.rss_context && !ops->cap_rss_ctx_supported)
+	if (rxfh.rss_context && !(ops->cap_rss_ctx_supported ||
+				  ops->create_rxfh_context))
 		return -EOPNOTSUPP;
 
 	rxfh.indir_size = rxfh_dev.indir_size;
@@ -1357,7 +1358,8 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd32)
 		return -EINVAL;
 	/* Most drivers don't handle rss_context, check it's 0 as well */
-	if (rxfh.rss_context && !ops->cap_rss_ctx_supported)
+	if (rxfh.rss_context && !(ops->cap_rss_ctx_supported ||
+				  ops->create_rxfh_context))
 		return -EOPNOTSUPP;
 	/* Check input data transformation capabilities */
 	if (rxfh.input_xfrm && rxfh.input_xfrm != RXH_XFRM_SYM_XOR &&
diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 5c4c4505ab9a..a06bdac8b8a2 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -60,7 +60,8 @@ rss_prepare_data(const struct ethnl_req_info *req_base,
 		return -EOPNOTSUPP;
 
 	/* Some drivers don't handle rss_context */
-	if (request->rss_context && !ops->cap_rss_ctx_supported)
+	if (request->rss_context && !(ops->cap_rss_ctx_supported ||
+				      ops->create_rxfh_context))
 		return -EOPNOTSUPP;
 
 	ret = ethnl_ops_begin(dev);
-- 
2.45.2

Re: [PATCH net-next v2 04/12] ethtool: make ethtool_ops::cap_rss_ctx_supported optional

[Gal Pressman]

On 03/08/2024 7:26, Jakub Kicinski wrote:
> cap_rss_ctx_supported was created because the API for creating
> and configuring additional contexts is mux'ed with the normal
> RSS API. Presence of ops does not imply driver can actually
> support rss_context != 0 (in fact drivers mostly ignore that
> field). cap_rss_ctx_supported lets core check that the driver
> is context-aware before calling it.
> 
> Now that we have .create_rxfh_context, there is no such
> ambiguity. We can depend on presence of the op.
> Make setting the bit optional.

Reviewed-by: Gal Pressman <gal@nvidia.com>

Re: [PATCH net-next v2 04/12] ethtool: make ethtool_ops::cap_rss_ctx_supported optional

[Edward Cree]

On 03/08/2024 05:26, Jakub Kicinski wrote:
> cap_rss_ctx_supported was created because the API for creating
> and configuring additional contexts is mux'ed with the normal
> RSS API. Presence of ops does not imply driver can actually
> support rss_context != 0 (in fact drivers mostly ignore that
> field). cap_rss_ctx_supported lets core check that the driver
> is context-aware before calling it.
> 
> Now that we have .create_rxfh_context, there is no such
> ambiguity. We can depend on presence of the op.
> Make setting the bit optional.
> 
> Reviewed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

[PATCH net-next v2 05/12] eth: remove .cap_rss_ctx_supported from updated drivers

[Jakub Kicinski]

Remove .cap_rss_ctx_supported from drivers which moved to the new API.
This makes it easy to grep for drivers which still need to be converted.

Reviewed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 1 -
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c   | 1 -
 drivers/net/ethernet/sfc/ef100_ethtool.c          | 1 -
 drivers/net/ethernet/sfc/ethtool.c                | 1 -
 4 files changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index ab8e3f197e7b..33e8cf0a3764 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -5289,7 +5289,6 @@ void bnxt_ethtool_free(struct bnxt *bp)
 
 const struct ethtool_ops bnxt_ethtool_ops = {
 	.cap_link_lanes_supported	= 1,
-	.cap_rss_ctx_supported		= 1,
 	.rxfh_max_context_id		= BNXT_MAX_ETH_RSS_CTX,
 	.rxfh_indir_space		= BNXT_MAX_RSS_TABLE_ENTRIES_P5,
 	.rxfh_priv_size			= sizeof(struct bnxt_rss_ctx),
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 90182f6fd9a1..5d5ad8d47e46 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -5791,7 +5791,6 @@ static const struct net_device_ops mvpp2_netdev_ops = {
 };
 
 static const struct ethtool_ops mvpp2_eth_tool_ops = {
-	.cap_rss_ctx_supported	= true,
 	.rxfh_max_context_id	= MVPP22_N_RSS_TABLES,
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES,
diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
index 896ffca4aee2..746b5314acb5 100644
--- a/drivers/net/ethernet/sfc/ef100_ethtool.c
+++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
@@ -37,7 +37,6 @@ ef100_ethtool_get_ringparam(struct net_device *net_dev,
 /*	Ethtool options available
  */
 const struct ethtool_ops ef100_ethtool_ops = {
-	.cap_rss_ctx_supported	= true,
 	.get_drvinfo		= efx_ethtool_get_drvinfo,
 	.get_msglevel		= efx_ethtool_get_msglevel,
 	.set_msglevel		= efx_ethtool_set_msglevel,
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 7c887160e2ef..15245720c949 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -240,7 +240,6 @@ static int efx_ethtool_get_ts_info(struct net_device *net_dev,
 }
 
 const struct ethtool_ops efx_ethtool_ops = {
-	.cap_rss_ctx_supported	= true,
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_USECS_IRQ |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
-- 
2.45.2

Re: [PATCH net-next v2 05/12] eth: remove .cap_rss_ctx_supported from updated drivers

[Gal Pressman]

On 03/08/2024 7:26, Jakub Kicinski wrote:
> Remove .cap_rss_ctx_supported from drivers which moved to the new API.
> This makes it easy to grep for drivers which still need to be converted.

Reviewed-by: Gal Pressman <gal@nvidia.com>

Re: [PATCH net-next v2 05/12] eth: remove .cap_rss_ctx_supported from updated drivers

[Edward Cree]

On 03/08/2024 05:26, Jakub Kicinski wrote:
> Remove .cap_rss_ctx_supported from drivers which moved to the new API.
> This makes it easy to grep for drivers which still need to be converted.
> 
> Reviewed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

[PATCH net-next v2 06/12] ethtool: rss: don't report key if device doesn't support it

[Jakub Kicinski]

marvell/otx2 and mvpp2 do not support setting different
keys for different RSS contexts. Contexts have separate
indirection tables but key is shared with all other contexts.
This is likely fine, indirection table is the most important
piece.

Don't report the key-related parameters from such drivers.
This prevents driver-errors, e.g. otx2 always writes
the main key, even when user asks to change per-context key.
The second reason is that without this change tracking
the keys by the core gets complicated. Even if the driver
correctly reject setting key with rss_context != 0,
change of the main key would have to be reflected in
the XArray for all additional contexts.

Since the additional contexts don't have their own keys
not including the attributes (in Netlink speak) seems
intuitive. ethtool CLI seems to deal with it just fine.

Reviewed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  1 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c  |  1 +
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  1 +
 drivers/net/ethernet/sfc/ef100_ethtool.c      |  1 +
 drivers/net/ethernet/sfc/ethtool.c            |  1 +
 drivers/net/ethernet/sfc/siena/ethtool.c      |  1 +
 include/linux/ethtool.h                       |  3 +++
 net/ethtool/ioctl.c                           | 25 ++++++++++++++++---
 net/ethtool/rss.c                             | 21 +++++++++++-----
 9 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 33e8cf0a3764..77621ccfff5e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -5289,6 +5289,7 @@ void bnxt_ethtool_free(struct bnxt *bp)
 
 const struct ethtool_ops bnxt_ethtool_ops = {
 	.cap_link_lanes_supported	= 1,
+	.rxfh_per_ctx_key		= 1,
 	.rxfh_max_context_id		= BNXT_MAX_ETH_RSS_CTX,
 	.rxfh_indir_space		= BNXT_MAX_RSS_TABLE_ENTRIES_P5,
 	.rxfh_priv_size			= sizeof(struct bnxt_rss_ctx),
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 8c990c976132..b5b57926cdc0 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -4725,6 +4725,7 @@ static const struct ethtool_ops ice_ethtool_ops = {
 				     ETHTOOL_COALESCE_USE_ADAPTIVE |
 				     ETHTOOL_COALESCE_RX_USECS_HIGH,
 	.cap_rss_sym_xor_supported = true,
+	.rxfh_per_ctx_key	= true,
 	.get_link_ksettings	= ice_get_link_ksettings,
 	.set_link_ksettings	= ice_set_link_ksettings,
 	.get_fec_stats		= ice_get_fec_stats,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 0b941482db30..2d514210aaec 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -2593,6 +2593,7 @@ static void mlx5e_get_ts_stats(struct net_device *netdev,
 
 const struct ethtool_ops mlx5e_ethtool_ops = {
 	.cap_rss_ctx_supported	= true,
+	.rxfh_per_ctx_key	= true,
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE |
diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
index 746b5314acb5..127b9d6ade6f 100644
--- a/drivers/net/ethernet/sfc/ef100_ethtool.c
+++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
@@ -58,6 +58,7 @@ const struct ethtool_ops ef100_ethtool_ops = {
 
 	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
 	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
+	.rxfh_per_ctx_key	= 1,
 	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
 	.get_rxfh		= efx_ethtool_get_rxfh,
 	.set_rxfh		= efx_ethtool_set_rxfh,
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 15245720c949..e4d86123b797 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -267,6 +267,7 @@ const struct ethtool_ops efx_ethtool_ops = {
 	.set_rxnfc		= efx_ethtool_set_rxnfc,
 	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
 	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
+	.rxfh_per_ctx_key	= 1,
 	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
 	.get_rxfh		= efx_ethtool_get_rxfh,
 	.set_rxfh		= efx_ethtool_set_rxfh,
diff --git a/drivers/net/ethernet/sfc/siena/ethtool.c b/drivers/net/ethernet/sfc/siena/ethtool.c
index 4c182d4edfc2..6d4e5101433a 100644
--- a/drivers/net/ethernet/sfc/siena/ethtool.c
+++ b/drivers/net/ethernet/sfc/siena/ethtool.c
@@ -241,6 +241,7 @@ static int efx_ethtool_get_ts_info(struct net_device *net_dev,
 
 const struct ethtool_ops efx_siena_ethtool_ops = {
 	.cap_rss_ctx_supported	= true,
+	.rxfh_per_ctx_key	= true,
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_USECS_IRQ |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 55c9f613ab64..16f72a556fe9 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -731,6 +731,8 @@ struct kernel_ethtool_ts_info {
  *	do not have to set this bit.
  * @cap_rss_sym_xor_supported: indicates if the driver supports symmetric-xor
  *	RSS.
+ * @rxfh_per_ctx_key: device supports setting different RSS key for each
+ *	additional context.
  * @rxfh_indir_space: max size of RSS indirection tables, if indirection table
  *	size as returned by @get_rxfh_indir_size may change during lifetime
  *	of the device. Leave as 0 if the table size is constant.
@@ -952,6 +954,7 @@ struct ethtool_ops {
 	u32     cap_link_lanes_supported:1;
 	u32     cap_rss_ctx_supported:1;
 	u32	cap_rss_sym_xor_supported:1;
+	u32	rxfh_per_ctx_key:1;
 	u32	rxfh_indir_space;
 	u16	rxfh_key_space;
 	u16	rxfh_priv_size;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 52dfb07393a6..e32b791f8d1c 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1261,10 +1261,15 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
 		if (rxfh_dev.indir)
 			memcpy(rxfh_dev.indir, ethtool_rxfh_context_indir(ctx),
 			       indir_bytes);
-		if (rxfh_dev.key)
-			memcpy(rxfh_dev.key, ethtool_rxfh_context_key(ctx),
-			       user_key_size);
-		rxfh_dev.hfunc = ctx->hfunc;
+		if (!ops->rxfh_per_ctx_key) {
+			rxfh_dev.key_size = 0;
+		} else {
+			if (rxfh_dev.key)
+				memcpy(rxfh_dev.key,
+				       ethtool_rxfh_context_key(ctx),
+				       user_key_size);
+			rxfh_dev.hfunc = ctx->hfunc;
+		}
 		rxfh_dev.input_xfrm = ctx->input_xfrm;
 		ret = 0;
 	} else {
@@ -1281,6 +1286,11 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
 				&rxfh_dev.input_xfrm,
 				sizeof(rxfh.input_xfrm))) {
 		ret = -EFAULT;
+	} else if (copy_to_user(useraddr +
+				offsetof(struct ethtool_rxfh, key_size),
+				&rxfh_dev.key_size,
+				sizeof(rxfh.key_size))) {
+		ret = -EFAULT;
 	} else if (copy_to_user(useraddr +
 			      offsetof(struct ethtool_rxfh, rss_config[0]),
 			      rss_config, total_size)) {
@@ -1386,6 +1396,13 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 
 	indir_bytes = dev_indir_size * sizeof(rxfh_dev.indir[0]);
 
+	/* Check settings which may be global rather than per RSS-context */
+	if (rxfh.rss_context && !ops->rxfh_per_ctx_key)
+		if (rxfh.key_size ||
+		    (rxfh.hfunc && rxfh.hfunc != ETH_RSS_HASH_NO_CHANGE) ||
+		    (rxfh.input_xfrm && rxfh.input_xfrm != RXH_XFRM_NO_CHANGE))
+			return -EOPNOTSUPP;
+
 	rss_config = kzalloc(indir_bytes + dev_key_size, GFP_USER);
 	if (!rss_config)
 		return -ENOMEM;
diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index a06bdac8b8a2..cd8100d81919 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -10,6 +10,7 @@ struct rss_req_info {
 
 struct rss_reply_data {
 	struct ethnl_reply_data		base;
+	bool				no_key_fields;
 	u32				indir_size;
 	u32				hkey_size;
 	u32				hfunc;
@@ -60,9 +61,12 @@ rss_prepare_data(const struct ethnl_req_info *req_base,
 		return -EOPNOTSUPP;
 
 	/* Some drivers don't handle rss_context */
-	if (request->rss_context && !(ops->cap_rss_ctx_supported ||
-				      ops->create_rxfh_context))
-		return -EOPNOTSUPP;
+	if (request->rss_context) {
+		if (!ops->cap_rss_ctx_supported && !ops->create_rxfh_context)
+			return -EOPNOTSUPP;
+
+		data->no_key_fields = !ops->rxfh_per_ctx_key;
+	}
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
@@ -132,13 +136,18 @@ rss_fill_reply(struct sk_buff *skb, const struct ethnl_req_info *req_base,
 	    nla_put_u32(skb, ETHTOOL_A_RSS_CONTEXT, request->rss_context))
 		return -EMSGSIZE;
 
+	if ((data->indir_size &&
+	     nla_put(skb, ETHTOOL_A_RSS_INDIR,
+		     sizeof(u32) * data->indir_size, data->indir_table)))
+		return -EMSGSIZE;
+
+	if (data->no_key_fields)
+		return 0;
+
 	if ((data->hfunc &&
 	     nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc)) ||
 	    (data->input_xfrm &&
 	     nla_put_u32(skb, ETHTOOL_A_RSS_INPUT_XFRM, data->input_xfrm)) ||
-	    (data->indir_size &&
-	     nla_put(skb, ETHTOOL_A_RSS_INDIR,
-		     sizeof(u32) * data->indir_size, data->indir_table)) ||
 	    (data->hkey_size &&
 	     nla_put(skb, ETHTOOL_A_RSS_HKEY, data->hkey_size, data->hkey)))
 		return -EMSGSIZE;
-- 
2.45.2

Re: [PATCH net-next v2 06/12] ethtool: rss: don't report key if device doesn't support it

[Edward Cree]

On 03/08/2024 05:26, Jakub Kicinski wrote:
> marvell/otx2 and mvpp2 do not support setting different
> keys for different RSS contexts. Contexts have separate
> indirection tables but key is shared with all other contexts.
> This is likely fine, indirection table is the most important
> piece.

Since drivers that do not support this are the odd ones out,
 would it be better to invert the sense of the flag?  Or is
 this to make sure that driver authors who don't think/know
 about the distinction automatically get safe behaviour?

> Don't report the key-related parameters from such drivers.
> This prevents driver-errors, e.g. otx2 always writes
> the main key, even when user asks to change per-context key.
> The second reason is that without this change tracking
> the keys by the core gets complicated. Even if the driver
> correctly reject setting key with rss_context != 0,
> change of the main key would have to be reflected in
> the XArray for all additional contexts.
> 
> Since the additional contexts don't have their own keys
> not including the attributes (in Netlink speak) seems
> intuitive. ethtool CLI seems to deal with it just fine.
> 
> Reviewed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
...
> diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
> index 746b5314acb5..127b9d6ade6f 100644
> --- a/drivers/net/ethernet/sfc/ef100_ethtool.c
> +++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
> @@ -58,6 +58,7 @@ const struct ethtool_ops ef100_ethtool_ops = {
>  
>  	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
>  	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
> +	.rxfh_per_ctx_key	= 1,

I would prefer 'true' for the sfc drivers, I think that
 better fits the general style of our code.

>  	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
>  	.get_rxfh		= efx_ethtool_get_rxfh,
>  	.set_rxfh		= efx_ethtool_set_rxfh,
> diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
> index 15245720c949..e4d86123b797 100644
> --- a/drivers/net/ethernet/sfc/ethtool.c
> +++ b/drivers/net/ethernet/sfc/ethtool.c
> @@ -267,6 +267,7 @@ const struct ethtool_ops efx_ethtool_ops = {
>  	.set_rxnfc		= efx_ethtool_set_rxnfc,
>  	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
>  	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
> +	.rxfh_per_ctx_key	= 1,
>  	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
>  	.get_rxfh		= efx_ethtool_get_rxfh,
>  	.set_rxfh		= efx_ethtool_set_rxfh,
> diff --git a/drivers/net/ethernet/sfc/siena/ethtool.c b/drivers/net/ethernet/sfc/siena/ethtool.c
> index 4c182d4edfc2..6d4e5101433a 100644
> --- a/drivers/net/ethernet/sfc/siena/ethtool.c
> +++ b/drivers/net/ethernet/sfc/siena/ethtool.c
> @@ -241,6 +241,7 @@ static int efx_ethtool_get_ts_info(struct net_device *net_dev,
>  
>  const struct ethtool_ops efx_siena_ethtool_ops = {
>  	.cap_rss_ctx_supported	= true,
> +	.rxfh_per_ctx_key	= true,

For the record, Siena hardware doesn't actually support
 custom RSS contexts; the code is only present in the
 driver as a holdover from when Siena and EF10 used the
 same driver.  Trying to actually use them on Siena will
 fail -EOPNOTSUPP.[1]
I'll send a patch to rip it out.

>  	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>  				     ETHTOOL_COALESCE_USECS_IRQ |
>  				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 55c9f613ab64..16f72a556fe9 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -731,6 +731,8 @@ struct kernel_ethtool_ts_info {
>   *	do not have to set this bit.
>   * @cap_rss_sym_xor_supported: indicates if the driver supports symmetric-xor
>   *	RSS.
> + * @rxfh_per_ctx_key: device supports setting different RSS key for each
> + *	additional context.

This comment should really make clear that it covers hfunc and
 input_xfrm as well, not just the key itself.

-ed

[1]: https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/ethernet/sfc/siena/ethtool_common.c#L1234

Re: [PATCH net-next v2 06/12] ethtool: rss: don't report key if device doesn't support it

[Jakub Kicinski]

[found this stuck in my outgoing mail :S]

On Mon, 5 Aug 2024 15:36:28 +0100 Edward Cree wrote:
> On 03/08/2024 05:26, Jakub Kicinski wrote:
> > marvell/otx2 and mvpp2 do not support setting different
> > keys for different RSS contexts. Contexts have separate
> > indirection tables but key is shared with all other contexts.
> > This is likely fine, indirection table is the most important
> > piece.  
> 
> Since drivers that do not support this are the odd ones out,
>  would it be better to invert the sense of the flag?  Or is
>  this to make sure that driver authors who don't think/know
>  about the distinction automatically get safe behaviour?

Yes, I wanted the 0 / default / sloppy choice to be the safe one.
As annoying as it is to have to set it in most drivers, I still
prefer that to the inevitable false-negatives.

> > Don't report the key-related parameters from such drivers.
> > This prevents driver-errors, e.g. otx2 always writes
> > the main key, even when user asks to change per-context key.
> > The second reason is that without this change tracking
> > the keys by the core gets complicated. Even if the driver
> > correctly reject setting key with rss_context != 0,
> > change of the main key would have to be reflected in
> > the XArray for all additional contexts.
> > 
> > Since the additional contexts don't have their own keys
> > not including the attributes (in Netlink speak) seems
> > intuitive. ethtool CLI seems to deal with it just fine.
> > 
> > Reviewed-by: Joe Damato <jdamato@fastly.com>
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>  
> ...
> > diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
> > index 746b5314acb5..127b9d6ade6f 100644
> > --- a/drivers/net/ethernet/sfc/ef100_ethtool.c
> > +++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
> > @@ -58,6 +58,7 @@ const struct ethtool_ops ef100_ethtool_ops = {
> >  
> >  	.get_rxfh_indir_size	= efx_ethtool_get_rxfh_indir_size,
> >  	.get_rxfh_key_size	= efx_ethtool_get_rxfh_key_size,
> > +	.rxfh_per_ctx_key	= 1,  
> 
> I would prefer 'true' for the sfc drivers, I think that
>  better fits the general style of our code.

Sure thing.

> >  	.rxfh_priv_size		= sizeof(struct efx_rss_context_priv),
> >  	.get_rxfh		= efx_ethtool_get_rxfh,
> >  	.set_rxfh		= efx_ethtool_set_rxfh,

> > diff --git a/drivers/net/ethernet/sfc/siena/ethtool.c b/drivers/net/ethernet/sfc/siena/ethtool.c
> > index 4c182d4edfc2..6d4e5101433a 100644
> > --- a/drivers/net/ethernet/sfc/siena/ethtool.c
> > +++ b/drivers/net/ethernet/sfc/siena/ethtool.c
> > @@ -241,6 +241,7 @@ static int efx_ethtool_get_ts_info(struct net_device *net_dev,
> >  
> >  const struct ethtool_ops efx_siena_ethtool_ops = {
> >  	.cap_rss_ctx_supported	= true,
> > +	.rxfh_per_ctx_key	= true,  
> 
> For the record, Siena hardware doesn't actually support
>  custom RSS contexts; the code is only present in the
>  driver as a holdover from when Siena and EF10 used the
>  same driver.  Trying to actually use them on Siena will
>  fail -EOPNOTSUPP.[1]
> I'll send a patch to rip it out.

Ack, will drop this chunk to avoid conflicts, then.

> >  	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> >  				     ETHTOOL_COALESCE_USECS_IRQ |
> >  				     ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index 55c9f613ab64..16f72a556fe9 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -731,6 +731,8 @@ struct kernel_ethtool_ts_info {
> >   *	do not have to set this bit.
> >   * @cap_rss_sym_xor_supported: indicates if the driver supports symmetric-xor
> >   *	RSS.
> > + * @rxfh_per_ctx_key: device supports setting different RSS key for each
> > + *	additional context.  
> 
> This comment should really make clear that it covers hfunc and
>  input_xfrm as well, not just the key itself.

Ack.

[PATCH net-next v2 07/12] ethtool: rss: move the device op invocation out of rss_prepare_data()

[Jakub Kicinski]

Factor calling device ops out of rss_prepare_data().
Next patch will add alternative path using xarray.
No functional changes.

Reviewed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/rss.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index cd8100d81919..5c477cc36251 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -43,13 +43,9 @@ rss_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
 }
 
 static int
-rss_prepare_data(const struct ethnl_req_info *req_base,
-		 struct ethnl_reply_data *reply_base,
-		 const struct genl_info *info)
+rss_prepare_get(const struct rss_req_info *request, struct net_device *dev,
+		struct rss_reply_data *data, const struct genl_info *info)
 {
-	struct rss_reply_data *data = RSS_REPDATA(reply_base);
-	struct rss_req_info *request = RSS_REQINFO(req_base);
-	struct net_device *dev = reply_base->dev;
 	struct ethtool_rxfh_param rxfh = {};
 	const struct ethtool_ops *ops;
 	u32 total_size, indir_bytes;
@@ -57,16 +53,6 @@ rss_prepare_data(const struct ethnl_req_info *req_base,
 	int ret;
 
 	ops = dev->ethtool_ops;
-	if (!ops->get_rxfh)
-		return -EOPNOTSUPP;
-
-	/* Some drivers don't handle rss_context */
-	if (request->rss_context) {
-		if (!ops->cap_rss_ctx_supported && !ops->create_rxfh_context)
-			return -EOPNOTSUPP;
-
-		data->no_key_fields = !ops->rxfh_per_ctx_key;
-	}
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
@@ -109,6 +95,31 @@ rss_prepare_data(const struct ethnl_req_info *req_base,
 	return ret;
 }
 
+static int
+rss_prepare_data(const struct ethnl_req_info *req_base,
+		 struct ethnl_reply_data *reply_base,
+		 const struct genl_info *info)
+{
+	struct rss_reply_data *data = RSS_REPDATA(reply_base);
+	struct rss_req_info *request = RSS_REQINFO(req_base);
+	struct net_device *dev = reply_base->dev;
+	const struct ethtool_ops *ops;
+
+	ops = dev->ethtool_ops;
+	if (!ops->get_rxfh)
+		return -EOPNOTSUPP;
+
+	/* Some drivers don't handle rss_context */
+	if (request->rss_context) {
+		if (!ops->cap_rss_ctx_supported && !ops->create_rxfh_context)
+			return -EOPNOTSUPP;
+
+		data->no_key_fields = !ops->rxfh_per_ctx_key;
+	}
+
+	return rss_prepare_get(request, dev, data, info);
+}
+
 static int
 rss_reply_size(const struct ethnl_req_info *req_base,
 	       const struct ethnl_reply_data *reply_base)
-- 
2.45.2

[PATCH net-next v2 08/12] ethtool: rss: report info about additional contexts from XArray

[Jakub Kicinski]

IOCTL already uses the XArray when reporting info about additional
contexts. Do the same thing in netlink code.

Reviewed-by: Joe Damato <jdamato@fastly.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/rss.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 5c477cc36251..023782ca1230 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -82,7 +82,6 @@ rss_prepare_get(const struct rss_req_info *request, struct net_device *dev,
 	rxfh.indir = data->indir_table;
 	rxfh.key_size = data->hkey_size;
 	rxfh.key = data->hkey;
-	rxfh.rss_context = request->rss_context;
 
 	ret = ops->get_rxfh(dev, &rxfh);
 	if (ret)
@@ -95,6 +94,41 @@ rss_prepare_get(const struct rss_req_info *request, struct net_device *dev,
 	return ret;
 }
 
+static int
+rss_prepare_ctx(const struct rss_req_info *request, struct net_device *dev,
+		struct rss_reply_data *data, const struct genl_info *info)
+{
+	struct ethtool_rxfh_context *ctx;
+	u32 total_size, indir_bytes;
+	u8 *rss_config;
+
+	ctx = xa_load(&dev->ethtool->rss_ctx, request->rss_context);
+	if (!ctx)
+		return -ENOENT;
+
+	data->indir_size = ctx->indir_size;
+	data->hkey_size = ctx->key_size;
+	data->hfunc = ctx->hfunc;
+	data->input_xfrm = ctx->input_xfrm;
+
+	indir_bytes = data->indir_size * sizeof(u32);
+	total_size = indir_bytes + data->hkey_size;
+	rss_config = kzalloc(total_size, GFP_KERNEL);
+	if (!rss_config)
+		return -ENOMEM;
+
+	data->indir_table = (u32 *)rss_config;
+	memcpy(data->indir_table, ethtool_rxfh_context_indir(ctx), indir_bytes);
+
+	if (data->hkey_size) {
+		data->hkey = rss_config + indir_bytes;
+		memcpy(data->hkey, ethtool_rxfh_context_key(ctx),
+		       data->hkey_size);
+	}
+
+	return 0;
+}
+
 static int
 rss_prepare_data(const struct ethnl_req_info *req_base,
 		 struct ethnl_reply_data *reply_base,
@@ -115,6 +149,7 @@ rss_prepare_data(const struct ethnl_req_info *req_base,
 			return -EOPNOTSUPP;
 
 		data->no_key_fields = !ops->rxfh_per_ctx_key;
+		return rss_prepare_ctx(request, dev, data, info);
 	}
 
 	return rss_prepare_get(request, dev, data, info);
-- 
2.45.2

Re: [PATCH net-next v2 08/12] ethtool: rss: report info about additional contexts from XArray

[Edward Cree]

On 03/08/2024 05:26, Jakub Kicinski wrote:
> IOCTL already uses the XArray when reporting info about additional
> contexts. Do the same thing in netlink code.
> 
> Reviewed-by: Joe Damato <jdamato@fastly.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

[PATCH net-next v2 09/12] ethtool: rss: support dumping RSS contexts

[Jakub Kicinski]

Now that we track RSS contexts in the core we can easily dump
them. This is a major introspection improvement, as previously
the only way to find all contexts would be to try all ids
(of which there may be 2^32 - 1).

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/ethtool.yaml |   9 +-
 net/ethtool/netlink.c                    |   2 +
 net/ethtool/netlink.h                    |   2 +
 net/ethtool/rss.c                        | 133 +++++++++++++++++++++++
 4 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index ea21fe135b97..cf69eedae51d 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1749,12 +1749,12 @@ doc: Partial family for Ethtool Netlink.
 
       attribute-set: rss
 
-      do: &rss-get-op
+      do:
         request:
           attributes:
             - header
             - context
-        reply:
+        reply: &rss-reply
           attributes:
             - header
             - context
@@ -1762,6 +1762,11 @@ doc: Partial family for Ethtool Netlink.
             - indir
             - hkey
             - input_xfrm
+      dump:
+        request:
+          attributes:
+            - header
+        reply: *rss-reply
     -
       name: plca-get-cfg
       doc: Get PLCA params.
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index cb1eea00e349..041548e5f5e6 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -1128,6 +1128,8 @@ static const struct genl_ops ethtool_genl_ops[] = {
 	{
 		.cmd	= ETHTOOL_MSG_RSS_GET,
 		.doit	= ethnl_default_doit,
+		.start	= ethnl_rss_dump_start,
+		.dumpit	= ethnl_rss_dumpit,
 		.policy = ethnl_rss_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_rss_get_policy) - 1,
 	},
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 46ec273a87c5..919371383b23 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -464,6 +464,8 @@ int ethnl_tunnel_info_doit(struct sk_buff *skb, struct genl_info *info);
 int ethnl_tunnel_info_start(struct netlink_callback *cb);
 int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info);
+int ethnl_rss_dump_start(struct netlink_callback *cb);
+int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 
 extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 023782ca1230..62e7b6fe605d 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -208,6 +208,139 @@ static void rss_cleanup_data(struct ethnl_reply_data *reply_base)
 	kfree(data->indir_table);
 }
 
+struct rss_nl_dump_ctx {
+	unsigned long		ifindex;
+	unsigned long		ctx_idx;
+
+	unsigned int		one_ifindex;
+};
+
+static struct rss_nl_dump_ctx *rss_dump_ctx(struct netlink_callback *cb)
+{
+	NL_ASSERT_DUMP_CTX_FITS(struct rss_nl_dump_ctx);
+
+	return (struct rss_nl_dump_ctx *)cb->ctx;
+}
+
+int ethnl_rss_dump_start(struct netlink_callback *cb)
+{
+	const struct genl_info *info = genl_info_dump(cb);
+	struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb);
+	struct ethnl_req_info req_info = {};
+	struct nlattr **tb = info->attrs;
+	int ret;
+
+	/* Filtering by context not supported */
+	if (tb[ETHTOOL_A_RSS_CONTEXT]) {
+		NL_SET_BAD_ATTR(info->extack, tb[ETHTOOL_A_RSS_CONTEXT]);
+		return -EINVAL;
+	}
+
+	ret = ethnl_parse_header_dev_get(&req_info,
+					 tb[ETHTOOL_A_RSS_HEADER],
+					 sock_net(cb->skb->sk), cb->extack,
+					 false);
+	if (req_info.dev) {
+		ctx->one_ifindex = req_info.dev->ifindex;
+		ctx->ifindex = ctx->one_ifindex;
+		ethnl_parse_header_dev_put(&req_info);
+		req_info.dev = NULL;
+	}
+
+	return ret;
+}
+
+static int
+rss_dump_one_ctx(struct sk_buff *skb, struct netlink_callback *cb,
+		 struct net_device *dev, u32 rss_context)
+{
+	const struct genl_info *info = genl_info_dump(cb);
+	struct rss_reply_data data = {};
+	struct rss_req_info req = {};
+	void *ehdr;
+	int ret;
+
+	req.rss_context = rss_context;
+
+	ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_RSS_GET_REPLY);
+	if (!ehdr)
+		return -EMSGSIZE;
+
+	ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_RSS_HEADER);
+	if (ret < 0)
+		goto err_cancel;
+
+	if (!rss_context)
+		ret = rss_prepare_get(&req, dev, &data, info);
+	else
+		ret = rss_prepare_ctx(&req, dev, &data, info);
+	if (ret)
+		goto err_cancel;
+
+	ret = rss_fill_reply(skb, &req.base, &data.base);
+	if (ret)
+		goto err_cleanup;
+	genlmsg_end(skb, ehdr);
+
+	rss_cleanup_data(&data.base);
+	return 0;
+
+err_cleanup:
+	rss_cleanup_data(&data.base);
+err_cancel:
+	genlmsg_cancel(skb, ehdr);
+	return ret;
+}
+
+static int
+rss_dump_one_dev(struct sk_buff *skb, struct netlink_callback *cb,
+		 struct net_device *dev)
+{
+	struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb);
+	int ret;
+
+	if (!dev->ethtool_ops->get_rxfh)
+		return 0;
+
+	if (!ctx->ctx_idx) {
+		ret = rss_dump_one_ctx(skb, cb, dev, 0);
+		if (ret)
+			return ret;
+		ctx->ctx_idx++;
+	}
+
+	for (; xa_find(&dev->ethtool->rss_ctx, &ctx->ctx_idx,
+		       ULONG_MAX, XA_PRESENT); ctx->ctx_idx++) {
+		ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx);
+		if (ret)
+			return ret;
+	}
+	ctx->ctx_idx = 0;
+
+	return 0;
+}
+
+int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb);
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev;
+	int ret = 0;
+
+	rtnl_lock();
+	for_each_netdev_dump(net, dev, ctx->ifindex) {
+		if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex)
+			break;
+
+		ret = rss_dump_one_dev(skb, cb, dev);
+		if (ret)
+			break;
+	}
+	rtnl_unlock();
+
+	return ret;
+}
+
 const struct ethnl_request_ops ethnl_rss_request_ops = {
 	.request_cmd		= ETHTOOL_MSG_RSS_GET,
 	.reply_cmd		= ETHTOOL_MSG_RSS_GET_REPLY,
-- 
2.45.2

Re: [PATCH net-next v2 09/12] ethtool: rss: support dumping RSS contexts

[Joe Damato]

On Fri, Aug 02, 2024 at 09:26:21PM -0700, Jakub Kicinski wrote:
> Now that we track RSS contexts in the core we can easily dump
> them. This is a major introspection improvement, as previously
> the only way to find all contexts would be to try all ids
> (of which there may be 2^32 - 1).
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

Thanks for doing this important and extremely useful work. I am
personally very excited to see this data available to userland.

[...]

> diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
> index 023782ca1230..62e7b6fe605d 100644
> --- a/net/ethtool/rss.c
> +++ b/net/ethtool/rss.c
> @@ -208,6 +208,139 @@ static void rss_cleanup_data(struct ethnl_reply_data *reply_base)
>  	kfree(data->indir_table);
>  }
>  
> +struct rss_nl_dump_ctx {
> +	unsigned long		ifindex;
> +	unsigned long		ctx_idx;
> +
> +	unsigned int		one_ifindex;

My apologies: I'm probably just not familiar enough with the code,
but I'm having a hard time understanding what the purpose of
one_ifindex is.

I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still
not following what this is used for; it'll probably be obvious in
retrospect once you explain it, but I suppose my feedback is that a
comment or something would be really helpful :)

> +};
> +
> +static struct rss_nl_dump_ctx *rss_dump_ctx(struct netlink_callback *cb)
> +{
> +	NL_ASSERT_DUMP_CTX_FITS(struct rss_nl_dump_ctx);
> +
> +	return (struct rss_nl_dump_ctx *)cb->ctx;
> +}
> +
> +int ethnl_rss_dump_start(struct netlink_callback *cb)
> +{
> +	const struct genl_info *info = genl_info_dump(cb);
> +	struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb);
> +	struct ethnl_req_info req_info = {};
> +	struct nlattr **tb = info->attrs;
> +	int ret;
> +
> +	/* Filtering by context not supported */
> +	if (tb[ETHTOOL_A_RSS_CONTEXT]) {
> +		NL_SET_BAD_ATTR(info->extack, tb[ETHTOOL_A_RSS_CONTEXT]);
> +		return -EINVAL;
> +	}
> +
> +	ret = ethnl_parse_header_dev_get(&req_info,
> +					 tb[ETHTOOL_A_RSS_HEADER],
> +					 sock_net(cb->skb->sk), cb->extack,
> +					 false);
> +	if (req_info.dev) {
> +		ctx->one_ifindex = req_info.dev->ifindex;
> +		ctx->ifindex = ctx->one_ifindex;
> +		ethnl_parse_header_dev_put(&req_info);
> +		req_info.dev = NULL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int
> +rss_dump_one_ctx(struct sk_buff *skb, struct netlink_callback *cb,
> +		 struct net_device *dev, u32 rss_context)
> +{
> +	const struct genl_info *info = genl_info_dump(cb);
> +	struct rss_reply_data data = {};
> +	struct rss_req_info req = {};
> +	void *ehdr;
> +	int ret;
> +
> +	req.rss_context = rss_context;
> +
> +	ehdr = ethnl_dump_put(skb, cb, ETHTOOL_MSG_RSS_GET_REPLY);
> +	if (!ehdr)
> +		return -EMSGSIZE;
> +
> +	ret = ethnl_fill_reply_header(skb, dev, ETHTOOL_A_RSS_HEADER);
> +	if (ret < 0)
> +		goto err_cancel;
> +
> +	if (!rss_context)
> +		ret = rss_prepare_get(&req, dev, &data, info);
> +	else
> +		ret = rss_prepare_ctx(&req, dev, &data, info);
> +	if (ret)
> +		goto err_cancel;
> +
> +	ret = rss_fill_reply(skb, &req.base, &data.base);
> +	if (ret)
> +		goto err_cleanup;
> +	genlmsg_end(skb, ehdr);
> +
> +	rss_cleanup_data(&data.base);
> +	return 0;
> +
> +err_cleanup:
> +	rss_cleanup_data(&data.base);
> +err_cancel:
> +	genlmsg_cancel(skb, ehdr);
> +	return ret;
> +}
> +
> +static int
> +rss_dump_one_dev(struct sk_buff *skb, struct netlink_callback *cb,
> +		 struct net_device *dev)
> +{
> +	struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb);
> +	int ret;
> +
> +	if (!dev->ethtool_ops->get_rxfh)
> +		return 0;
> +
> +	if (!ctx->ctx_idx) {
> +		ret = rss_dump_one_ctx(skb, cb, dev, 0);
> +		if (ret)
> +			return ret;
> +		ctx->ctx_idx++;
> +	}
> +
> +	for (; xa_find(&dev->ethtool->rss_ctx, &ctx->ctx_idx,
> +		       ULONG_MAX, XA_PRESENT); ctx->ctx_idx++) {
> +		ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx);
> +		if (ret)
> +			return ret;
> +	}
> +	ctx->ctx_idx = 0;
> +
> +	return 0;
> +}
> +
> +int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb);
> +	struct net *net = sock_net(skb->sk);
> +	struct net_device *dev;
> +	int ret = 0;
> +
> +	rtnl_lock();
> +	for_each_netdev_dump(net, dev, ctx->ifindex) {
> +		if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex)
> +			break;
> +
> +		ret = rss_dump_one_dev(skb, cb, dev);
> +		if (ret)
> +			break;
> +	}
> +	rtnl_unlock();
> +
> +	return ret;
> +}
> +
>  const struct ethnl_request_ops ethnl_rss_request_ops = {
>  	.request_cmd		= ETHTOOL_MSG_RSS_GET,
>  	.reply_cmd		= ETHTOOL_MSG_RSS_GET_REPLY,
> -- 
> 2.45.2
> 

Re: [PATCH net-next v2 09/12] ethtool: rss: support dumping RSS contexts

[Jakub Kicinski]

On Sat, 3 Aug 2024 19:11:28 +0100 Joe Damato wrote:
> > +struct rss_nl_dump_ctx {
> > +	unsigned long		ifindex;
> > +	unsigned long		ctx_idx;
> > +
> > +	unsigned int		one_ifindex;  
> 
> My apologies: I'm probably just not familiar enough with the code,
> but I'm having a hard time understanding what the purpose of
> one_ifindex is.
> 
> I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still
> not following what this is used for; it'll probably be obvious in
> retrospect once you explain it, but I suppose my feedback is that a
> comment or something would be really helpful :)

Better name would probably help, but can't think of any.

User can (optionally) pass an ifindex/ifname to the dump, to dump
contexts only for the specified ifindex. If they do we "preset"
the ifindex and one_ifindex:

+	if (req_info.dev) {
+		ctx->one_ifindex = req_info.dev->ifindex;
+		ctx->ifindex = ctx->one_ifindex;
+		ethnl_parse_header_dev_put(&req_info);
+		req_info.dev = NULL;
+	}

and then the iteration is stopped after first full pass:

+	rtnl_lock();
+	for_each_netdev_dump(net, dev, ctx->ifindex) {
+		if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex)
+			break;

Unfortunately we don't have any best practice for handling filtering 
in dumps. I find this cleaner than approaches I previously tried, but
we'll see if it stands the test of time.

I'll add the following comment:

	/* User wants to dump contexts for one ifindex only */

Re: [PATCH net-next v2 09/12] ethtool: rss: support dumping RSS contexts

[Joe Damato]

On Mon, Aug 05, 2024 at 02:59:33PM -0700, Jakub Kicinski wrote:
> On Sat, 3 Aug 2024 19:11:28 +0100 Joe Damato wrote:
> > > +struct rss_nl_dump_ctx {
> > > +	unsigned long		ifindex;
> > > +	unsigned long		ctx_idx;
> > > +
> > > +	unsigned int		one_ifindex;  
> > 
> > My apologies: I'm probably just not familiar enough with the code,
> > but I'm having a hard time understanding what the purpose of
> > one_ifindex is.
> > 
> > I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still
> > not following what this is used for; it'll probably be obvious in
> > retrospect once you explain it, but I suppose my feedback is that a
> > comment or something would be really helpful :)
> 
> Better name would probably help, but can't think of any.
> 
> User can (optionally) pass an ifindex/ifname to the dump, to dump
> contexts only for the specified ifindex. If they do we "preset"
> the ifindex and one_ifindex:
 
> +	if (req_info.dev) {
> +		ctx->one_ifindex = req_info.dev->ifindex;
> +		ctx->ifindex = ctx->one_ifindex;
> +		ethnl_parse_header_dev_put(&req_info);
> +		req_info.dev = NULL;
> +	}
> 
> and then the iteration is stopped after first full pass:
> 
> +	rtnl_lock();
> +	for_each_netdev_dump(net, dev, ctx->ifindex) {
> +		if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex)
> +			break;

Ah, OK; that all makes sense. Thanks for the explanation.

> Unfortunately we don't have any best practice for handling filtering 
> in dumps. I find this cleaner than approaches I previously tried, but
> we'll see if it stands the test of time.
> 
> I'll add the following comment:
> 
> 	/* User wants to dump contexts for one ifindex only */

Sounds good. If you like, you can also add:

Reviewed-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net-next v2 09/12] ethtool: rss: support dumping RSS contexts

[Przemek Kitszel]

On 8/5/24 23:59, Jakub Kicinski wrote:
> On Sat, 3 Aug 2024 19:11:28 +0100 Joe Damato wrote:
>>> +struct rss_nl_dump_ctx {
>>> +	unsigned long		ifindex;
>>> +	unsigned long		ctx_idx;
>>> +
>>> +	unsigned int		one_ifindex;
>>
>> My apologies: I'm probably just not familiar enough with the code,
>> but I'm having a hard time understanding what the purpose of
>> one_ifindex is.
>>
>> I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still
>> not following what this is used for; it'll probably be obvious in
>> retrospect once you explain it, but I suppose my feedback is that a
>> comment or something would be really helpful :)
> 
> Better name would probably help, but can't think of any.

perhaps:
ifindex -> if_iter
one_ifindex -> if_requested

(I would also like 'ifc' instead of 'if', for the obvious reasons)

> 
> User can (optionally) pass an ifindex/ifname to the dump, to dump
> contexts only for the specified ifindex. If they do we "preset"
> the ifindex and one_ifindex:
> 
> +	if (req_info.dev) {
> +		ctx->one_ifindex = req_info.dev->ifindex;
> +		ctx->ifindex = ctx->one_ifindex;
> +		ethnl_parse_header_dev_put(&req_info);
> +		req_info.dev = NULL;
> +	}
> 
> and then the iteration is stopped after first full pass:
> 
> +	rtnl_lock();
> +	for_each_netdev_dump(net, dev, ctx->ifindex) {
> +		if (ctx->one_ifindex && ctx->one_ifindex != ctx->ifindex)
> +			break;
> 
> Unfortunately we don't have any best practice for handling filtering
> in dumps. I find this cleaner than approaches I previously tried, but
> we'll see if it stands the test of time.

This code is clean, but by just looking at the struct one could not
expect it though :/ Perhaps a rename could help, or just wait until
people learn it (I remember you have recently explained this dump
scheme to me :))

> 
> I'll add the following comment:
> 
> 	/* User wants to dump contexts for one ifindex only */

Re: [PATCH net-next v2 09/12] ethtool: rss: support dumping RSS contexts

[Edward Cree]

On 05/08/2024 22:59, Jakub Kicinski wrote:
> On Sat, 3 Aug 2024 19:11:28 +0100 Joe Damato wrote:
>>> +struct rss_nl_dump_ctx {
>>> +	unsigned long		ifindex;
>>> +	unsigned long		ctx_idx;
>>> +
>>> +	unsigned int		one_ifindex;  
>>
>> My apologies: I'm probably just not familiar enough with the code,
>> but I'm having a hard time understanding what the purpose of
>> one_ifindex is.
>>
>> I read both ethnl_rss_dump_start and ethnl_rss_dumpit, but I'm still
>> not following what this is used for; it'll probably be obvious in
>> retrospect once you explain it, but I suppose my feedback is that a
>> comment or something would be really helpful :)
> 
> Better name would probably help, but can't think of any.

'only_ifindex'?  'match_ifindex'?

Re: [PATCH net-next v2 09/12] ethtool: rss: support dumping RSS contexts

[Jakub Kicinski]

On Tue, 6 Aug 2024 14:58:59 +0100 Edward Cree wrote:
> > Better name would probably help, but can't think of any.  
> 
> 'only_ifindex'?  'match_ifindex'?

'match' sounds good, I'll use that, thanks!

Re: [PATCH net-next v2 09/12] ethtool: rss: support dumping RSS contexts

[Edward Cree]

On 03/08/2024 05:26, Jakub Kicinski wrote:
> Now that we track RSS contexts in the core we can easily dump
> them. This is a major introspection improvement, as previously
> the only way to find all contexts would be to try all ids
> (of which there may be 2^32 - 1).
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
...
> +	if (!ctx->ctx_idx) {
> +		ret = rss_dump_one_ctx(skb, cb, dev, 0);
> +		if (ret)
> +			return ret;
> +		ctx->ctx_idx++;
> +	}

Maybe comment this block with something like "context 0 is
 not stored in the XArray" to make clear why this is split
 out from a loop that looks like it should be able to handle
 it.

> +
> +	for (; xa_find(&dev->ethtool->rss_ctx, &ctx->ctx_idx,
> +		       ULONG_MAX, XA_PRESENT); ctx->ctx_idx++) {
> +		ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx);
> +		if (ret)
> +			return ret;
> +	}
> +	ctx->ctx_idx = 0;

Feels like there has to be a way to do this with
 xa_for_each_start()?  Something like (untested):

	struct ethtool_rxfh_context *rss_ctx;

	xa_for_each_start(&dev->ethtool->rss_ctx, ctx->ctx_idx,
			  rss_ctx, ctx->ctx_idx) {
		ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx);
		if (ret)
			return ret;
	}
	ctx->ctx_idx = 0;

Re: [PATCH net-next v2 09/12] ethtool: rss: support dumping RSS contexts

[Jakub Kicinski]

On Tue, 6 Aug 2024 15:24:50 +0100 Edward Cree wrote:
> > +	if (!ctx->ctx_idx) {
> > +		ret = rss_dump_one_ctx(skb, cb, dev, 0);
> > +		if (ret)
> > +			return ret;
> > +		ctx->ctx_idx++;
> > +	}  
> 
> Maybe comment this block with something like "context 0 is
>  not stored in the XArray" to make clear why this is split
>  out from a loop that looks like it should be able to handle
>  it.

Will do.

> > +
> > +	for (; xa_find(&dev->ethtool->rss_ctx, &ctx->ctx_idx,
> > +		       ULONG_MAX, XA_PRESENT); ctx->ctx_idx++) {
> > +		ret = rss_dump_one_ctx(skb, cb, dev, ctx->ctx_idx);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	ctx->ctx_idx = 0;  
> 
> Feels like there has to be a way to do this with
>  xa_for_each_start()?  Something like (untested):

It may work in the current code but I prefer to stay away from xarray
iterators in netlink code. They cause too many bugs. They do not
invalidate / move the index past the end of the array once they are
done. Which means if the dump ever gets called again after finishing
we'll re-dump the last element.

[PATCH net-next v2 10/12] ethtool: rss: support skipping contexts during dump

[Jakub Kicinski]

Applications may want to deal with dynamic RSS contexts only.
So dumping context 0 will be counter-productive for them.
Support starting the dump from a given context ID.

Alternative would be to implement a dump flag to skip just
context 0, not sure which is better...

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/ethtool.yaml     |  4 ++++
 Documentation/networking/ethtool-netlink.rst | 12 ++++++++++--
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/netlink.h                        |  2 +-
 net/ethtool/rss.c                            | 12 +++++++++++-
 5 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index cf69eedae51d..4c2334c213b0 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1028,6 +1028,9 @@ doc: Partial family for Ethtool Netlink.
       -
         name: input_xfrm
         type: u32
+      -
+        name: start-context
+        type: u32
   -
     name: plca
     attributes:
@@ -1766,6 +1769,7 @@ doc: Partial family for Ethtool Netlink.
         request:
           attributes:
             - header
+            - start-context
         reply: *rss-reply
     -
       name: plca-get-cfg
diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index d5f246aceb9f..82c5542c80ce 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1866,10 +1866,18 @@ RSS context of an interface similar to ``ETHTOOL_GRSSH`` ioctl request.
 
 Request contents:
 
-=====================================  ======  ==========================
+=====================================  ======  ============================
   ``ETHTOOL_A_RSS_HEADER``             nested  request header
   ``ETHTOOL_A_RSS_CONTEXT``            u32     context number
-=====================================  ======  ==========================
+  ``ETHTOOL_A_RSS_START_CONTEXT``      u32     start context number (dumps)
+=====================================  ======  ============================
+
+``ETHTOOL_A_RSS_CONTEXT`` specifies which RSS context number to query,
+if not set context 0 (the main context) is queried. Dumps can be filtered
+by device (only listing contexts of a given netdev). Filtering single
+context number is not supported but ``ETHTOOL_A_RSS_START_CONTEXT``
+can be used to start dumping context from the given number (primarily
+used to ignore context 0s and only dump additional contexts).
 
 Kernel response contents:
 
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 6d5bdcc67631..93c57525a975 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -965,6 +965,7 @@ enum {
 	ETHTOOL_A_RSS_INDIR,		/* binary */
 	ETHTOOL_A_RSS_HKEY,		/* binary */
 	ETHTOOL_A_RSS_INPUT_XFRM,	/* u32 */
+	ETHTOOL_A_RSS_START_CONTEXT,	/* u32 */
 
 	__ETHTOOL_A_RSS_CNT,
 	ETHTOOL_A_RSS_MAX = (__ETHTOOL_A_RSS_CNT - 1),
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 919371383b23..236c189fc968 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -449,7 +449,7 @@ extern const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER +
 extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1];
 extern const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1];
 extern const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1];
-extern const struct nla_policy ethnl_rss_get_policy[ETHTOOL_A_RSS_CONTEXT + 1];
+extern const struct nla_policy ethnl_rss_get_policy[ETHTOOL_A_RSS_START_CONTEXT + 1];
 extern const struct nla_policy ethnl_plca_get_cfg_policy[ETHTOOL_A_PLCA_HEADER + 1];
 extern const struct nla_policy ethnl_plca_set_cfg_policy[ETHTOOL_A_PLCA_MAX + 1];
 extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADER + 1];
diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 62e7b6fe605d..659468965de7 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -28,6 +28,7 @@ struct rss_reply_data {
 const struct nla_policy ethnl_rss_get_policy[] = {
 	[ETHTOOL_A_RSS_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy),
 	[ETHTOOL_A_RSS_CONTEXT] = { .type = NLA_U32 },
+	[ETHTOOL_A_RSS_START_CONTEXT] = { .type = NLA_U32 },
 };
 
 static int
@@ -38,6 +39,10 @@ rss_parse_request(struct ethnl_req_info *req_info, struct nlattr **tb,
 
 	if (tb[ETHTOOL_A_RSS_CONTEXT])
 		request->rss_context = nla_get_u32(tb[ETHTOOL_A_RSS_CONTEXT]);
+	if (tb[ETHTOOL_A_RSS_START_CONTEXT]) {
+		NL_SET_BAD_ATTR(extack, tb[ETHTOOL_A_RSS_START_CONTEXT]);
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -213,6 +218,7 @@ struct rss_nl_dump_ctx {
 	unsigned long		ctx_idx;
 
 	unsigned int		one_ifindex;
+	unsigned int		start_ctx;
 };
 
 static struct rss_nl_dump_ctx *rss_dump_ctx(struct netlink_callback *cb)
@@ -235,6 +241,10 @@ int ethnl_rss_dump_start(struct netlink_callback *cb)
 		NL_SET_BAD_ATTR(info->extack, tb[ETHTOOL_A_RSS_CONTEXT]);
 		return -EINVAL;
 	}
+	if (tb[ETHTOOL_A_RSS_START_CONTEXT]) {
+		ctx->start_ctx = nla_get_u32(tb[ETHTOOL_A_RSS_START_CONTEXT]);
+		ctx->ctx_idx = ctx->start_ctx;
+	}
 
 	ret = ethnl_parse_header_dev_get(&req_info,
 					 tb[ETHTOOL_A_RSS_HEADER],
@@ -315,7 +325,7 @@ rss_dump_one_dev(struct sk_buff *skb, struct netlink_callback *cb,
 		if (ret)
 			return ret;
 	}
-	ctx->ctx_idx = 0;
+	ctx->ctx_idx = ctx->start_ctx;
 
 	return 0;
 }
-- 
2.45.2

Re: [PATCH net-next v2 10/12] ethtool: rss: support skipping contexts during dump

[Joe Damato]

On Fri, Aug 02, 2024 at 09:26:22PM -0700, Jakub Kicinski wrote:
> Applications may want to deal with dynamic RSS contexts only.
> So dumping context 0 will be counter-productive for them.
> Support starting the dump from a given context ID.
> 
> Alternative would be to implement a dump flag to skip just
> context 0, not sure which is better...

Neither am I.

While I personally like the idea of skipping an arbitrary number of
contexts when dumping them, I would guess that the vast majority of
use cases (maybe all?) will be to skip context 0.

Re: [PATCH net-next v2 10/12] ethtool: rss: support skipping contexts during dump

[Edward Cree]

On 03/08/2024 05:26, Jakub Kicinski wrote:
> Applications may want to deal with dynamic RSS contexts only.
> So dumping context 0 will be counter-productive for them.
> Support starting the dump from a given context ID.
> 
> Alternative would be to implement a dump flag to skip just
> context 0, not sure which is better...
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

Start ID feels more elegant than skip-0 flag to me.

[PATCH net-next v2 11/12] netlink: specs: decode indirection table as u32 array

[Jakub Kicinski]

Indirection table is dumped as a raw u32 array, decode it.
It's tempting to decode hash key, too, but it is an actual
bitstream, so leave it be for now.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/ethtool.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 4c2334c213b0..1bbeaba5c644 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1022,6 +1022,7 @@ doc: Partial family for Ethtool Netlink.
       -
         name: indir
         type: binary
+        sub-type: u32
       -
         name: hkey
         type: binary
-- 
2.45.2

Re: [PATCH net-next v2 11/12] netlink: specs: decode indirection table as u32 array

[Joe Damato]

On Fri, Aug 02, 2024 at 09:26:23PM -0700, Jakub Kicinski wrote:
> Indirection table is dumped as a raw u32 array, decode it.
> It's tempting to decode hash key, too, but it is an actual
> bitstream, so leave it be for now.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  Documentation/netlink/specs/ethtool.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index 4c2334c213b0..1bbeaba5c644 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -1022,6 +1022,7 @@ doc: Partial family for Ethtool Netlink.
>        -
>          name: indir
>          type: binary
> +        sub-type: u32
>        -
>          name: hkey
>          type: binary
> -- 
> 2.45.2
> 

Reviewed-by: Joe Damato <jdamato@fastly.com>

[PATCH net-next v2 12/12] selftests: drv-net: rss_ctx: test dumping RSS contexts

[Jakub Kicinski]

Add a test for dumping RSS contexts. Make sure indir table
and key are sane when contexts are created with various
combination of inputs. Test the dump filtering by ifname
and start-context.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../selftests/drivers/net/hw/rss_ctx.py       | 70 ++++++++++++++++++-
 tools/testing/selftests/net/lib/py/ksft.py    |  6 ++
 2 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
index 1da6b214f4fe..cbff3061abd7 100755
--- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
+++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
@@ -3,7 +3,7 @@
 
 import datetime
 import random
-from lib.py import ksft_run, ksft_pr, ksft_exit, ksft_eq, ksft_ge, ksft_lt
+from lib.py import ksft_run, ksft_pr, ksft_exit, ksft_eq, ksft_ne, ksft_ge, ksft_lt
 from lib.py import NetDrvEpEnv
 from lib.py import EthtoolFamily, NetdevFamily
 from lib.py import KsftSkipEx
@@ -302,6 +302,72 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
     ksft_eq(carrier1 - carrier0, 0)
 
 
+def test_rss_context_dump(cfg):
+    """
+    Test dumping RSS contexts. This tests mostly exercises the kernel APIs.
+    """
+
+    # Get a random key of the right size
+    data = get_rss(cfg)
+    if 'rss-hash-key' in data:
+        key_data = _rss_key_rand(len(data['rss-hash-key']))
+        key = _rss_key_str(key_data)
+    else:
+        key_data = []
+        key = "ba:ad"
+
+    ids = []
+    try:
+        ids.append(ethtool_create(cfg, "-X", f"context new"))
+        defer(ethtool, f"-X {cfg.ifname} context {ids[-1]} delete")
+
+        ids.append(ethtool_create(cfg, "-X", f"context new weight 1 1"))
+        defer(ethtool, f"-X {cfg.ifname} context {ids[-1]} delete")
+
+        ids.append(ethtool_create(cfg, "-X", f"context new hkey {key}"))
+        defer(ethtool, f"-X {cfg.ifname} context {ids[-1]} delete")
+    except CmdExitFailure:
+        if not ids:
+            raise KsftSkipEx("Unable to add any contexts")
+        ksft_pr(f"Added only {len(ids)} out of 3 contexts")
+
+    expect_tuples = set([(cfg.ifname, -1)] + [(cfg.ifname, ctx_id) for ctx_id in ids])
+
+    # Dump all
+    ctxs = cfg.ethnl.rss_get({}, dump=True)
+    ctx_tuples = set([(c['header']['dev-name'], c.get('context', -1)) for c in ctxs])
+    ksft_eq(expect_tuples, ctx_tuples)
+
+    # Sanity-check the results
+    for data in ctxs:
+        ksft_ne(set(data['indir']), {0}, "indir table is all zero")
+        ksft_ne(set(data.get('hkey', [1])), {0}, "key is all zero")
+
+        # More specific checks
+        if len(ids) > 1 and data.get('context') == ids[1]:
+            ksft_eq(set(data['indir']), {0, 1},
+                    "ctx1 - indir table mismatch")
+        if len(ids) > 2 and data.get('context') == ids[2]:
+            ksft_eq(data['hkey'], bytes(key_data), "ctx2 - key mismatch")
+
+    # Ifindex filter
+    ctxs = cfg.ethnl.rss_get({'header': {'dev-name': cfg.ifname}}, dump=True)
+    ctx_tuples = set([(c['header']['dev-name'], c.get('context', -1)) for c in ctxs])
+    ksft_eq(expect_tuples, ctx_tuples)
+
+    # Skip ctx 0
+    expect_tuples.remove((cfg.ifname, -1))
+
+    ctxs = cfg.ethnl.rss_get({'start-context': 1}, dump=True)
+    ctx_tuples = set([(c['header']['dev-name'], c.get('context', -1)) for c in ctxs])
+    ksft_eq(expect_tuples, ctx_tuples)
+
+    # And finally both with ifindex and skip main
+    ctxs = cfg.ethnl.rss_get({'header': {'dev-name': cfg.ifname}, 'start-context': 1}, dump=True)
+    ctx_tuples = set([(c['header']['dev-name'], c.get('context', -1)) for c in ctxs])
+    ksft_eq(expect_tuples, ctx_tuples)
+
+
 def test_rss_context(cfg, ctx_cnt=1, create_with_cfg=None):
     """
     Test separating traffic into RSS contexts.
@@ -542,7 +608,7 @@ from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
         ksft_run([test_rss_key_indir, test_rss_queue_reconfigure,
                   test_rss_resize, test_hitless_key_update,
                   test_rss_context, test_rss_context4, test_rss_context32,
-                  test_rss_context_queue_reconfigure,
+                  test_rss_context_dump, test_rss_context_queue_reconfigure,
                   test_rss_context_overlap, test_rss_context_overlap2,
                   test_rss_context_out_of_order, test_rss_context4_create_with_cfg],
                  args=(cfg, ))
diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
index f663e0daec0d..477ae76de93d 100644
--- a/tools/testing/selftests/net/lib/py/ksft.py
+++ b/tools/testing/selftests/net/lib/py/ksft.py
@@ -55,6 +55,12 @@ KSFT_DISRUPTIVE = True
         _fail("Check failed", a, "!=", b, comment)
 
 
+def ksft_ne(a, b, comment=""):
+    global KSFT_RESULT
+    if a == b:
+        _fail("Check failed", a, "==", b, comment)
+
+
 def ksft_true(a, comment=""):
     if not a:
         _fail("Check failed", a, "does not eval to True", comment)
-- 
2.45.2

Re: [PATCH net-next v2 12/12] selftests: drv-net: rss_ctx: test dumping RSS contexts

[Joe Damato]

On Fri, Aug 02, 2024 at 09:26:24PM -0700, Jakub Kicinski wrote:
> Add a test for dumping RSS contexts. Make sure indir table
> and key are sane when contexts are created with various
> combination of inputs. Test the dump filtering by ifname
> and start-context.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  .../selftests/drivers/net/hw/rss_ctx.py       | 70 ++++++++++++++++++-
>  tools/testing/selftests/net/lib/py/ksft.py    |  6 ++
>  2 files changed, 74 insertions(+), 2 deletions(-)

Reviewed-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net-next v2 12/12] selftests: drv-net: rss_ctx: test dumping RSS contexts

[Edward Cree]

On 03/08/2024 05:26, Jakub Kicinski wrote:
> Add a test for dumping RSS contexts. Make sure indir table
> and key are sane when contexts are created with various
> combination of inputs. Test the dump filtering by ifname
> and start-context.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
...
> +    expect_tuples = set([(cfg.ifname, -1)] + [(cfg.ifname, ctx_id) for ctx_id in ids])
> +
> +    # Dump all
> +    ctxs = cfg.ethnl.rss_get({}, dump=True)
> +    ctx_tuples = set([(c['header']['dev-name'], c.get('context', -1)) for c in ctxs])

Won't this return all ctxes on all netdevs in the system?

> +    ksft_eq(expect_tuples, ctx_tuples)

Whereas expect_tuples only contains cfg.ifname, so this
 assertion will fail if you have more than one RSS-
 supporting netdev.

Re: [PATCH net-next v2 12/12] selftests: drv-net: rss_ctx: test dumping RSS contexts

[Jakub Kicinski]

On Tue, 6 Aug 2024 17:48:27 +0100 Edward Cree wrote:
> > +    expect_tuples = set([(cfg.ifname, -1)] + [(cfg.ifname, ctx_id) for ctx_id in ids])
> > +
> > +    # Dump all
> > +    ctxs = cfg.ethnl.rss_get({}, dump=True)
> > +    ctx_tuples = set([(c['header']['dev-name'], c.get('context', -1)) for c in ctxs])  
> 
> Won't this return all ctxes on all netdevs in the system?
> 
> > +    ksft_eq(expect_tuples, ctx_tuples)  
> 
> Whereas expect_tuples only contains cfg.ifname, so this
>  assertion will fail if you have more than one RSS-
>  supporting netdev.

And RSS contexts are actively used on another interface, yes.
Will fix. More importantly we should check that there are no
duplicates.

Re: [PATCH net-next v2 00/12] ethtool: rss: driver tweaks and netlink context dumps

[Gal Pressman]

On 03/08/2024 7:26, Jakub Kicinski wrote:
> This series is a semi-related collection of RSS patches.
> Main point is supporting dumping RSS contexts via ethtool netlink.
> At present additional RSS contexts can be queried one by one, and
> assuming user know the right IDs. This series uses the XArray
> added by Ed to provide netlink dump support for ETHTOOL_GET_RSS.
> 
> Patch 1 is a trivial selftest debug patch.
> Patch 2 coverts mvpp2 for no real reason other than that I had
> 	a grand plan of converting all drivers at some stage.
> Patch 3 removes a now moot check from mlx5 so that all tests
> 	can pass.
> Patch 4 and 5 make a bit used for context support optional,
> 	for easier grepping of drivers which need converting
> 	if nothing else.
> Patch 6 OTOH adds a new cap bit; some devices don't support
> 	using a different key per context and currently act
> 	in surprising ways.
> Patch 7 and 8 update the RSS netlink code to use XArray.
> Patch 9 and 10 add support for dumping contexts.
> Patch 11 and 12 are small adjustments to spec and a new test.

Very useful, I was messing around with the RSS code lately and was
thinking about these stuff too, thanks!

> 
> 
> I'm getting distracted with other work, so probably won't have
> the time soon to complete next steps, but things which are missing
> are (and some of these may be bad ideas):
> 
>  - better discovery
> 
>    Some sort of API to tell the user who many contexts the device
>    can create. Upper bound, devices often share contexts between
>    ports etc. so it's hard to tell exactly and upfront number of
>    contexts for a netdev. But order of magnitude (4 vs 10s) may
>    be enough for container management system to know whether to bother.
> 
>  - create/modify/delete via netlink

And actually plugging extack into set_rxfh :).

>  
>    The only question here is how to handle all the tricky IOCTL
>    legacy. "No change" maps trivially to attribute not present.
>    "reset" (indir_size = 0) probably needs to be a new NLA_FLAG?

FWIW, we have an incompatibility issue with the recent rxfh.input_xfrm
parameter.

In ethtool_set_rxfh():
	/* If either indir, hash key or function is valid, proceed further.
	 * Must request at least one change: indir size, hash key, function
	 * or input transformation.
	 */
	if ((rxfh.indir_size &&
	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
	     rxfh.indir_size != dev_indir_size) ||
	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
	     rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE &&
	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))
		return -EINVAL;

When using a recent kernel with an old userspace ethtool,
rxfh.input_xfrm is treated as zero (which is different than
RXH_XFRM_NO_CHANGE) and passes the check, whereas the same command with
a recent userspace would result in an error.
This also makes it so old userspace always disables input_xfrm
unintentionally. I do not have any ideas on how to resolve this..

Regardless, I believe this check is wrong as it prevents us from
creating RSS context with no parameters (i.e. 'ethtool -X eth0 context
new', as done in selftests), it works by mistake with old userspace.
I plan to submit a patch soon to skip this check in case of context
creation.

Re: [PATCH net-next v2 00/12] ethtool: rss: driver tweaks and netlink context dumps

[Jakub Kicinski]

On Sun, 4 Aug 2024 09:08:50 +0300 Gal Pressman wrote:
> >    The only question here is how to handle all the tricky IOCTL
> >    legacy. "No change" maps trivially to attribute not present.
> >    "reset" (indir_size = 0) probably needs to be a new NLA_FLAG?  
> 
> FWIW, we have an incompatibility issue with the recent rxfh.input_xfrm
> parameter.
> 
> In ethtool_set_rxfh():
> 	/* If either indir, hash key or function is valid, proceed further.
> 	 * Must request at least one change: indir size, hash key, function
> 	 * or input transformation.
> 	 */
> 	if ((rxfh.indir_size &&
> 	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
> 	     rxfh.indir_size != dev_indir_size) ||
> 	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
> 	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
> 	     rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE &&
> 	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))
> 		return -EINVAL;
> 
> When using a recent kernel with an old userspace ethtool,
> rxfh.input_xfrm is treated as zero (which is different than
> RXH_XFRM_NO_CHANGE) and passes the check, whereas the same command with
> a recent userspace would result in an error.
> This also makes it so old userspace always disables input_xfrm
> unintentionally. I do not have any ideas on how to resolve this..
> 
> Regardless, I believe this check is wrong as it prevents us from
> creating RSS context with no parameters (i.e. 'ethtool -X eth0 context
> new', as done in selftests), it works by mistake with old userspace.
> I plan to submit a patch soon to skip this check in case of context
> creation.

I guess we just need to throw "&& !create" into the condition?
Sounds good! We should probably split the "actual invalid" from 
the "nothing specified" checks.

Also - curious what you'll put under Fixes, looks like a pretty 
ancient bug :)

Re: [PATCH net-next v2 00/12] ethtool: rss: driver tweaks and netlink context dumps

[Gal Pressman]

On 06/08/2024 1:13, Jakub Kicinski wrote:
> On Sun, 4 Aug 2024 09:08:50 +0300 Gal Pressman wrote:
>>>    The only question here is how to handle all the tricky IOCTL
>>>    legacy. "No change" maps trivially to attribute not present.
>>>    "reset" (indir_size = 0) probably needs to be a new NLA_FLAG?  
>>
>> FWIW, we have an incompatibility issue with the recent rxfh.input_xfrm
>> parameter.
>>
>> In ethtool_set_rxfh():
>> 	/* If either indir, hash key or function is valid, proceed further.
>> 	 * Must request at least one change: indir size, hash key, function
>> 	 * or input transformation.
>> 	 */
>> 	if ((rxfh.indir_size &&
>> 	     rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE &&
>> 	     rxfh.indir_size != dev_indir_size) ||
>> 	    (rxfh.key_size && (rxfh.key_size != dev_key_size)) ||
>> 	    (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE &&
>> 	     rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE &&
>> 	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))
>> 		return -EINVAL;
>>
>> When using a recent kernel with an old userspace ethtool,
>> rxfh.input_xfrm is treated as zero (which is different than
>> RXH_XFRM_NO_CHANGE) and passes the check, whereas the same command with
>> a recent userspace would result in an error.
>> This also makes it so old userspace always disables input_xfrm
>> unintentionally. I do not have any ideas on how to resolve this..
>>
>> Regardless, I believe this check is wrong as it prevents us from
>> creating RSS context with no parameters (i.e. 'ethtool -X eth0 context
>> new', as done in selftests), it works by mistake with old userspace.
>> I plan to submit a patch soon to skip this check in case of context
>> creation.
> 
> I guess we just need to throw "&& !create" into the condition?
> Sounds good! 

Yes.

> We should probably split the "actual invalid" from 
> the "nothing specified" checks.

And make the "no change" check return zero?

> 
> Also - curious what you'll put under Fixes, looks like a pretty 
> ancient bug :)

Maybe 84a1d9c48200 ("net: ethtool: extend RXNFC API to support RSS
spreading of filter matches")?

Re: [PATCH net-next v2 00/12] ethtool: rss: driver tweaks and netlink context dumps

[Jakub Kicinski]

On Tue, 6 Aug 2024 15:22:07 +0300 Gal Pressman wrote:
> > I guess we just need to throw "&& !create" into the condition?
> > Sounds good!   
> 
> Yes.
> 
> > We should probably split the "actual invalid" from 
> > the "nothing specified" checks.  
> 
> And make the "no change" check return zero?

My knee jerk reaction would be to keep the error return code.
But I guess one could argue in either direction.

> > Also - curious what you'll put under Fixes, looks like a pretty 
> > ancient bug :)  
> 
> Maybe 84a1d9c48200 ("net: ethtool: extend RXNFC API to support RSS
> spreading of filter matches")?

Nod.

Re: [PATCH net-next v2 00/12] ethtool: rss: driver tweaks and netlink context dumps

[Gal Pressman]

On 06/08/2024 17:20, Jakub Kicinski wrote:
> On Tue, 6 Aug 2024 15:22:07 +0300 Gal Pressman wrote:
>>> I guess we just need to throw "&& !create" into the condition?
>>> Sounds good!   
>>
>> Yes.
>>
>>> We should probably split the "actual invalid" from 
>>> the "nothing specified" checks.  
>>
>> And make the "no change" check return zero?
> 
> My knee jerk reaction would be to keep the error return code.
> But I guess one could argue in either direction.

Yea, maybe it's better to not risk upsetting users with a behavior change.
I'll start with a net submission for the fix, then figure out what
should be done for net-next.

Repeating what I said in my earlier mail, I do not think there's a way
to actually solve the compatibility issue. There is no way for the
kernel to differentiate between old userspace that is not aware of
input_xfrm vs. new userspace that explicitly sets it to zero, I guess
we're stuck with this bug :\.