Re: [PATCH net-next v7 04/13] net: mana: swap queue sets in mana_set_channels

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Old RX queues are marked retiring before the TX drain wait, while
  RSS still sends all RX traffic to them.

Pre-existing issues:
- [Low] Concern: the live replacement path throws away a user-configured
  RSS indirection table without calling ethtool_rxfh_indir_lost().
- [Low] Concern: the 120 s TX drain in mana_publish_qset(), with all
  queues stopped and carrier on, lets dev_watchdog (watchdog_timeo =…

--

net: mana: swap queue sets in mana_set_channels

This commit changes mana_set_channels() to build a replacement queue set
first, quiesce TX, drain the old TX queues for up to 120 seconds and then
publish the new set, with a rollback path, instead of doing a
detach/attach cycle. It adds per-RXQ drain_stats and a retiring flag, so
that retiring RX queues stop writing the shared per-index counters; the
private counters are folded into rxq_stats_ret[] later. It also gates
mana_select_queue() on port_is_up and keeps carrier off after a failed
rollback.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 744d5ebe8dbbb..c80a06ffbe508 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -4053,9 +4155,305 @@ int mana_alloc_qset(struct mana_port_context *apc,

[ ... ]

> +int mana_publish_qset(struct mana_port_context *apc, struct mana_qset *newq,
> +		      struct mana_qset *out_old)
> +{
> +	struct net_device *ndev = apc->ndev;
> +	int err;
> +
> +	ASSERT_RTNL();

[ ... ]

> +	netif_tx_disable(ndev);
> +
> +	mana_qset_snapshot(apc, out_old);
> +
> +	/* Mark before the grace period so old completions cannot wake the
> +	 * replacement's stopped queue.
> +	 */
> +	mana_qset_set_retiring(out_old, newq, true);

[Severity: Low]
Can this freeze the visible RX counters for the whole TX drain wait?

This call sets rxq->retiring on the old RX queues before synchronize_net()
and mana_wait_qset_txqs(), and that wait can last up to 120 seconds. RSS
still sends all RX traffic to these old queues during the wait.

Once retiring is set, mana_rxq_stats() picks the private counters:

    return READ_ONCE(rxq->retiring) ? &rxq->drain_stats : rxq->stats;

>From then on, updates from mana_rx_skb(), mana_run_xdp() and
mana_process_rx_cqe() go into rxq->drain_stats. mana_get_stats64() and the
ethtool stats code read only rxq_stats[] and rxq_stats_ret[]. The counters
users see therefore stop moving until mana_fold_qset_rx_stats() or
mana_destroy_rxq() folds them in later.

mana_update_rx_dim() also returns early for retiring RX queues, so adaptive
RX moderation stops for the same window.

The window is usually a few milliseconds, but it reaches 120 seconds on the
drain timeout path. The same pattern stays in place for full rebuilds and
for the retiring tail in channel reductions later in the series.

The early marking only seems needed for the TX wakeup race. Could the RX
retiring flag be set later, just before mana_config_rss() steers traffic to
the replacement queues, while the TX retiring flag stays where it is?

> +
> +	/* Drain TX/XDP readers past the gate and polls missing retiring. */
> +	synchronize_net();
> +
> +	err = mana_wait_qset_txqs(apc);
> +	if (err)
> +		goto resume_old;

[Severity: Low]
This isn't a bug introduced by this patch, but while mana_wait_qset_txqs()
waits up to 120 seconds with every TX queue stopped and carrier still on,
dev_watchdog (watchdog_timeo is 15*HZ) can call mana_tx_timeout(). That
queues queue_reset_work, which then runs after mana_publish_qset() returns.

The baseline path had the same window. mana_detach() called
netif_tx_disable(), and mana_dealloc_queues() then waited up to 120 seconds
under RTNL with carrier on.

On the drain timeout path here, the reset only runs after newq has been
freed, so it does not invalidate the replacement queues. In effect it is
the existing recovery for TX queues that really are stuck.

Is that interaction intended for the new publish path?

[ ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 43c5c3bf5c408..913349edfe13c 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c

[ ... ]

> +	err = mana_alloc_qset(apc, scratch, new_count, apc->rx_queue_size,
> +			      apc->tx_queue_size, apc->priv_flags, &newq);
> +	if (err)
> +		goto free_scratch;

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch.
mana_alloc_qset() always resets the replacement set's indirection table
with mana_rss_table_init(scratch). A live set_channels therefore discards
a user-configured RSS indirection table without calling
ethtool_rxfh_indir_lost().

The baseline mana_alloc_queues() did the same on every detach/attach.

A later patch in the series fixes this ("net: mana: keep a user-configured
RSS table across a queue rebuild"). It adds mana_rss_table_keep() to the
split, grow and full-rebuild allocators and calls ethtool_rxfh_indir_lost()
after a successful publish.

[ ... ]

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