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

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

[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