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

[Jesper Dangaard Brouer]

On 08/06/2026 12.38, Simon Schippers wrote:
> On 5/27/26 15:54, hawk@xxxxxxxxxx wrote:
> > From: Simon Schippers <simon.schippers@xxxxxxxxxxxxxx>
> >
> > Per-packet BQL completion forces DQL to converge on limit=2, causing
> > excessive NAPI scheduling overhead and qdisc requeues.
> >
> > Accumulate BQL completions and flush them when a configurable time
> > threshold is exceeded, letting DQL discover a limit that bounds actual
> > queuing delay to the configured interval. Coalescing state persists
> > across NAPI polls in struct veth_rq so completions can accumulate
> > beyond a single budget=64 cycle.
> >
> > Add ethtool tx-usecs support for runtime tuning. Default is 100 us;
> > setting tx-usecs to 0 disables coalescing and falls back to per-packet
> > completion.
> >
> >    ethtool -C <veth-dev> tx-usecs 500  # 500us coalescing
> >    ethtool -C <veth-dev> tx-usecs 0    # per-packet (no coalescing)
> >
> > Co-developed-by: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
> > Signed-off-by: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
> > Signed-off-by: Simon Schippers <simon.schippers@xxxxxxxxxxxxxx>
> > ---
>
> I found the issue that n_bql may become infinitly large if producer
> and consumer have the same speed (and tx_usecs is large). It could
> cause a potential BUG_ON if n_bql grows beyond INT_MAX...
> Also I figured that no hardware BQL driver ever completes more than
> BQL limit many elements.
>
> Therefore, I propose a simpler logic (see attachment) that completes
> either on the usual bql_flush_ns or if n_bql > dql.limit.
> If n_bql > dql.limit then we either have the case above that the
> producer is as fast as the consumer or we have BQL starvation.
>
> if (state->time + bql_flush_ns <= current_time ||
> 	state->n_bql > peer_txq->dql.limit) {
>
> It must be n_bql *bigger than* dql.limit because the producer will
> always exceed the limit before it stops, see netdev_tx_sent_queue().
> It is fast because peer_txq->dql.limit is in the cacheline of the
> completion path, see dynamic_queue_limits.h.
>
> Another advantage is that we avoid the snippet checking for empty
> and BQL stopped which requires an smp_rmb() and an test_bit().
>
> Apart from that I:
> - Always call veth_bql_maybe_complete() in the for loop to have
>    more accurate completion intervals when having mixed XDP and
>    non-XDP packets.
> - Made it so tx_usecs = 0 is now also a normal case.
> - Change the type of n_bql to uint instead of int.
> - Added _ONCE() for tx_coalesce_usecs as suggested by Paolo.
> - Moved the bql_state init in __veth_napi_enable_range() in front
>    of napi_enable() to avoid a race (Sashiko).
> - Moved the bql_state reset in veth_napi_del_range() after the
>    ptr_ring_cleanup() (probably does not matter but makes sense to me)
>
> Benchmarks look just fine, see commit message.
>
> WDYT?

Looks good to me, I will use this in my V7 patchset.

A bike-shedding issue: We change the coalescing parameters for the veth
net_device, but should this be a TX or RX parameter?

For physical NICs adjusting TX coalescing will affect the BQL as this
affect the TX completion of the transmitted packets. For veth, it is the
veth-peer's RX NAPI that is "completing" or emptying the ptr_ring, which
is where this patch adds netdev_tx_completed_queue calls for BQL.
Any opinions on the "TX" or "RX" color?

--Jesper