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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Resetting the peer's TX queue during NAPI deletion fails to explicitly schedule the qdisc if it was stopped solely by BQL, potentially stranding buffered packets indefinitely.
- [High] veth_napi_del_range() conditionally skips waking the peer's stopped TX queue when the local device is going down, permanently leaking the DRV_XOFF stop state on the peer.
- [Medium] DRV_XOFF queue wakeups are bypassed for deactivated TX queues, leading to permanently stuck queues if reactivated.
--

commit 1b05a5559481675536cd1059fcf1bd7e7b6e721c
Author: Jesper Dangaard Brouer <hawk@kernel.org>
Subject: veth: implement Byte Queue Limits (BQL) for latency reduction

This commit adds Byte Queue Limits (BQL) support to veth to give qdiscs 
visibility into the ptr_ring queue depth, reducing base latency. It charges 
a fixed unit per packet under the producer lock and resets BQL state on 
NAPI deletion.

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

[ ... ]

> @@ -1074,6 +1118,7 @@ static int __veth_napi_enable(struct net_device *dev)
>  static void veth_napi_del_range(struct net_device *dev, int start, int end)
>  {
>  	struct veth_priv *priv = netdev_priv(dev);
> +	struct net_device *peer;
>  	int i;
>  
>  	for (i = start; i < end; i++) {
> @@ -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);
> +
> +		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);

Does netdev_tx_reset_queue() clear STACK_XOFF without scheduling the qdisc?

If netif_tx_wake_queue() only schedules the qdisc when DRV_XOFF was set,
wouldn't a queue stopped solely by BQL (STACK_XOFF set, DRV_XOFF clear) be
stalled indefinitely here?

By checking netif_running(dev), the wakeup is skipped when the local device
is brought down. Does this permanently leak the DRV_XOFF state on the peer?
If the local device is brought back up later, it appears the peer's TX queue
would remain wedged.

Additionally, if a user reduces the peer's active TX queues (e.g., via
ethtool), the loop bound peer->real_num_tx_queues will skip deactivated
queues.

If those inactive queues had DRV_XOFF set, do they retain that state
permanently when reactivated later, since BQL resets on real_num changes
but DRV_XOFF does not?

> +		}
> +	}
> +
>  	for (i = start; i < end; i++) {
>  		page_pool_destroy(priv->rq[i].page_pool);

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