Re: [PATCH net-next v6 2/5] veth: implement Byte Queue Limits (BQL) for latency reduction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Flawed queue wakeup logic in veth_napi_del_range() causes permanent TX stalls on the peer device by failing to schedule STACK_XOFF clearances.
- [High] BQL state permanently leaks for deactivated TX queues, causing a permanent stall if they are later reactivated.
--

commit 01e0529ce363cdc7993646292c97c9feb7c956f9
Author: Jesper Dangaard Brouer <hawk@kernel.org>

veth: implement Byte Queue Limits (BQL) for latency reduction

This commit implements Byte Queue Limits (BQL) support for veth to provide
backpressure and reduce queuing latency. It introduces a fixed unit charge
per packet under the ptr_ring producer_lock and tracks per-SKB BQL state to
safely complete it on the RX side.

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 0cfb19b760dd5..21ff785339431 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c

[ ... ]

> @@ -928,9 +968,13 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
>  			}
>  		} else {
>  			/* ndo_start_xmit */
> -			struct sk_buff *skb = ptr;
> +			bool bql_charged = veth_ptr_is_bql(ptr);
> +			struct sk_buff *skb = veth_ptr_to_skb(ptr);
>  
>  			stats->xdp_bytes += skb->len;
> +			if (peer_txq && bql_charged)
> +				netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);

[Severity: High]
If the user decreases peer->real_num_tx_queues, veth_poll() will set peer_txq
to NULL for queues that are now deactivated. Will this skip completions for
in-flight packets that originated from those deactivated queues?

If the completions are permanently leaked, won't the BQL state remain corrupt
and immediately halt the queue if it is later reactivated?

> +
>  			skb = veth_xdp_rcv_skb(rq, skb, bq, stats);
>  			if (skb) {
>  				if (skb_shared(skb) || skb_unclone(skb, GFP_ATOMIC))

[ ... ]

> @@ -1092,6 +1137,24 @@ static void veth_napi_del_range(struct net_device *dev, int start, int end)
>  		ptr_ring_cleanup(&rq->xdp_ring, veth_ptr_free);
>  	}
>  
> +	/* Reset BQL and wake stopped peer txqs.  A concurrent veth_xmit()
> +	 * may have set DRV_XOFF between rcu_assign_pointer(napi, NULL) and
> +	 * synchronize_net(), and NAPI can no longer clear it.
> +	 * Only wake when the device is still up.
> +	 */
> +	peer = rtnl_dereference(priv->peer);
> +	if (peer) {
> +		int peer_end = min_t(int, end, peer->real_num_tx_queues);

[Severity: High]
Similar to the issue in veth_xdp_rcv(), bounding this loop to
peer->real_num_tx_queues skips resetting the BQL state for deactivated queues.
Could this leave the state permanently corrupted if those queues are later
reactivated?

> +
> +		for (i = start; i < peer_end; i++) {
> +			struct netdev_queue *txq = netdev_get_tx_queue(peer, i);
> +
> +			netdev_tx_reset_queue(txq);
> +			if (netif_running(dev))
> +				netif_tx_wake_queue(txq);

[Severity: High]
Does this sequence cause a permanent TX stall if the queue was stopped
purely by BQL?

netdev_tx_reset_queue() clears the __QUEUE_STATE_STACK_XOFF bit. However,
netif_tx_wake_queue() only schedules the Qdisc if __QUEUE_STATE_DRV_XOFF
was set. If the queue was only stopped by BQL, the Qdisc won't be scheduled,
leaving it asleep with no stop bits set.

Also, if the device is being brought down, netif_running(dev) evaluates to
false, skipping the wake. Could this leave __QUEUE_STATE_DRV_XOFF set on the
peer, resulting in a 16-second watchdog timeout when the device is later
brought back up?

> +		}
> +	}
> +
>  	for (i = start; i < end; i++) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527135418.1166665-1-hawk@kernel.org?part=2