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

[Jesper Dangaard Brouer <hawk@kernel.org>]

On 19/03/2026 11.04, Toke Høiland-Jørgensen wrote:
> Jesper Dangaard Brouer <hawk@kernel.org> writes:
> 
>> On 18/03/2026 15.28, Toke Høiland-Jørgensen wrote:
>>> hawk@kernel.org writes:
>>>
>>>> From: Jesper Dangaard Brouer <hawk@kernel.org>
>>>>
>>>> Add BQL support to the veth driver to dynamically limit the number of
>>>> bytes queued in the ptr_ring, giving the qdisc earlier feedback to shape
>>>> traffic and reduce latency.
>>>>
>>>> The BQL charge (netdev_tx_sent_queue) is placed in veth_xmit() BEFORE
>>>> veth_forward_skb() produces the SKB into the ptr_ring. This ordering is
>>>> critical: with threaded NAPI the consumer runs on a separate CPU and can
>>>> complete the SKB (calling dql_completed) before veth_xmit() returns. If
>>>> the charge happened after the produce, the completion could race ahead
>>>> of the charge, violating dql_completed()'s invariant that completed
>>>> bytes never exceed queued bytes (BUG_ON).
>>>>
>>>> Whether an SKB was BQL-charged is tracked per-SKB using a VETH_BQL_FLAG
>>>> bit in the ptr_ring pointer (BIT(1), alongside the existing VETH_XDP_FLAG
>>>> BIT(0)). The do_bql flag from veth_xmit() propagates through
>>>> veth_forward_skb() and veth_xdp_rx() into the ptr_ring entry. On the
>>>> completion side in veth_xdp_rcv(), veth_ptr_is_bql() reads the flag to
>>>> decide whether to call netdev_tx_completed_queue(). Per-SKB tracking is
>>>> necessary because the qdisc can be replaced live (e.g. noqueue->sfq or
>>>> vice versa via 'tc qdisc replace') while SKBs are already in-flight in
>>>> the ptr_ring. SKBs charged under the old qdisc must complete correctly
>>>> regardless of what qdisc is attached when the consumer runs, so each
>>>> SKB carries its own BQL-charged state rather than re-checking the peer's
>>>> qdisc at completion time.
>>>
>>> It's not completely obvious to me why BQL can't be active regardless of
>>> whether there's a qdisc installed or not? If there's no qdisc, shouldn't
>>> BQL auto-tune to a higher value because the queue runs empty more?
>>>
>>
>> When net_device don't have qdisc we hit this code path:
>>    - [0]
>> https://elixir.bootlin.com/linux/v7.0-rc4/source/net/core/dev.c#L4806-L4852
>>    - Notice the check if(!netif_xmit_stopped(txq))
>>    - resulting in "Virtual device %s asks to queue packet!"
>>
>> We cannot unconditionally track BQL as calling netdev_tx_sent_queue()
>> can result in setting STACK_XOFF.  Resulting in above code dropping
>> packets and complaining.  (It have no qdisc to requeue store back-
>> pressured packet).
> 
> Ah, right. I realised the packet would be dropped, of course, but I did
> not realise the stack would complain. That seems... odd? Why not just
> get rid of the complaint instead of having this kludge to work around
> it?
> 

The BQL code manipulates txq (struct dql) and this requires correct
locking. Using noqueue on veth doesn't do the correct locking.

In the linked[0] code you were likely fooled by this code line[1]:
  HARD_TX_LOCK(dev, txq, cpu);
It is actually not taking the lock, because veth is a lltx device.
(Don't be fooled by the annotation it is for WARN_CONTEXT_ANALYSIS)

For fun I actually implemented it to see what happened. And I did manage
to crash the kernel on the DQL internal BUG_ON.  Analyzing the BUG_ON I
realized that my scheme of charging BQL and then undo the charge (if
ptr_ring were full) isn't correct, there is a race.  The BQL call
netdev_tx_completed_queue() is strictly to be used by the consumer, not
by the producer (like I did).

I'm now working on different approaches for the undo...

--Jesper

[1] https://elixir.bootlin.com/linux/v7.0-rc4/source/net/core/dev.c#L4831
[2] 
https://elixir.bootlin.com/linux/v7.0-rc4/source/include/linux/compiler-context-analysis.h#L166