mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net] net: mana: do not discard an ethtool-configured RSS table on a queue rebuild
@ 2026-09-05  0:44 Long Li
  2026-09-10 12:46 ` netdev-bot+sashiko
  2026-09-10 15:18 ` Jakub Kicinski
  0 siblings, 2 replies; 4+ messages in thread
From: Long Li @ 2026-09-05  0:44 UTC (permalink / raw)
  To: Long Li, Konstantin Taranov, Jakub Kicinski, David S . Miller,
	Paolo Abeni, Eric Dumazet, Andrew Lunn, Jason Gunthorpe,
	Leon Romanovsky, Haiyang Zhang, K . Y . Srinivasan, Wei Liu,
	Dexuan Cui, shradhagupta, Simon Horman, ernis, stephen,
	shirazsaleem
  Cc: netdev, linux-rdma, linux-hyperv, linux-kernel

mana_alloc_queues() regenerates the RSS indirection table from the driver
default every time the queues are built, so a table installed with
"ethtool -X" is silently replaced by any operation that rebuilds them:
an MTU change, a ring-size, channel-count or private-flag change, an XDP
attach, TX-timeout reset recovery, or resume.

The driver never clears the core's IFF_RXFH_CONFIGURED, so
netif_is_rxfh_configured() keeps reporting a user table while the
hardware has been reprogrammed with the default one. "ethtool -x" then
shows a table the user did not ask for, with no indication it changed:

  # ethtool -X ens1 equal 2
  # ethtool -x ens1
  RX flow hash indirection table for ens1 with 16 RX ring(s):
      0:      0     1     0     1     0     1     0     1
      8:      0     1     0     1     0     1     0     1
  # ip link set ens1 mtu 1400
  # ethtool -x ens1
  RX flow hash indirection table for ens1 with 16 RX ring(s):
      0:      0     1     2     3     4     5     6     7
      8:      8     9    10    11    12    13    14    15

Keep the table instead, and rebuild it only when it is driver-generated
or cannot be honoured. An entry may not be kept if it points past the
last queue: mana_config_rss() uses these entries to index apc->rxqs[],
which holds apc->num_queues pointers. That is reachable because
mana_attach() calls mana_init_port(), which lowers apc->num_queues to the
maximum the device reports, so a table configured for more queues can
outlive them.

A table that cannot be kept is still replaced by the default silently,
without ethtool_rxfh_indir_lost(). That helper sends
ETHTOOL_MSG_RSS_NTF, which requires the netdev instance lock, and
mana_alloc_queues() runs both with that lock held, from ndo_open, and
without it, from mana_attach() on the reset and resume paths. Leaving
the core's view untouched is what the driver did for every table before
this change.

Opt the RSS ethtool operations into rtnl_lock() while here. Reading the
table in mana_alloc_queues() has to be serialized against mana_set_rxfh()
replacing it, and the two had no lock in common: mana_set_rxfh() ran under
the netdev instance lock alone, while mana_alloc_queues() reaches this
point holding only RTNL, from ndo_open and from mana_attach() on the reset
and resume paths. Taking the instance lock there instead is not possible,
since ndo_open already runs with it held. The same flag covers the netlink
and ioctl entry points.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 37 ++++++++++++++++++-
 .../ethernet/microsoft/mana/mana_ethtool.c    |  3 +-
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 7a1ac853e3abcd28c4a1e5c6987ec631a18ad840..97386e17b9421aedefa25bab6fbf0b7b1f2996d4 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -3306,6 +3306,28 @@ static void mana_rss_table_init(struct mana_port_context *apc)
 			ethtool_rxfh_indir_default(i, apc->num_queues);
 }
 
+/* Whether the current indirection table can be kept for apc->num_queues,
+ * rather than rebuilt from the driver default.
+ *
+ * Only a user table ("ethtool -X") is worth keeping; a driver-generated one
+ * is rebuilt so that it spreads over every queue. An entry pointing past the
+ * last queue cannot be kept: mana_config_rss() uses these entries to index
+ * apc->rxqs[], which holds apc->num_queues pointers.
+ */
+static bool mana_rss_table_keep(struct mana_port_context *apc)
+{
+	u32 i;
+
+	if (!netif_is_rxfh_configured(apc->ndev))
+		return false;
+
+	for (i = 0; i < apc->indir_table_sz; i++)
+		if (apc->indir_table[i] >= apc->num_queues)
+			return false;
+
+	return true;
+}
+
 int mana_disable_vport_rx(struct mana_port_context *apc)
 {
 	return mana_cfg_vport_steering(apc, TRI_STATE_FALSE, false, false,
@@ -3621,7 +3643,20 @@ int mana_alloc_queues(struct net_device *ndev)
 		goto destroy_rxq;
 	}
 
-	mana_rss_table_init(apc);
+	/* Keep a user-configured table across the rebuild: its entries are
+	 * queue indices and stay meaningful while they are all still in range.
+	 * Only a driver-generated table is regenerated here.
+	 *
+	 * A table that cannot be kept is replaced by the default without
+	 * telling the core, which keeps reporting the table as user
+	 * configured. That is what this function did for every table before,
+	 * and reporting it here is not an option: ethtool_rxfh_indir_lost()
+	 * sends ETHTOOL_MSG_RSS_NTF, which requires the netdev instance lock,
+	 * and this runs both with that lock held (ndo_open) and without it
+	 * (mana_attach() from the reset and resume paths).
+	 */
+	if (!mana_rss_table_keep(apc))
+		mana_rss_table_init(apc);
 
 	err = mana_config_rss(apc, TRI_STATE_TRUE, true, true);
 	if (err) {
diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index ece7ff9cc409a806b6a6de70a85b44874bfa6dad..e3acaf17efabcd62f7cffabdb8404cf16b917272 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -871,7 +871,8 @@ const struct ethtool_ops mana_ethtool_ops = {
 	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
 				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
 				  ETHTOOL_OP_NEEDS_RTNL_SPFLAGS |
-				  ETHTOOL_OP_NEEDS_RTNL_GLINK,
+				  ETHTOOL_OP_NEEDS_RTNL_GLINK |
+				  ETHTOOL_OP_NEEDS_RTNL_RSS,
 	.get_ethtool_stats	= mana_get_ethtool_stats,
 	.get_sset_count		= mana_get_sset_count,
 	.get_strings		= mana_get_strings,
-- 
2.43.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net: mana: do not discard an ethtool-configured RSS table on a queue rebuild
  2026-09-05  0:44 [PATCH net] net: mana: do not discard an ethtool-configured RSS table on a queue rebuild Long Li
@ 2026-09-10 12:46 ` netdev-bot+sashiko
  2026-09-10 15:18 ` Jakub Kicinski
  1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-10 12:46 UTC (permalink / raw)
  To: longli
  Cc: kotaranov, kuba, davem, pabeni, edumazet, andrew+netdev, jgg,
	leon, haiyangz, kys, wei.liu, decui, shradhagupta, horms, ernis,
	stephen, shirazsaleem, netdev, linux-rdma, linux-hyperv,
	linux-kernel

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 2 (1)

- [Medium] Undisclosed TX-side consequence of preserving the indirection
  table.
- [Low] The commit message states: "The driver never clears the core's
  IFF_RXFH_CONFIGURED, so netif_is_rxfh_configured() keeps reporting a…
- [Low] When the surviving table cannot be honoured (an entry >=
  apc->num_queues after mana_init_port() lowered the queue count),…

Pre-existing issues:
- [Low] Incomplete lockset for apc->indir_table on the ethtool read side.

--

net: mana: do not discard an ethtool-configured RSS table on a queue rebuild

mana_alloc_queues() no longer regenerates the RSS indirection table from the
driver default when the table was installed by the user, so an "ethtool -X"
table now survives operations that rebuild the queues.  A table whose
entries point past the last queue is still replaced by the default.  The RSS
ethtool operations are also opted into rtnl_lock() via
ETHTOOL_OP_NEEDS_RTNL_RSS.

A couple of questions about the commit message and the new code below.

>     The driver never clears the core's IFF_RXFH_CONFIGURED, so
>     netif_is_rxfh_configured() keeps reporting a user table while the
>     hardware has been reprogrammed with the default one.

[Severity: Low]
Is IFF_RXFH_CONFIGURED still the right name here?  A tree-wide grep for
IFF_RXFH_CONFIGURED at this commit returns nothing.  The state that
netif_is_rxfh_configured() consults is now:

net/ethtool/common.c:
bool netif_is_rxfh_configured(const struct net_device *dev)
{
	return dev->ethtool->rss_indir_user_size;
}

set and cleared by the core in net/ethtool/rss.c and net/ethtool/ioctl.c.
The mechanism described matches, only the symbol name looks stale.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 45a7520491a65..396b741fa86ed 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -3310,6 +3310,28 @@ static void mana_rss_table_init(struct mana_port_context *apc)
>  			ethtool_rxfh_indir_default(i, apc->num_queues);
>  }
>  
> +/* Whether the current indirection table can be kept for apc->num_queues,
> + * rather than rebuilt from the driver default.
> + *
> + * Only a user table ("ethtool -X") is worth keeping; a driver-generated one
> + * is rebuilt so that it spreads over every queue. An entry pointing past the
> + * last queue cannot be kept: mana_config_rss() uses these entries to index
> + * apc->rxqs[], which holds apc->num_queues pointers.
> + */
> +static bool mana_rss_table_keep(struct mana_port_context *apc)
> +{
> +	u32 i;
> +
> +	if (!netif_is_rxfh_configured(apc->ndev))
> +		return false;
> +
> +	for (i = 0; i < apc->indir_table_sz; i++)
> +		if (apc->indir_table[i] >= apc->num_queues)
> +			return false;
> +
> +	return true;
> +}

[Severity: Medium]
Does keeping the table also pin the TX side?  In this driver
apc->indir_table selects the transmit queue too:

mana_en.c:mana_get_tx_queue() {
	...
	txq = apc->indir_table[hash & (apc->indir_table_sz - 1)];
	...
}

reached from mana_select_queue(), and the struct member itself is commented
"Indirection Table for RX & TX" in include/net/mana/mana.h.

Before this change every rebuild ran mana_rss_table_init() and restored
index % num_queues, so this sequence ended up with both RX and TX spread
over all 16 queues:

	ethtool -X ens1 equal 2
	ethtool -L ens1 combined 16

After the change mana_rss_table_keep() returns true, since every entry (0
and 1) is below the new apc->num_queues of 16, so the two-entry table
survives mana_set_channels() -> mana_attach() -> mana_alloc_queues().
netif_set_real_num_tx_queues() then publishes 16 TX queues while
mana_get_tx_queue() can only ever return 0 or 1.

The changelog lists the rebuild paths that are affected but describes the
change purely in terms of the RSS indirection table.  Would it be worth
mentioning that TX queue selection becomes sticky in the same way?

> @@ -3625,7 +3647,20 @@ int mana_alloc_queues(struct net_device *ndev)
>  		goto destroy_rxq;
>  	}
>  
> -	mana_rss_table_init(apc);
> +	/* Keep a user-configured table across the rebuild: its entries are
> +	 * queue indices and stay meaningful while they are all still in range.
> +	 * Only a driver-generated table is regenerated here.
> +	 *
> +	 * A table that cannot be kept is replaced by the default without
> +	 * telling the core, which keeps reporting the table as user
> +	 * configured. That is what this function did for every table before,
> +	 * and reporting it here is not an option: ethtool_rxfh_indir_lost()
> +	 * sends ETHTOOL_MSG_RSS_NTF, which requires the netdev instance lock,
> +	 * and this runs both with that lock held (ndo_open) and without it
> +	 * (mana_attach() from the reset and resume paths).
> +	 */
> +	if (!mana_rss_table_keep(apc))
> +		mana_rss_table_init(apc);

[Severity: Low]
This is the acknowledged case, so mostly a question about whether it can be
narrowed rather than a new problem.

When mana_attach() -> mana_init_port() lowers apc->num_queues and an entry
falls out of range, this branch installs the driver default while
dev->ethtool->rss_indir_user_size stays set, so netif_is_rxfh_configured()
and "ethtool -x" keep advertising a user table the user never installed.

The stated reason for skipping ethtool_rxfh_indir_lost() does hold up: its
own assert accepts RTNL,

net/ethtool/common.c:ethtool_rxfh_indir_lost() {
	WARN_ONCE(!rtnl_is_locked() &&
		  !lockdep_is_held_type(&dev->ethtool->rss_lock, -1),
		  "RSS context lock assertion failed\n");
	...
	dev->ethtool->rss_indir_user_size = 0;
	ethtool_rss_notify(dev, ETHTOOL_MSG_RSS_NTF, 0);
}

but the notification lands in ethnl_notify(), which runs
netdev_assert_locked_ops_compat(dev).  mana registers net_shaper_ops, so
with CONFIG_NET_SHAPER=y that reduces to lockdep_assert_held(&dev->lock),
which the rtnl-only reset and resume paths do not hold.

Is there a way to reach the same result without the notification, so that
the core's view and the programmed table agree in this case?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index ece7ff9cc409a..e3acaf17efabc 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -871,7 +871,8 @@ const struct ethtool_ops mana_ethtool_ops = {
>  	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
>  				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
>  				  ETHTOOL_OP_NEEDS_RTNL_SPFLAGS |
> -				  ETHTOOL_OP_NEEDS_RTNL_GLINK,
> +				  ETHTOOL_OP_NEEDS_RTNL_GLINK |
> +				  ETHTOOL_OP_NEEDS_RTNL_RSS,

[Severity: Low]
This isn't a problem introduced by this patch, but does the new flag cover
the read side as well?

The core only consults ETHTOOL_OP_NEEDS_RTNL_RSS for ETHTOOL_MSG_RSS_SET
and for the SRSSH / SRXFH / SRXFHINDIR ioctls:

net/ethtool/common.h:
	case ETHTOOL_MSG_RSS_SET:
		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_RSS;

so mana_get_rxfh() still runs with the netdev instance lock plus rss_lock
only, while the rebuild writes the same array entry by entry holding RTNL
alone when it comes from mana_attach():

mana_en.c:mana_rss_table_init() {
	for (i = 0; i < apc->indir_table_sz; i++)
		apc->indir_table[i] =
			ethtool_rxfh_indir_default(i, apc->num_queues);
}

CPU0: mana_per_port_queue_reset_work_handler() -> rtnl_lock() ->
      mana_attach() -> mana_alloc_queues() -> mana_rss_table_init()
CPU1: ethtool -x -> rss_prepare_get() -> mana_get_rxfh()

Can "ethtool -x" print a mix of old and new entries in that window?  The
hardware itself is programmed from the driver copy by mana_config_rss(), so
this looks limited to the reported table.  The write/read race the patch set
out to close on the set side does get closed by the new flag.

For what it is worth, the freed-table variant of this does not appear to
exist: mana_attach()'s failure path calls mana_cleanup_port_context(), which
only frees apc->rxqs and the debugfs dentry, and mana_cleanup_indir_table()
is only reached from mana_probe_port() before register_netdev() and from
mana_remove() after unregister_netdevice().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260905004401.3937066-1-longli%40microsoft.com

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net: mana: do not discard an ethtool-configured RSS table on a queue rebuild
  2026-09-05  0:44 [PATCH net] net: mana: do not discard an ethtool-configured RSS table on a queue rebuild Long Li
  2026-09-10 12:46 ` netdev-bot+sashiko
@ 2026-09-10 15:18 ` Jakub Kicinski
  2026-09-10 20:00   ` [EXTERNAL] " Long Li
  1 sibling, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2026-09-10 15:18 UTC (permalink / raw)
  To: Long Li
  Cc: Konstantin Taranov, David S . Miller, Paolo Abeni, Eric Dumazet,
	Andrew Lunn, Jason Gunthorpe, Leon Romanovsky, Haiyang Zhang,
	K . Y . Srinivasan, Wei Liu, Dexuan Cui, shradhagupta,
	Simon Horman, ernis, stephen, shirazsaleem, netdev, linux-rdma,
	linux-hyperv, linux-kernel

On Fri,  4 Sep 2026 17:44:01 -0700 Long Li wrote:
> mana_alloc_queues() regenerates the RSS indirection table from the driver
> default every time the queues are built, so a table installed with
> "ethtool -X" is silently replaced by any operation that rebuilds them:
> an MTU change, a ring-size, channel-count or private-flag change, an XDP
> attach, TX-timeout reset recovery, or resume.
> 
> The driver never clears the core's IFF_RXFH_CONFIGURED, so
> netif_is_rxfh_configured() keeps reporting a user table while the
> hardware has been reprogrammed with the default one. "ethtool -x" then
> shows a table the user did not ask for, with no indication it changed:
> 
>   # ethtool -X ens1 equal 2
>   # ethtool -x ens1
>   RX flow hash indirection table for ens1 with 16 RX ring(s):
>       0:      0     1     0     1     0     1     0     1
>       8:      0     1     0     1     0     1     0     1
>   # ip link set ens1 mtu 1400
>   # ethtool -x ens1
>   RX flow hash indirection table for ens1 with 16 RX ring(s):
>       0:      0     1     2     3     4     5     6     7
>       8:      8     9    10    11    12    13    14    15
> 
> Keep the table instead, and rebuild it only when it is driver-generated
> or cannot be honoured. An entry may not be kept if it points past the
> last queue: mana_config_rss() uses these entries to index apc->rxqs[],
> which holds apc->num_queues pointers. That is reachable because
> mana_attach() calls mana_init_port(), which lowers apc->num_queues to the
> maximum the device reports, so a table configured for more queues can
> outlive them.

To be clear this is only acceptable if the number of queues drops due 
to re-negotiation of caps with the device, not for example if XDP
requires some queues to be used for other purposes. In the latter case
just refuse the config change.

> A table that cannot be kept is still replaced by the default silently,
> without ethtool_rxfh_indir_lost(). That helper sends
> ETHTOOL_MSG_RSS_NTF, which requires the netdev instance lock, and
> mana_alloc_queues() runs both with that lock held, from ndo_open, and
> without it, from mana_attach() on the reset and resume paths. Leaving
> the core's view untouched is what the driver did for every table before
> this change.

Okay, so you have a problem of not having the lock...

> Opt the RSS ethtool operations into rtnl_lock() while here. Reading the
> table in mana_alloc_queues() has to be serialized against mana_set_rxfh()
> replacing it, and the two had no lock in common: mana_set_rxfh() ran under
> the netdev instance lock alone, while mana_alloc_queues() reaches this
> point holding only RTNL, from ndo_open and from mana_attach() on the reset
> and resume paths. Taking the instance lock there instead is not possible,
> since ndo_open already runs with it held. The same flag covers the netlink
> and ioctl entry points.

.. and yet your fix is not to try to take it but the reverse, to add 
a different lock? You need to explain why reset path can't take the
instance lock. Of course you can't take it _inside_ ndo_open, but the
caller should be able to.
 
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 37 ++++++++++++++++++-
>  .../ethernet/microsoft/mana/mana_ethtool.c    |  3 +-
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7a1ac853e3abcd28c4a1e5c6987ec631a18ad840..97386e17b9421aedefa25bab6fbf0b7b1f2996d4 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -3306,6 +3306,28 @@ static void mana_rss_table_init(struct mana_port_context *apc)
>  			ethtool_rxfh_indir_default(i, apc->num_queues);
>  }
>  
> +/* Whether the current indirection table can be kept for apc->num_queues,
> + * rather than rebuilt from the driver default.
> + *
> + * Only a user table ("ethtool -X") is worth keeping; a driver-generated one
> + * is rebuilt so that it spreads over every queue. An entry pointing past the
> + * last queue cannot be kept: mana_config_rss() uses these entries to index
> + * apc->rxqs[], which holds apc->num_queues pointers.
> + */
> +static bool mana_rss_table_keep(struct mana_port_context *apc)
> +{
> +	u32 i;
> +
> +	if (!netif_is_rxfh_configured(apc->ndev))
> +		return false;
> +
> +	for (i = 0; i < apc->indir_table_sz; i++)
> +		if (apc->indir_table[i] >= apc->num_queues)
> +			return false;
> +
> +	return true;
> +}
> +
>  int mana_disable_vport_rx(struct mana_port_context *apc)
>  {
>  	return mana_cfg_vport_steering(apc, TRI_STATE_FALSE, false, false,
> @@ -3621,7 +3643,20 @@ int mana_alloc_queues(struct net_device *ndev)
>  		goto destroy_rxq;
>  	}
>  
> -	mana_rss_table_init(apc);
> +	/* Keep a user-configured table across the rebuild: its entries are
> +	 * queue indices and stay meaningful while they are all still in range.
> +	 * Only a driver-generated table is regenerated here.
> +	 *
> +	 * A table that cannot be kept is replaced by the default without
> +	 * telling the core, which keeps reporting the table as user
> +	 * configured. That is what this function did for every table before,
> +	 * and reporting it here is not an option: ethtool_rxfh_indir_lost()
> +	 * sends ETHTOOL_MSG_RSS_NTF, which requires the netdev instance lock,
> +	 * and this runs both with that lock held (ndo_open) and without it
> +	 * (mana_attach() from the reset and resume paths).
> +	 */
> +	if (!mana_rss_table_keep(apc))
> +		mana_rss_table_init(apc);
>  
>  	err = mana_config_rss(apc, TRI_STATE_TRUE, true, true);
>  	if (err) {
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index ece7ff9cc409a806b6a6de70a85b44874bfa6dad..e3acaf17efabcd62f7cffabdb8404cf16b917272 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -871,7 +871,8 @@ const struct ethtool_ops mana_ethtool_ops = {
>  	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
>  				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
>  				  ETHTOOL_OP_NEEDS_RTNL_SPFLAGS |
> -				  ETHTOOL_OP_NEEDS_RTNL_GLINK,
> +				  ETHTOOL_OP_NEEDS_RTNL_GLINK |
> +				  ETHTOOL_OP_NEEDS_RTNL_RSS,
>  	.get_ethtool_stats	= mana_get_ethtool_stats,
>  	.get_sset_count		= mana_get_sset_count,
>  	.get_strings		= mana_get_strings,


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [EXTERNAL] Re: [PATCH net] net: mana: do not discard an ethtool-configured RSS table on a queue rebuild
  2026-09-10 15:18 ` Jakub Kicinski
@ 2026-09-10 20:00   ` Long Li
  0 siblings, 0 replies; 4+ messages in thread
From: Long Li @ 2026-09-10 20:00 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Konstantin Taranov, David S . Miller, Paolo Abeni, Eric Dumazet,
	Andrew Lunn, Jason Gunthorpe, Leon Romanovsky, Haiyang Zhang,
	KY Srinivasan, Wei Liu, Dexuan Cui, shradhagupta, Simon Horman,
	ernis, stephen, shirazsaleem, netdev, linux-rdma, linux-hyperv,
	linux-kernel

> On Fri,  4 Sep 2026 17:44:01 -0700 Long Li wrote:
> > mana_alloc_queues() regenerates the RSS indirection table from the
> > driver default every time the queues are built, so a table installed
> > with "ethtool -X" is silently replaced by any operation that rebuilds them:
> > an MTU change, a ring-size, channel-count or private-flag change, an
> > XDP attach, TX-timeout reset recovery, or resume.
> >
> > The driver never clears the core's IFF_RXFH_CONFIGURED, so
> > netif_is_rxfh_configured() keeps reporting a user table while the
> > hardware has been reprogrammed with the default one. "ethtool -x" then
> > shows a table the user did not ask for, with no indication it changed:
> >
> >   # ethtool -X ens1 equal 2
> >   # ethtool -x ens1
> >   RX flow hash indirection table for ens1 with 16 RX ring(s):
> >       0:      0     1     0     1     0     1     0     1
> >       8:      0     1     0     1     0     1     0     1
> >   # ip link set ens1 mtu 1400
> >   # ethtool -x ens1
> >   RX flow hash indirection table for ens1 with 16 RX ring(s):
> >       0:      0     1     2     3     4     5     6     7
> >       8:      8     9    10    11    12    13    14    15
> >
> > Keep the table instead, and rebuild it only when it is
> > driver-generated or cannot be honoured. An entry may not be kept if it
> > points past the last queue: mana_config_rss() uses these entries to
> > index apc->rxqs[], which holds apc->num_queues pointers. That is
> > reachable because
> > mana_attach() calls mana_init_port(), which lowers apc->num_queues to
> > the maximum the device reports, so a table configured for more queues
> > can outlive them.
> 
> To be clear this is only acceptable if the number of queues drops due to re-
> negotiation of caps with the device, not for example if XDP requires some
> queues to be used for other purposes. In the latter case just refuse the config
> change.
> 
> > A table that cannot be kept is still replaced by the default silently,
> > without ethtool_rxfh_indir_lost(). That helper sends
> > ETHTOOL_MSG_RSS_NTF, which requires the netdev instance lock, and
> > mana_alloc_queues() runs both with that lock held, from ndo_open, and
> > without it, from mana_attach() on the reset and resume paths. Leaving
> > the core's view untouched is what the driver did for every table
> > before this change.
> 
> Okay, so you have a problem of not having the lock...
> 
> > Opt the RSS ethtool operations into rtnl_lock() while here. Reading
> > the table in mana_alloc_queues() has to be serialized against
> > mana_set_rxfh() replacing it, and the two had no lock in common:
> > mana_set_rxfh() ran under the netdev instance lock alone, while
> > mana_alloc_queues() reaches this point holding only RTNL, from
> > ndo_open and from mana_attach() on the reset and resume paths. Taking
> > the instance lock there instead is not possible, since ndo_open
> > already runs with it held. The same flag covers the netlink and ioctl entry
> points.
> 
> .. and yet your fix is not to try to take it but the reverse, to add a different
> lock? You need to explain why reset path can't take the instance lock. Of
> course you can't take it _inside_ ndo_open, but the caller should be able to.

Okay, will send a patch implementing the locks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-09-10 20:00 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-05  0:44 [PATCH net] net: mana: do not discard an ethtool-configured RSS table on a queue rebuild Long Li
2026-09-10 12:46 ` netdev-bot+sashiko
2026-09-10 15:18 ` Jakub Kicinski
2026-09-10 20:00   ` [EXTERNAL] " Long Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®