[PATCH net-next v7 11/13] net: mana: keep a user-configured RSS table across a queue rebuild

[PATCH net-next v7 11/13] net: mana: keep a user-configured RSS table across a queue rebuild

[Wei Hu]

From: Long Li <longli@microsoft.com>

Preserve a user RSS table whenever all entries fit the requested queue
count. Regenerate driver defaults. On growth, a retained user table does
not steer RSS traffic to the added queues until the user updates it.

Report table loss only after successful queue-set publication. The
non-swap allocation path still replaces invalid tables silently because
its callers do not consistently hold the notification's netdev lock.

Require RTNL for RSS setters to serialize them with reset/resume rebuilds.

Signed-off-by: Long Li <longli@microsoft.com>
Signed-off-by: Wei Hu <weh@microsoft.com>
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 41 +++++++++++++++++--
 .../ethernet/microsoft/mana/mana_ethtool.c    |  3 +-
 include/net/mana/mana.h                       |  2 +
 3 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 2aa18cb10d90..7cf7ce9bfa28 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -3621,6 +3621,27 @@ static void mana_rss_table_init(struct mana_port_context *apc)
 			ethtool_rxfh_indir_default(i, apc->num_queues);
 }
 
+/* Keep user tables with valid indices; defer loss notification. */
+static bool mana_rss_table_keep(struct mana_port_context *apc,
+				unsigned int num_queues, bool *lost)
+{
+	u32 i;
+
+	*lost = false;
+
+	if (!netif_is_rxfh_configured(apc->ndev))
+		return false;
+
+	for (i = 0; i < apc->indir_table_sz; i++) {
+		if (apc->indir_table[i] >= num_queues) {
+			*lost = true;
+			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,
@@ -3891,6 +3912,7 @@ int mana_alloc_queues(struct net_device *ndev)
 {
 	struct mana_port_context *apc = netdev_priv(ndev);
 	struct gdma_dev *gd = apc->ac->gdma_dev;
+	bool indir_lost;
 	int err;
 
 	err = mana_create_vport(apc, ndev);
@@ -3936,7 +3958,9 @@ int mana_alloc_queues(struct net_device *ndev)
 		goto destroy_rxq;
 	}
 
-	mana_rss_table_init(apc);
+	/* Loss notification needs a netdev instance lock we may lack. */
+	if (!mana_rss_table_keep(apc, apc->num_queues, &indir_lost))
+		mana_rss_table_init(apc);
 
 	err = mana_config_rss(apc, TRI_STATE_TRUE, true, true);
 	if (err) {
@@ -4094,9 +4118,10 @@ static void mana_qset_snapshot(const struct mana_port_context *ctx,
 	out->priv_flags		= ctx->priv_flags;
 	out->mtu		= ctx->configured_mtu;
 	out->bpf_prog		= ctx->bpf_prog;
+
+	out->rxfh_indir_lost	= false;
 }
 
-/* Vport identity and port debugfs outlive queue sets. */
 static void mana_qset_install(struct mana_port_context *ctx,
 			      const struct mana_qset *qset)
 {
@@ -4156,6 +4181,7 @@ int mana_alloc_qset(struct mana_port_context *apc,
 		    struct mana_qset *out)
 {
 	struct net_device *ndev = scratch->ndev;
+	bool indir_lost;
 	int err;
 
 	ASSERT_RTNL();
@@ -4191,9 +4217,14 @@ int mana_alloc_qset(struct mana_port_context *apc,
 	if (err)
 		goto cleanup_rxq;
 
-	mana_rss_table_init(scratch);
+	if (mana_rss_table_keep(apc, num_queues, &indir_lost))
+		memcpy(scratch->indir_table, apc->indir_table,
+		       apc->indir_table_sz * sizeof(*apc->indir_table));
+	else
+		mana_rss_table_init(scratch);
 
 	mana_qset_snapshot(scratch, out);
+	out->rxfh_indir_lost = indir_lost;
 	return 0;
 
 cleanup_rxq:
@@ -4432,6 +4463,10 @@ int mana_publish_qset(struct mana_port_context *apc, struct mana_qset *newq,
 	WRITE_ONCE(apc->port_is_up, true);
 	mana_start_txqs(apc);
 
+	/* Report a lost user table only after successful publication. */
+	if (newq->rxfh_indir_lost)
+		ethtool_rxfh_indir_lost(ndev);
+
 	return 0;
 
 rollback:
diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index d26fefbb72c3..8988a25db504 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -952,7 +952,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,
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index fcb66029e51c..d5eef06dedb2 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -720,6 +720,8 @@ struct mana_qset {
 	int			mtu;
 	struct bpf_prog		*bpf_prog;
 
+	/* Notify the core only after this set is published. */
+	bool			rxfh_indir_lost;
 };
 
 netdev_tx_t mana_start_xmit(struct sk_buff *skb, struct net_device *ndev);
-- 
2.43.0

Re: [PATCH net-next v7 11/13] net: mana: keep a user-configured RSS table across a queue rebuild

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: mana: State desynchronization on silent RSS table reset leaves queues unutilized
--

commit f7f07913c3b72971a54af27655b2426b066f266e
Author: Long Li <longli@microsoft.com>

net: mana: keep a user-configured RSS table across a queue rebuild

Preserve a user RSS table whenever all entries fit the requested queue
count. Regenerate driver defaults. On growth, a retained user table does
not steer RSS traffic to the added queues until the user updates it.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 2aa18cb10d902..7cf7ce9bfa28c 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -3936,7 +3958,9 @@ int mana_alloc_queues(struct net_device *ndev)
>  		goto destroy_rxq;
>  	}
>  
> -	mana_rss_table_init(apc);
> +	/* Loss notification needs a netdev instance lock we may lack. */
> +	if (!mana_rss_table_keep(apc, apc->num_queues, &indir_lost))
> +		mana_rss_table_init(apc);

[Severity: Medium]
Does this silent reset desynchronize the driver from the ethtool core?

By silently reinitializing the table in mana_alloc_queues() without calling
ethtool_rxfh_indir_lost(), it leaves the ethtool core's
dev->ethtool->rss_indir_user_size > 0. If a user subsequently increases the
queue count, the ethtool core will falsely validate the small table, and
the newly added Rx queues will receive no RSS traffic.

The comment mentions lacking a netdev instance lock. However, callers of
mana_alloc_queues() like mana_open() and mana_attach() hold the RTNL lock,
which satisfies the locking requirements of ethtool_rxfh_indir_lost():

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");
    ...
}

Can we safely call ethtool_rxfh_indir_lost() here to keep the states
synchronized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791710735.git.weh@linux.microsoft.com?part=11

Re: [PATCH net-next v7 11/13] net: mana: keep a user-configured RSS table across a queue rebuild

[Wei Hu]

Hi,

> Does this silent reset desynchronize the driver from the ethtool core?

I will leave the ordinary allocation path unchanged in v8. The stale
user-indirection metadata is a real gap, but both upstream bases already
reinitialize the RSS table in mana_alloc_queues() without clearing the
ethtool core's user-table state. It is not introduced by this series.

A hardware queue-limit reduction during reattach can make a saved user
table invalid. Rebuilding a default table without reporting the loss can
then leave the core describing a user configuration that no longer
exists. I am treating this as an independent baseline issue, not claiming
that documenting the silent reset makes it correct.

The specific later-growth scenario also needs qualification because the
inspected reattach path monotonically clamps apc->max_queues. The stale
metadata and missing loss notification are the more direct issues.

> Can we safely call ethtool_rxfh_indir_lost() here to keep the states
> synchronized?

Not solely on the basis of its initial RTNL assertion. The notification
call chain also reaches netdev_assert_locked_ops_compat(). MANA registers
net_shaper_ops, so that check requires the netdev operations lock. The
reset worker holds RTNL but not that lock.

A separate correction needs to account for the notification context and
locking of every ordinary allocation caller; an unconditional helper
call at this location is not sufficient. That broader baseline fix is
outside this queue-set revision.

Thanks,
Wei

Re: [PATCH net-next v7 11/13] net: mana: keep a user-configured RSS table across a queue rebuild

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In mana_alloc_queues()
  (drivers/net/ethernet/microsoft/mana/mana_en.c:3961-3963),…
- [Low] The publish-time loss notification added by this patch can never
  fire.
- [Low] Before this patch, mana_alloc_queues() and mana_alloc_qset()
  called mana_rss_table_init() unconditionally.
- [Low] The patch deletes the comment `/* Vport identity and port debugfs
  outlive queue sets.

--

net: mana: keep a user-configured RSS table across a queue rebuild

This commit keeps a user-configured RSS indirection table across queue
rebuilds when every entry is valid for the new queue count. Otherwise it
regenerates the driver default table. A lost user table is reported
through ethtool_rxfh_indir_lost() after a queue set is published, and
RSS setters now require RTNL.

> Preserve a user RSS table whenever all entries fit the requested queue
> count. Regenerate driver defaults. On growth, a retained user table does
> not steer RSS traffic to the added queues until the user updates it.

[Severity: Low]
This isn't a bug, but before this patch mana_alloc_queues() and
mana_alloc_qset() both called mana_rss_table_init() unconditionally.
That dropped a user "ethtool -X" table on every rebuild (down/up,
reset/resume, MTU, ring, priv-flag and XDP changes), while the core's
rss_indir_user_size stayed set.

Since this fixes a bug users can see, should the commit message say so
and carry a Fixes: tag?

The message also doesn't mention that adding ETHTOOL_OP_NEEDS_RTNL_RSS is
part of the fix. It doesn't mention the dependency on the earlier
queue-set swap patches either.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 2aa18cb10d902..7cf7ce9bfa28c 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -3936,7 +3958,9 @@ int mana_alloc_queues(struct net_device *ndev)
>  		goto destroy_rxq;
>  	}
>  
> -	mana_rss_table_init(apc);
> +	/* Loss notification needs a netdev instance lock we may lack. */
> +	if (!mana_rss_table_keep(apc, apc->num_queues, &indir_lost))
> +		mana_rss_table_init(apc);

[Severity: Medium]
When mana_rss_table_keep() sets indir_lost here, the value is thrown away
and ethtool_rxfh_indir_lost() is never called. Doesn't that leave
dev->ethtool->rss_indir_user_size set? If so, netif_is_rxfh_configured()
keeps returning true for what is now the driver default table.

This looks like the one loss path that can actually be reached. On reset
or resume, mana_attach()->mana_init_port() queries the vPort again and
clamps the queue count:

	if (apc->num_queues > apc->max_queues)
		apc->num_queues = apc->max_queues;

On resume, gc->max_num_queues_vport is also recomputed in
mana_gd_setup()->mana_gd_query_max_resources() from num_online_cpus()
and the MSI-X count. If the port was down during the reset, the same loss
happens later in mana_open()->mana_alloc_queues(), which also drops
indir_lost.

The kernel-doc for ethtool_rxfh_indir_lost() describes this exact case
("typically after a HW fault recovery that reduced the maximum queue
count"). The helper also clears the configured state, not just sends a
notification:

	dev->ethtool->rss_indir_user_size = 0;
	ethtool_rss_notify(dev, ETHTOOL_MSG_RSS_NTF, 0);

With the configured flag left set, ethtool_get_max_rxfh_channel() reads
the regenerated default table, and that table spans every queue.
ethtool_check_max_channel() would then reject every later channel
reduction with -EINVAL until the user runs "ethtool -X <dev> default".
Later rebuilds would also accept the defaults as a user table in
mana_rss_table_keep(), and no ETHTOOL_MSG_RSS_NTF is ever sent.

The comment is right that the netdev instance lock may not be held here.
mana_per_port_queue_reset_work_handler() holds only RTNL, and the resume
loop in mana_probe() drops netdev_lock() just before mana_attach().

Could mana_alloc_queues()/mana_attach() return or record the lost state
instead? Callers could then call ethtool_rxfh_indir_lost() under RTNL
plus netdev_lock(), as mlx5e_attach_netdev() does. mana_open() already
holds both locks and could report the loss directly.

The commit message says this path "replaces invalid tables silently". It
doesn't mention that the core's configured state is left stale. The
indir_lost value still appears to be dropped at this call site at the end
of the series.

>  
>  	err = mana_config_rss(apc, TRI_STATE_TRUE, true, true);
>  	if (err) {
> @@ -4094,9 +4118,10 @@ static void mana_qset_snapshot(const struct mana_port_context *ctx,
>  	out->priv_flags		= ctx->priv_flags;
>  	out->mtu		= ctx->configured_mtu;
>  	out->bpf_prog		= ctx->bpf_prog;
> +
> +	out->rxfh_indir_lost	= false;
>  }
>  
> -/* Vport identity and port debugfs outlive queue sets. */
>  static void mana_qset_install(struct mana_port_context *ctx,
>  			      const struct mana_qset *qset)

[Severity: Low]
This isn't a bug, but was this comment meant to be removed? The change
looks unrelated to keeping the RSS table, and the commit message doesn't
mention it.

The comment explains why mana_qset_install() leaves port_handle, the MAC
address and mana_port_debugfs alone. The function still works that way,
and the comment isn't added back later in the series.

[ ... ]

> @@ -4432,6 +4463,10 @@ int mana_publish_qset(struct mana_port_context *apc, struct mana_qset *newq,
>  	WRITE_ONCE(apc->port_is_up, true);
>  	mana_start_txqs(apc);
>  
> +	/* Report a lost user table only after successful publication. */
> +	if (newq->rxfh_indir_lost)
> +		ethtool_rxfh_indir_lost(ndev);

[Severity: Low]
Can newq->rxfh_indir_lost ever be true here?

mana_change_mtu(), mana_set_ringparam(), mana_set_priv_flags() and
mana_xdp_set() all rebuild at the current num_queues. At that count the
live table is already valid: mana_set_rxfh() rejects entries >=
num_queues, and mana_alloc_queues() regenerates invalid tables.

mana_set_channels() is the only path that changes the count. Both the
netlink and ioctl set-channels handlers call ethtool_check_max_channel()
before the driver runs:

	max_rxfh_in_use = ethtool_get_max_rxfh_channel(dev);
	if (channels.combined_count + channels.rx_count <= max_rxfh_in_use) {

That check rejects any count that would invalidate a user-configured
table. So it seems mana_rss_table_keep() can never report a loss on the
swap path. The same is true for mana_split_qset() and mana_grow_qset()
later in the series.

If that's right, the rxfh_indir_lost field and this call cover a case
that can't happen. Meanwhile the loss that can happen, in
mana_alloc_queues(), is never reported. Should the commit message
sentence "Report table loss only after successful queue-set publication"
be reworded, or this code dropped?

>  
>  	return 0;
>  
>  rollback:

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791710735.git.weh%40linux.microsoft.com