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

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

[Hamza Mahfooz]

MANA doesn't support byte queue limits, so each TX queue can hold 256
WQEs by default, and up to 16384, of up to 64 KB TSO packets, before the
qdisc layer sees any backpressure. This adds latency to all the flows
sharing a queue, and defeats AQM qdiscs such as fq_codel, which rely on
BQL to keep the driver queues short.

Report the bytes queued and completed to BQL. XDP_TX and XDP_REDIRECT
packets go through the same SQs, so they are accounted too. BQL doesn't
drop or delay them, since the XDP paths send even when the queue is
stopped, it only holds back the stack's packets. For the packets sent on
a stopped queue, only count the bytes: stopping the queue again would
refresh its trans_start, so XDP traffic would keep the TX watchdog from
firing.

Reset the BQL state when destroying the queues, once their NAPI is
disabled, since the SKBs still pending after a drain timeout are freed
without being completed.

Lost TX completions can now stop a queue sooner: it also stops when the
bytes in flight exceed the BQL limit, not only when the SQ is full, and
the limit may never grow enough to cover the lost bytes. The TX watchdog
covers the queues stopped by BQL, and its handler resets the queues
along with their BQL state.

Tested on two Standard_D16ds_v6 Azure VMs in a proximity placement
group, where the throughput is capped at 10.6 Gbps, with the DUT
alternately booting net-next (c1c1f0a31712) with and without this patch
applied.

The throughput stays at the cap with 1, 8 and 64 TCP flows, and the idle
TCP_RR latency doesn't change (p50 of 47 us with vs. without the patch).
With 16 senders of 64-byte UDP packets, the DUT sends 3.7 to 4.0 Mpps
without the patch and 3.7 to 4.1 Mpps with it. With bulk TCP flows
sharing the queues, the netperf TCP_RR latency drops (medians of 6
runs):

  16 queues, 64 flows: p50 4649 -> 2182 us, p99 5621 -> 2677 us
  1 queue, 8 flows:    p50 2554 ->  418 us, p99 2723 ->  624 us

The standing queue moves from the SQs to fq_codel, which now drops
packets: about 2500 qdisc drops and 88K TCP retransmissions per 48 s run
with 64 flows, against at most 29 retransmissions before. The
retransmissions are about 0.2% of the segments sent. The TCP_RR flows
lose no packets, and the packets of the bulk flows are dropped before
they reach the wire, so the goodput changes by less than 0.01%.

Signed-off-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>
---
v2: Only count the bytes of the packets sent on a stopped queue, so that
    XDP traffic doesn't keep the TX watchdog from firing on a queue
    stopped by BQL. (Sashiko)
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 21 ++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 489558176a5d..7f081c2d16d0 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);
+
 	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);
 
 	/* Ensure checking txq_stopped before apc->port_is_up. */
@@ -2694,6 +2706,13 @@ static void mana_destroy_txq(struct mana_port_context *apc)
 			apc->tx_qp[i]->txq.napi_initialized = false;
 		}
 
+		/* Forget the SKBs freed without being completed after a drain
+		 * timeout, now that NAPI can't complete any. Doing it here also
+		 * covers the queues that a lower channel count won't recreate,
+		 * which could otherwise stay stopped.
+		 */
+		netdev_tx_reset_queue(apc->tx_qp[i]->txq.net_txq);
+
 		if (apc->tx_qp[i]->tx_object != INVALID_MANA_HANDLE)
 			mana_destroy_wq_obj(apc, GDMA_SQ, apc->tx_qp[i]->tx_object);
 
-- 
2.56.0

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

[netdev-bot+sashiko]

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

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

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).

- [High] mana: false positive TX watchdog timeout on XDP due to bypassed
  trans_start
- [Medium] The v2 change says it keeps XDP traffic from holding off the
  TX watchdog on a queue stopped by BQL, but XDP traffic can still do so…
- [Low] The bytes-only BQL branch in mana_start_xmit() is meant for XDP
  sends on an already-stopped queue, but it also catches the stack…
- [Low] mana_poll_tx_cq() has WARN_ON_ONCE() early returns inside its
  completion loop: !completions[i].is_sq (line 1984), a bad client_type…

--

net: mana: add BQL support

This adds byte queue limits support to the mana driver. Bytes are reported
as queued in mana_start_xmit() and as completed in mana_poll_tx_cq(). On
an already stopped queue only the bytes are counted, 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 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.
> +	 */

[Severity: Medium]
Does this fully stop XDP traffic from holding off the TX watchdog?

A few lines earlier in mana_start_xmit(), the driver stops the queue
whenever the SQ is low. It doesn't check whether the queue is already
stopped, and it runs before the post error is checked:

	err = mana_gd_post_work_request(gdma_sq, &pkg.wqe_req,
					(struct gdma_posted_wqe_info *)skb->cb);
	if (!mana_can_tx(gdma_sq)) {
		netif_tx_stop_queue(net_txq);
		apc->eth_stats.stop_queue++;
	}

netif_tx_stop_queue() always does:

	WRITE_ONCE(dev_queue->trans_start, jiffies);

Say TX completions stop arriving. BQL sets STACK_XOFF, but XDP_TX and
XDP_REDIRECT frames keep reaching mana_start_xmit() through mana_xdp_tx().
mana_move_wq_tail() never runs, so XDP fills the SQ.

After that, each XDP attempt either takes the last slot or fails with
-ENOSPC in mana_gd_post_work_request(), since monitor_avl_buf is set. Both
cases go through netif_tx_stop_queue() and refresh trans_start, so
dev_watchdog() never sees:

	time_after(jiffies, trans_start + dev->watchdog_timeo)

Then mana_tx_timeout() never schedules the reset that clears the BQL
state. Stack traffic on that queue stays stopped as long as XDP frames
keep arriving.

The pre-patch driver had the same DRV_XOFF refresh when completions were
lost entirely. Still, the commit message says "The TX watchdog covers the
queues stopped by BQL", and the v2 note says "so that XDP traffic doesn't
keep the TX watchdog from firing on a queue stopped by BQL".

Does that only hold until the SQ fills? Should the stop and its
trans_start refresh also be skipped when the queue is already stopped or
the post failed?

> +	if (netif_xmit_stopped(net_txq))
> +		__netdev_tx_sent_queue(net_txq, len, true);
> +	else
> +		netdev_tx_sent_queue(net_txq, len);

[Severity: High]
Can this branch make dev_watchdog() fire on a queue whose completions
are still arriving normally?

XDP_TX and XDP_REDIRECT frames reach mana_start_xmit() without looking
at STACK_XOFF. Under sustained XDP load they can keep the dql in-flight
count above its limit.

Once a stack packet crosses the limit, netdev_tx_sent_queue() writes
trans_start and sets __QUEUE_STATE_STACK_XOFF. From then on
netif_xmit_stopped() is true, so every XDP frame takes this branch and
only calls dql_queued(). trans_start is never refreshed.

The netdev_tx_completed_queue() call in mana_poll_tx_cq() does not
clear STACK_XOFF either. While XDP keeps refilling the queue,
dql_avail() stays negative after each completion batch.

If the SQ is large enough, for example after a larger ring is set with
ethtool -G, mana_can_tx() never goes false. Then netif_tx_stop_queue()
is never called, and nothing else touches trans_start.

The hardware is fine and completions keep coming. Still, after
watchdog_timeo, dev_watchdog() sees a stopped queue with a stale
trans_start and calls mana_tx_timeout(). The queue reset tears down and
recreates the port. As long as the XDP load continues, the same thing
happens again on the next timeout.

This is the other side of the trade-off made in v2. Refreshing
trans_start on every XDP send hides a real stall. Never refreshing it
turns a queue that is only kept over the BQL limit by XDP into a false
timeout. Stack traffic on that queue is also held off by STACK_XOFF the
whole time, so the reset is only the visible symptom.

Should XDP frames be kept out of the BQL accounting on both the sent
and completed side, for example by marking them when they are posted
and skipping their bytes in mana_poll_tx_cq()? Or is there another way
to tell a queue that BQL keeps stopped because of XDP from one whose
completions were lost?

[Severity: Low]
Is this branch only reached by XDP sends on a queue that was already
stopped?

Take a stack packet whose post leaves the SQ low. The
netif_tx_stop_queue() earlier in the same mana_start_xmit() call has just
set DRV_XOFF. QUEUE_STATE_ANY_XOFF includes DRV_XOFF, so
netif_xmit_stopped() returns true here.

That packet then goes through __netdev_tx_sent_queue(net_txq, len, true),
which only calls dql_queued(). It skips the over-limit check in
netdev_tx_sent_queue() that sets __QUEUE_STATE_STACK_XOFF.

If that packet pushed BQL over its limit, STACK_XOFF is never set. On the
next completion, netdev_tx_completed_queue() returns early because
dql_avail() < 0. The DRV_XOFF wake path in mana_poll_tx_cq() then restarts
the queue while BQL is still over its limit.

So each time the SQ fills, one extra stack packet gets past the BQL limit,
even though the comment above describes this branch as XDP-only. Could the
stopped state be checked before the driver's own netif_tx_stop_queue()
call?

> +
>  	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);

[Severity: Low]
Can BQL's in-flight byte count leak when mana_poll_tx_cq() returns early?
The completion loop has several WARN_ON_ONCE() early returns:

		if (WARN_ON_ONCE(!completions[i].is_sq))
			return;
		...
		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;

There is also the client_type check inside the loop and the
wqe_unit_cnt == 0 check after it.

When one of these fires at iteration k, the earlier iterations have
already added their lengths to tx_bytes and freed their SKBs with
napi_consume_skb(). This netdev_tx_completed_queue() call is skipped.

Those bytes then count as in flight in the dql until
netdev_tx_reset_queue() runs in mana_destroy_txq(). That shrinks the
usable BQL budget and can set STACK_XOFF while the SQ still has room,
leaving recovery to the watchdog and a port reset.

Before this patch, these early returns already skipped mana_move_wq_tail()
and the pending_sends decrement, but the dql imbalance is new. Would
calling netdev_tx_completed_queue() for the SKBs already freed, for
example from a common exit label, keep BQL balanced?

> +
>  	/* Ensure tail updated before checking q stop */
>  	smp_mb();
>  

[ ... ]

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