Re: [PATCH net-next v7 5/5] veth: time-based BQL completion coalescing via ethtool tx-usecs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] BQL completions are stranded when NAPI goes to sleep if the timer has not expired, artificially destroying DQL performance estimates.
- [Low] Direct access to `peer_txq->dql.limit` breaks compilation when CONFIG_BQL is disabled.
--

commit 7aba7f3c0951cf87cb2ef78e981e1f66a316a71d
Author: Simon Schippers <simon.schippers@tu-dortmund.de>

veth: time-based BQL completion coalescing via ethtool tx-usecs

This patch accumulates BQL completions and flushes them when a configurable
time threshold (tx-usecs) is exceeded. This lets DQL discover a limit that
bounds actual queuing delay to the configured interval.

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 2473f730734b7..c62d87a8402cb 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c

[ ... ]

> @@ -969,13 +1029,54 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
>  	return NULL;
>  }
>  
> +static void veth_bql_maybe_complete(struct veth_bql_state *state,
> +				    struct netdev_queue *peer_txq,
> +				    u64 bql_flush_ns)
> +{
> +	u64 current_time;
> +
> +	/* There is no reason to complete with 0 and
> +	 * peer_txq could go away.
> +	 */
> +	if (!state->n_bql || !peer_txq)
> +		return;
> +
> +	current_time = sched_clock();
> +
> +	/* We complete if:
> +	 * 1. We reach bql_flush_ns.
> +	 * 2. We potentially have BQL starvation.
> +	 */
> +	if (state->time + bql_flush_ns <= current_time ||
> +	    state->n_bql > peer_txq->dql.limit) {

[Severity: Low]
Does this direct access to peer_txq->dql.limit cause compilation errors when
CONFIG_BQL is disabled? The dql member of struct netdev_queue is
conditionally compiled, so accessing it directly might break the build.

> +		netdev_tx_completed_queue(peer_txq, state->n_bql,
> +					  state->n_bql * VETH_BQL_UNIT);
> +		state->time = current_time;
> +		state->n_bql = 0;
> +	}
> +}
> +
>  static int veth_xdp_rcv(struct veth_rq *rq, int budget,
>  			struct veth_xdp_tx_bq *bq,
>  			struct veth_stats *stats,
>  			struct netdev_queue *peer_txq)

[ ... ]

> @@ -1000,12 +1101,11 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
>  			}
>  		} else {
>  			/* ndo_start_xmit */
> -			bool bql_charged = veth_ptr_is_bql(ptr);
>  			struct sk_buff *skb = veth_ptr_to_skb(ptr);
>  
> +			if (veth_ptr_is_bql(ptr))
> +				state->n_bql++;
>  			stats->xdp_bytes += skb->len;
> -			if (peer_txq && bql_charged)
> -				netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);
>  
>  			skb = veth_xdp_rcv_skb(rq, skb, bq, stats);
>  			if (skb) {
> @@ -1015,6 +1115,7 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
>  					napi_gro_receive(&rq->xdp_napi, skb);
>  			}
>  		}
> +		veth_bql_maybe_complete(state, peer_txq, bql_flush_ns);
>  		done++;
>  	}

[Severity: High]
If veth_xdp_rcv() finishes and returns a done count less than the budget,
NAPI will go to sleep in veth_poll(). Do we need to unconditionally flush
any stranded BQL completions in veth_poll() before sleeping?

If completions are left in rq->bql_state indefinitely across NAPI idle
periods, it might present an artificially massive delay to DQL. This could
cause DQL to mistakenly conclude the hardware is extremely slow and
aggressively shrink dql.limit to its minimum, crippling throughput on
subsequent bursts.

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