Re: [PATCH net-next] net: mana: add BQL support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] If TX completions are lost, XDP traffic can keep a BQL-stopped
  MANA queue from ever being recovered by the TX watchdog.

--

net: mana: add BQL support

This patch adds byte queue limit accounting to MANA. Queued bytes are
reported in mana_start_xmit() and completed bytes in mana_poll_tx_cq().
XDP_TX and XDP_REDIRECT sends are counted through the same path, and the
BQL state is reset in mana_destroy_txq() after NAPI is disabled.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 2ba48a1c4b15e..67048fe64d63e 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -543,6 +543,11 @@ 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.
> +	 */
> +	netdev_tx_sent_queue(net_txq, len);
> +

[Severity: Medium]
Can XDP traffic stop the TX watchdog from ever recovering a queue that
BQL has stopped? The commit message says:

  The TX watchdog covers the queues stopped by BQL, and its handler resets
  the queues along with their BQL state.

XDP_TX and XDP_REDIRECT (mana_xdp_xmit()->mana_xdp_xmit_fm()) both reach
mana_start_xmit() through mana_xdp_tx(). That function does not check
whether the queue is stopped:

drivers/net/ethernet/microsoft/mana/mana_bpf.c:mana_xdp_tx() {
    ...
	__netif_tx_lock(ndevtxq, smp_processor_id());

	rc = mana_start_xmit(skb, ndev);
    ...
}

With this patch, each of those sends now calls netdev_tx_sent_queue().
If the queue is over its limit, netdev_tx_sent_queue() always refreshes
trans_start, even when __QUEUE_STATE_STACK_XOFF is already set:

include/linux/netdevice.h:netdev_tx_sent_queue() {
    ...
	/* Paired with READ_ONCE() from dev_watchdog() */
	WRITE_ONCE(dev_queue->trans_start, jiffies);
    ...
	set_bit(__QUEUE_STATE_STACK_XOFF, &dev_queue->state);
    ...
}

dev_watchdog() only fires when trans_start is older than watchdog_timeo.
MANA sets watchdog_timeo to 15 * HZ. One XDP packet every 15 seconds on
that queue seems to be enough to stop mana_tx_timeout() from ever
running.

In that state, what clears STACK_XOFF? mana_poll_tx_cq() matches CQEs to
SKBs in FIFO order with skb_dequeue(). If completions are lost, the lost
bytes become a permanent debt D in num_queued - num_completed.
netdev_tx_completed_queue() only clears STACK_XOFF when dql_avail() >= 0,
so the limit would have to reach D.

In dql_completed(), the limit only grows here:

	if ((ovlimit && !inprogress) ||
	    (dql->prev_ovlimit && all_prev_completed)) {

The first condition can't be true while inprogress >= D. The second
needs one poll to complete at least the bytes that were in flight at the
previous poll. A low-rate XDP flow with one completion per poll won't
meet that.

For example, take equal R-byte packets, a learned limit of R, and two
lost completions (D = 2R). The limit never grows, and the decrease
branch can shrink it further.

netif_tx_wake_queue() only clears DRV_XOFF. The only
netdev_tx_reset_queue() call is in mana_destroy_txq().

Would stack traffic on that queue then stay blocked indefinitely while
XDP traffic keeps flowing?

Hardware faults are not the only source of lost completions. The early
returns in mana_poll_tx_cq() exit after some SKBs have already been
dequeued and freed, but before netdev_tx_completed_queue() runs:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_poll_tx_cq() {
    ...
		if (WARN_ON_ONCE(txq->gdma_txq_id != completions[i].wq_num))
			return;

		skb = skb_dequeue(&txq->pending_skbs);
		if (WARN_ON_ONCE(!skb))
			return;
    ...
}

Before this patch, a few leaked WQEs did not block stack traffic on the
queue as long as the SQ was not full.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008152302.3192778-1-hamzamahfooz%40linux.microsoft.com