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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Jesper Dangaard Brouer <hawk@kernel.org> writes:

> 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...

Alright, good to hear that my obtuse questions turned out to be helpful ;)

-Toke