On Tue, Oct 13, 2026 at 07:35:27PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
> - [High] mana: false positive TX watchdog timeout on XDP due to bypassed
> trans_start
> - [High] net: mana: missing memory barrier allows Dekker's algorithm race
> stalling TX queue
> --
>
> commit 9321c04ed33bbb92ceb52c62c2cef3faad2705da
> Author: Hamza Mahfooz <[email protected]>
>
> net: mana: add BQL support
>
> The patch introduces Byte Queue Limits (BQL) support to the MANA network
> driver to prevent high latency and improve AQM interactions.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 489558176a5de..7f081c2d16d03 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -543,6 +543,16 @@ netdev_tx_t mana_start_xmit(struct sk_buff *skb,
> > struct net_device *ndev)
> > err = NETDEV_TX_OK;
> > atomic_inc(&txq->pending_sends);
> >
> > + /* Account the skb in BQL before the doorbell lets the hardware
> > + * complete it. The XDP paths send even when the queue is stopped,
> > + * only count the bytes then, since stopping the queue again would
> > + * refresh its trans_start and keep the TX watchdog from firing.
> > + */
> > + if (netif_xmit_stopped(net_txq))
> > + __netdev_tx_sent_queue(net_txq, len, true);
> > + else
> > + netdev_tx_sent_queue(net_txq, len);
> > +
>
> [Severity: High]
> Could this logic cause a false positive TX watchdog timeout on XDP workloads?
>
> When XDP traffic pushes packets directly, it ignores BQL limits. If the
> traffic sustains without hitting the physical queue limit, trans_start is
> never updated here since __netdev_tx_sent_queue(net_txq, len, true) is
> called.
>
> Because the physical limit isn't hit, netif_tx_stop_queue() is also never
> called, leaving trans_start entirely frozen. Will this trigger a watchdog
> timeout after 5 seconds?
>
Ya, this does seem founded, I'm siding with just keeping XDP out of the
BQL equation entirely.
> > mana_gd_wq_ring_doorbell(gd->gdma_context, gdma_sq);
> >
> > /* skb may be freed after mana_gd_post_work_request. Do not use it. */
>
> [ ... ]
>
> > @@ -2038,10 +2048,12 @@ static void mana_poll_tx_cq(struct mana_cq *cq)
> > gdma_wq = txq->gdma_sq;
> > avail_space = mana_gd_wq_avail_space(gdma_wq);
> >
> > + net_txq = txq->net_txq;
> > + netdev_tx_completed_queue(net_txq, pkt_transmitted, tx_bytes);
> > +
> > /* Ensure tail updated before checking q stop */
> > smp_mb();
> >
> > - net_txq = txq->net_txq;
> > txq_stopped = netif_tx_queue_stopped(net_txq);
>
> [Severity: High]
> Does this memory barrier fully prevent a Dekker's algorithm race with the TX
> thread?
>
> In mana_start_xmit(), netif_tx_stop_queue() stops the queue using set_bit(),
> which is atomic but doesn't imply a memory ordering barrier. It then checks
> hardware space again:
>
> mana_start_xmit()
> if (!mana_can_tx(gdma_sq)) {
> netif_tx_stop_queue(net_txq);
> ...
> }
> ...
> if (netif_tx_queue_stopped(net_txq) && mana_can_tx(gdma_sq)) {
> netif_tx_wake_queue(net_txq);
> ...
> }
>
> Because the TX thread lacks an smp_mb() or smp_mb__after_atomic(),
> architectures with relaxed memory ordering could reorder the read of the tail
> pointer before the store to the queue state.
>
> If that read is hoisted, both threads could miss each other's updates:
>
> - TX thread sees an old full queue.
> - CQ thread sees an unstopped queue.
>
> Could this leave the queue permanently stopped?
No, the patch doesn't modify the driver's stop and wake logic, and
BQL stops and wakes the queue with its own barriers, in
netdev_tx_sent_queue() and netdev_tx_completed_queue().
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=1
