Re: [PATCH net-next v7 0/5] veth: add Byte Queue Limits (BQL) support

[Simon Schippers <simon.schippers@tu-dortmund.de>]

On 9/16/26 08:45, Jesper Dangaard Brouer wrote:
> 
> 
> On 9/15/26 16:30, Simon Schippers wrote:
>> On 8/10/26 15:25, Simon Schippers wrote:
>>> On 6/12/26 10:35, hawk@kernel.org wrote:
>>>> From: Jesper Dangaard Brouer <hawk@kernel.org>
>>>>
>>>> This series adds BQL (Byte Queue Limits) to the veth driver, reducing
>>>> latency by dynamically limiting in-flight packets in the ptr_ring and
>>>> moving buffering into the qdisc where AQM algorithms can act on it.
>>>
>>> Hi :)
>>>
>>> I worked on my implementation of DQL coalescing that lives in
>>> dynamic_queue_limits.{h,c} and wanted to share it so we can consider it
>>> for the next cycle. This is because in the next cycle I would
>>> like to add BQL support for tun/tap as well, in addition to veth.
>>> Is that fine for you?
>>>
>>> I think it is in good shape. It uses the same logic as the v7, but
>>> every new field fits inside the existing dql struct, and drivers only
>>> need to call the usual netdev_tx_sent_queue() and
>>> netdev_tx_completed_queue() to use it. Patch 3, 5 and 6 are the same
>>> as before, only 1, 2 and 4 are new. Benchmarks looked fine for me.
>>>
>>> There should be no regressions for other DQL/BQL users. I paid close
>>> attention not to break the dql cache lines or other logic.
>>> coal_usecs is now configurable per queue via sysfs and also via ethtool
>>> as usual.
>>>
>>> While working on this, I found a missing barrier in v7:
>>> There was no smp_rmb() pairing the smp_wmb() in __ptr_ring_produce()
>>> before dql_completed() reads dql->num_queued. This happens to be safe
>>> on x86, but on other platforms the read of dql->num_queued could be
>>> reordered before __ptr_ring_consume(), triggering a BUG_ON() in
>>> dql_completed(). Fixed by adding the missing smp_rmb() in veth_xdp_rcv()
>>> before completing.
>>>
>>> Would love to hear your thoughts on the implementation!
>>>
>>> Thanks,
>>> Simon
>>
>> Hi! Any thoughts on this? Do you want to continue this series?
> 
> Appreciate getting poked :-)
> 
> I will not have time to work on this until after October 5th.

Noted, no problem.

> 
> If you Simon have time, feel free to submit a V8 patchset with the
> barrier fix mentioned above.  I should have cycles to review and ACK
> (except between 26 sep to Oct 4).

I would then also move the coalescing logic to
dynamic_queue_limits.{h,c}, ok?

I am convinced that it is the right decision, because it:
- saves us the new struct veth_bql_state, everything fits nicely into
  the dql struct in the existing cachelines
- allows other software interfaces like tun/tap to use the same logic,
  which is my goal :-) others could be e.g. wireguard and ifb
- allows to change coal_usecs per queue

> 
> We are still interested in getting this merged. Notice the bug fix from
> Jonas 60db47f02bfa ("veth: fix queue index used to wake the peer txq in
> veth_poll").  We are running XDP on our veth production interfaces, so
> we didn't notice this.  Still, we are currently waiting for this fix to
> get fully rolled out, before proceeding with the BQL variant.

Yes, I saw that. Probably did not happen to me because I was always
using single queue..

Thanks.