[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

[sashiko-bot]

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 <hamzamahfooz@linux.microsoft.com>

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?

>  	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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012193325.4071665-1-hamzamahfooz@linux.microsoft.com?part=1