Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] u64_stats_update_begin called with preemption enabled causes potential deadlock. Pre-existing issues: - [High] Missing Mutual Exclusion on tx_stats updates between mana_xdp_xmit and mana_start_xmit. -- commit 45f5d2a315921074d282869dd20e8233e5bb463b Author: Long Li <[email protected]> Subject: net: mana: keep per-queue statistics in the port context Move RX/TX counters into port-lifetime arrays so queue rebuilds do not reset them. Sum all max_queues slots for ndo_get_stats64(), keep the current queue count for ethtool, and fold retiring RX queues count into private drain_stats upon destruction. > diff --git a/drivers/net/ethernet/microsoft/mana/mana_bpf.c > b/drivers/net/ethernet/microsoft/mana/mana_bpf.c > index debf0e7765862..2060e08d82277 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_bpf.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_bpf.c [ ... ] > @@ -73,7 +73,7 @@ int mana_xdp_xmit(struct net_device *ndev, int n, struct > xdp_frame **frames, > count++; > } > > - tx_stats = &apc->tx_qp[q_idx]->txq.stats; > + tx_stats = apc->tx_qp[q_idx]->txq.stats; > > u64_stats_update_begin(&tx_stats->syncp); > tx_stats->xdp_xmit += count; > u64_stats_update_end(&tx_stats->syncp); [Severity: High] This is a pre-existing issue, but is there a missing mutual exclusion between mana_xdp_xmit() and mana_start_xmit() when updating tx_stats->syncp? mana_xdp_xmit() bypasses the qdisc HARD_TX_LOCK and maps q_idx using smp_processor_id(). As a result, it can run concurrently with mana_start_xmit() which uses the skb's queue mapping. Because the skb queue mapping could map to the same queue from another CPU, both functions might run simultaneously on different CPUs and call u64_stats_update_begin() on the same syncp. This would corrupt the sequence counter. > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c > b/drivers/net/ethernet/microsoft/mana/mana_en.c > index 5752e3745ccc6..98007bb3ef7ca 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c [ ... ] > @@ -575,7 +575,7 @@ netdev_tx_t mana_start_xmit(struct sk_buff *skb, struct > net_device *ndev) > /* Populated the packet and bytes counters based on post GSO packet > * calculations > */ > - tx_stats = &txq->stats; > + tx_stats = txq->stats; > u64_stats_update_begin(&tx_stats->syncp); > tx_stats->packets += num_gso_seg; > tx_stats->bytes += len + ((num_gso_seg - 1) * gso_hs); [Severity: High] This is a pre-existing issue, but this is the primary packet transmission path that can run concurrently with the XDP transmission path in mana_xdp_xmit() mentioned above, contributing to the lost updates on the shared tx_stats struct. [ ... ] > +static void mana_fold_rxq_stats(struct mana_port_context *apc, > + struct mana_rxq *rxq) > +{ > + struct mana_stats_rx *src = &rxq->drain_stats; > + struct mana_stats_rx *dst; > + unsigned int i; > + > + ASSERT_RTNL(); > + > + if (!apc->rxq_stats_ret || rxq->rxq_idx >= apc->max_queues) > + return; > + > + dst = &apc->rxq_stats_ret[rxq->rxq_idx]; > + > + u64_stats_update_begin(&dst->syncp); > + dst->packets += src->packets; > + dst->bytes += src->bytes; [Severity: High] Does mana_fold_rxq_stats() need to disable preemption or bottom halves before acquiring the sequence lock here? This function executes in process context holding the RTNL lock, but it calls u64_stats_update_begin() with preemption enabled. On 32-bit systems, if the thread is preempted or interrupted by a softirq on the same CPU that subsequently reads the stats via mana_get_stats64(), the reader could spin infinitely waiting for the sequence lock to become even. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
