Re: [PATCH net-next v7 04/13] net: mana: swap queue sets in mana_set_channels
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-20 16:47:51
Also in:
bpf, linux-hyperv, linux-rdma, lkml
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.
quoted hunk
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
[ ... ]
quoted hunk
@@ -4053,9 +4155,305 @@ int mana_alloc_qset(struct mana_port_context *apc,
[ ... ]
quoted hunk
+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();
[ ... ]
quoted hunk
+ 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?
quoted hunk
+ + /* 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? [ ... ]
quoted hunk
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
[ ... ]
quoted hunk
+ 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