mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL)
@ 2025-11-04 16:13 Simon Schippers
  2025-11-04 16:13 ` [PATCH net-next v1 1/1] " Simon Schippers
  2025-11-04 17:02 ` [PATCH net-next v1 0/1] " Eric Dumazet
  0 siblings, 2 replies; 21+ messages in thread
From: Simon Schippers @ 2025-11-04 16:13 UTC (permalink / raw)
  To: oneukum, andrew+netdev, davem, edumazet, kuba, pabeni, netdev,
	linux-usb, linux-kernel, therbert
  Cc: Simon Schippers

During recent testing, I observed significant latency spikes when using
Quectel 5G modems under load. Investigation revealed that the issue was
caused by bufferbloat in the usbnet driver.

In the current implementation, usbnet uses a fixed tx_qlen of:

USB2: 60 * 1518 bytes = 91.08 KB
USB3: 60 * 5 * 1518 bytes = 454.80 KB

Such large transmit queues can be problematic, especially for cellular
modems. For example, with a typical celluar link speed of 10 Mbit/s, a
fully occupied USB3 transmit queue results in:

454.80 KB / (10 Mbit/s / 8 bit/byte) = 363.84 ms

of additional latency.

To address this issue, this patch introduces support for
Byte Queue Limits (BQL) [1][2] in the usbnet driver. BQL dynamically
limits the amount of data queued in the driver, effectively reducing
latency without impacting throughput.
This implementation was successfully tested on several devices as
described in the commit.



Future work

Due to offloading, TCP often produces SKBs up to 64 KB in size. To
further decrease buffer bloat, I tried to disable TSO, GSO and LRO but it
did not have the intended effect in my tests. The only dirty workaround I
found so far was to call netif_stop_queue() whenever BQL sets
__QUEUE_STATE_STACK_XOFF. However, a proper solution to this issue would
be desirable.

I also plan to publish a scientific paper on this topic in the near
future.

Thanks,
Simon

[1] https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83
[2] https://lwn.net/Articles/469652/

Simon Schippers (1):
  usbnet: Add support for Byte Queue Limits (BQL)

 drivers/net/usb/usbnet.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-04 16:13 [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL) Simon Schippers
@ 2025-11-04 16:13 ` Simon Schippers
  2025-11-04 17:00   ` Eric Dumazet
  2025-11-04 17:02 ` [PATCH net-next v1 0/1] " Eric Dumazet
  1 sibling, 1 reply; 21+ messages in thread
From: Simon Schippers @ 2025-11-04 16:13 UTC (permalink / raw)
  To: oneukum, andrew+netdev, davem, edumazet, kuba, pabeni, netdev,
	linux-usb, linux-kernel, therbert
  Cc: Simon Schippers

The usbnet driver currently relies on fixed transmit queue lengths, which
can lead to bufferbloat and large latency spikes under load -
particularly with cellular modems.
This patch adds support for Byte Queue Limits (BQL) to dynamically manage
the transmit queue size and reduce latency without sacrificing
throughput.

Testing was performed on various devices using the usbnet driver for
packet transmission:

- DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
- DELOCK 61969: USB2 to 1 GbE adapter (asix)
- Quectel RM520: 5G modem (qmi_wwan)
- USB2 Android tethering (cdc_ncm)

No performance degradation was observed for iperf3 TCP or UDP traffic,
while latency for a prioritized ping application was significantly
reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
utilized by iperf3 UDP traffic, the prioritized ping was improved from
1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
connection, the prioritized ping was improved from 35 ms to 5 ms.

Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
---
 drivers/net/usb/usbnet.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 62a85dbad31a..1994f03a78ad 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
 
 	clear_bit(EVENT_DEV_OPEN, &dev->flags);
 	netif_stop_queue (net);
+	netdev_reset_queue(net);
 
 	netif_info(dev, ifdown, dev->net,
 		   "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
@@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
 	}
 
 	set_bit(EVENT_DEV_OPEN, &dev->flags);
+	netdev_reset_queue(net);
 	netif_start_queue (net);
 	netif_info(dev, ifup, dev->net,
 		   "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
@@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
 	case 0:
 		netif_trans_update(net);
 		__usbnet_queue_skb(&dev->txq, skb, tx_start);
+		netdev_sent_queue(net, skb->len);
 		if (dev->txq.qlen >= TX_QLEN (dev))
 			netif_stop_queue (net);
 	}
@@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
 static void usbnet_bh(struct timer_list *t)
 {
 	struct usbnet		*dev = timer_container_of(dev, t, delay);
+	unsigned int bytes_compl = 0, pkts_compl = 0;
 	struct sk_buff		*skb;
 	struct skb_data		*entry;
 
@@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
 				usb_free_skb(skb);
 			continue;
 		case tx_done:
+			bytes_compl += skb->len;
+			pkts_compl++;
 			kfree(entry->urb->sg);
 			fallthrough;
 		case rx_cleanup:
@@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
 		}
 	}
 
+	netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
+
 	/* restart RX again after disabling due to high error rate */
 	clear_bit(EVENT_RX_KILL, &dev->flags);
 
-- 
2.43.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-04 16:13 ` [PATCH net-next v1 1/1] " Simon Schippers
@ 2025-11-04 17:00   ` Eric Dumazet
  2025-11-05 10:35     ` Simon Schippers
  0 siblings, 1 reply; 21+ messages in thread
From: Eric Dumazet @ 2025-11-04 17:00 UTC (permalink / raw)
  To: Simon Schippers
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
<simon.schippers@tu-dortmund.de> wrote:
>
> The usbnet driver currently relies on fixed transmit queue lengths, which
> can lead to bufferbloat and large latency spikes under load -
> particularly with cellular modems.
> This patch adds support for Byte Queue Limits (BQL) to dynamically manage
> the transmit queue size and reduce latency without sacrificing
> throughput.
>
> Testing was performed on various devices using the usbnet driver for
> packet transmission:
>
> - DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
> - DELOCK 61969: USB2 to 1 GbE adapter (asix)
> - Quectel RM520: 5G modem (qmi_wwan)
> - USB2 Android tethering (cdc_ncm)
>
> No performance degradation was observed for iperf3 TCP or UDP traffic,
> while latency for a prioritized ping application was significantly
> reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
> utilized by iperf3 UDP traffic, the prioritized ping was improved from
> 1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
> connection, the prioritized ping was improved from 35 ms to 5 ms.
>
> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
> ---
>  drivers/net/usb/usbnet.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 62a85dbad31a..1994f03a78ad 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
>
>         clear_bit(EVENT_DEV_OPEN, &dev->flags);
>         netif_stop_queue (net);
> +       netdev_reset_queue(net);
>
>         netif_info(dev, ifdown, dev->net,
>                    "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
> @@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
>         }
>
>         set_bit(EVENT_DEV_OPEN, &dev->flags);
> +       netdev_reset_queue(net);
>         netif_start_queue (net);
>         netif_info(dev, ifup, dev->net,
>                    "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
> @@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
>         case 0:
>                 netif_trans_update(net);
>                 __usbnet_queue_skb(&dev->txq, skb, tx_start);
> +               netdev_sent_queue(net, skb->len);
>                 if (dev->txq.qlen >= TX_QLEN (dev))
>                         netif_stop_queue (net);
>         }
> @@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
>  static void usbnet_bh(struct timer_list *t)
>  {
>         struct usbnet           *dev = timer_container_of(dev, t, delay);
> +       unsigned int bytes_compl = 0, pkts_compl = 0;
>         struct sk_buff          *skb;
>         struct skb_data         *entry;
>
> @@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
>                                 usb_free_skb(skb);
>                         continue;
>                 case tx_done:
> +                       bytes_compl += skb->len;
> +                       pkts_compl++;
>                         kfree(entry->urb->sg);
>                         fallthrough;
>                 case rx_cleanup:
> @@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
>                 }
>         }
>
> +       netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> +
>         /* restart RX again after disabling due to high error rate */
>         clear_bit(EVENT_RX_KILL, &dev->flags);
>

I think this is racy. usbnet_bh() can run from two different contexts,
at the same time (from two cpus)

1) From process context :
usbnet_bh_work()

2) From a timer. (dev->delay)


To use BQL, you will need to add mutual exclusion.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-04 16:13 [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL) Simon Schippers
  2025-11-04 16:13 ` [PATCH net-next v1 1/1] " Simon Schippers
@ 2025-11-04 17:02 ` Eric Dumazet
  2025-11-05 10:40   ` Simon Schippers
  1 sibling, 1 reply; 21+ messages in thread
From: Eric Dumazet @ 2025-11-04 17:02 UTC (permalink / raw)
  To: Simon Schippers
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
<simon.schippers@tu-dortmund.de> wrote:
>
> During recent testing, I observed significant latency spikes when using
> Quectel 5G modems under load. Investigation revealed that the issue was
> caused by bufferbloat in the usbnet driver.
>
> In the current implementation, usbnet uses a fixed tx_qlen of:
>
> USB2: 60 * 1518 bytes = 91.08 KB
> USB3: 60 * 5 * 1518 bytes = 454.80 KB
>
> Such large transmit queues can be problematic, especially for cellular
> modems. For example, with a typical celluar link speed of 10 Mbit/s, a
> fully occupied USB3 transmit queue results in:
>
> 454.80 KB / (10 Mbit/s / 8 bit/byte) = 363.84 ms
>
> of additional latency.

Doesn't 5G need to push more packets to the driver to get good aggregation ?

>
> To address this issue, this patch introduces support for
> Byte Queue Limits (BQL) [1][2] in the usbnet driver. BQL dynamically
> limits the amount of data queued in the driver, effectively reducing
> latency without impacting throughput.
> This implementation was successfully tested on several devices as
> described in the commit.
>
>
>
> Future work
>
> Due to offloading, TCP often produces SKBs up to 64 KB in size.

Only for rates > 500 Mbit. After BQL, we had many more improvements in
the stack.
https://lwn.net/Articles/564978/


> To
> further decrease buffer bloat, I tried to disable TSO, GSO and LRO but it
> did not have the intended effect in my tests. The only dirty workaround I
> found so far was to call netif_stop_queue() whenever BQL sets
> __QUEUE_STATE_STACK_XOFF. However, a proper solution to this issue would
> be desirable.
>
> I also plan to publish a scientific paper on this topic in the near
> future.
>
> Thanks,
> Simon
>
> [1] https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83
> [2] https://lwn.net/Articles/469652/
>
> Simon Schippers (1):
>   usbnet: Add support for Byte Queue Limits (BQL)
>
>  drivers/net/usb/usbnet.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> --
> 2.43.0
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 10:40   ` Simon Schippers
@ 2025-11-05 10:35     ` Daniele Palmas
  2025-11-05 11:04       ` Simon Schippers
  2025-11-05 11:34     ` Eric Dumazet
  1 sibling, 1 reply; 21+ messages in thread
From: Daniele Palmas @ 2025-11-05 10:35 UTC (permalink / raw)
  To: Simon Schippers
  Cc: Eric Dumazet, oneukum, andrew+netdev, davem, kuba, pabeni,
	netdev, linux-usb, linux-kernel

Hello Simon,

Il giorno mer 5 nov 2025 alle ore 11:40 Simon Schippers
<simon.schippers@tu-dortmund.de> ha scritto:
>
> On 11/4/25 18:02, Eric Dumazet wrote:
> > On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
> > <simon.schippers@tu-dortmund.de> wrote:
> >>
> >> During recent testing, I observed significant latency spikes when using
> >> Quectel 5G modems under load. Investigation revealed that the issue was
> >> caused by bufferbloat in the usbnet driver.
> >>
> >> In the current implementation, usbnet uses a fixed tx_qlen of:
> >>
> >> USB2: 60 * 1518 bytes = 91.08 KB
> >> USB3: 60 * 5 * 1518 bytes = 454.80 KB
> >>
> >> Such large transmit queues can be problematic, especially for cellular
> >> modems. For example, with a typical celluar link speed of 10 Mbit/s, a
> >> fully occupied USB3 transmit queue results in:
> >>
> >> 454.80 KB / (10 Mbit/s / 8 bit/byte) = 363.84 ms
> >>
> >> of additional latency.
> >
> > Doesn't 5G need to push more packets to the driver to get good aggregation ?
> >
>
> Yes, but not 455 KB for low speeds. 5G requires a queue of a few ms to
> aggregate enough packets for a frame but not of several hundred ms as
> calculated in my example. And yes, there are situations where 5G,
> especially FR2 mmWave, reaches Gbit/s speeds where a big queue is
> required. But the dynamic queue limit approach of BQL should be well
> suited for these varying speeds.
>

out of curiosity, related to the test with 5G Quectel, did you test
enabling aggregation through QMAP (kernel module rmnet) or simply
qmi_wwan raw_ip ?

Regards,
Daniele

> >>
> >> To address this issue, this patch introduces support for
> >> Byte Queue Limits (BQL) [1][2] in the usbnet driver. BQL dynamically
> >> limits the amount of data queued in the driver, effectively reducing
> >> latency without impacting throughput.
> >> This implementation was successfully tested on several devices as
> >> described in the commit.
> >>
> >>
> >>
> >> Future work
> >>
> >> Due to offloading, TCP often produces SKBs up to 64 KB in size.
> >
> > Only for rates > 500 Mbit. After BQL, we had many more improvements in
> > the stack.
> > https://lwn.net/Articles/564978/
> >
> >
>
> I also saw these large SKBs, for example, for my USB2 Android tethering,
> which advertises a network speed of < 500 Mbit/s.
> I saw these large SKBs by looking at the file:
>
> cat /sys/class/net/INTERFACE/queues/tx-0/byte_queue_limits/inflight
>
> For UDP-only traffic, inflight always maxed out at MTU size.
>
> Thank you for your replies!
>
> >> To
> >> further decrease buffer bloat, I tried to disable TSO, GSO and LRO but it
> >> did not have the intended effect in my tests. The only dirty workaround I
> >> found so far was to call netif_stop_queue() whenever BQL sets
> >> __QUEUE_STATE_STACK_XOFF. However, a proper solution to this issue would
> >> be desirable.
> >>
> >> I also plan to publish a scientific paper on this topic in the near
> >> future.
> >>
> >> Thanks,
> >> Simon
> >>
> >> [1] https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83
> >> [2] https://lwn.net/Articles/469652/
> >>
> >> Simon Schippers (1):
> >>   usbnet: Add support for Byte Queue Limits (BQL)
> >>
> >>  drivers/net/usb/usbnet.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> --
> >> 2.43.0
> >>
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-04 17:00   ` Eric Dumazet
@ 2025-11-05 10:35     ` Simon Schippers
  2025-11-05 11:28       ` Eric Dumazet
  0 siblings, 1 reply; 21+ messages in thread
From: Simon Schippers @ 2025-11-05 10:35 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On 11/4/25 18:00, Eric Dumazet wrote:
> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
> <simon.schippers@tu-dortmund.de> wrote:
>>
>> The usbnet driver currently relies on fixed transmit queue lengths, which
>> can lead to bufferbloat and large latency spikes under load -
>> particularly with cellular modems.
>> This patch adds support for Byte Queue Limits (BQL) to dynamically manage
>> the transmit queue size and reduce latency without sacrificing
>> throughput.
>>
>> Testing was performed on various devices using the usbnet driver for
>> packet transmission:
>>
>> - DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
>> - DELOCK 61969: USB2 to 1 GbE adapter (asix)
>> - Quectel RM520: 5G modem (qmi_wwan)
>> - USB2 Android tethering (cdc_ncm)
>>
>> No performance degradation was observed for iperf3 TCP or UDP traffic,
>> while latency for a prioritized ping application was significantly
>> reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
>> utilized by iperf3 UDP traffic, the prioritized ping was improved from
>> 1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
>> connection, the prioritized ping was improved from 35 ms to 5 ms.
>>
>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
>> ---
>>  drivers/net/usb/usbnet.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 62a85dbad31a..1994f03a78ad 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
>>
>>         clear_bit(EVENT_DEV_OPEN, &dev->flags);
>>         netif_stop_queue (net);
>> +       netdev_reset_queue(net);
>>
>>         netif_info(dev, ifdown, dev->net,
>>                    "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
>> @@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
>>         }
>>
>>         set_bit(EVENT_DEV_OPEN, &dev->flags);
>> +       netdev_reset_queue(net);
>>         netif_start_queue (net);
>>         netif_info(dev, ifup, dev->net,
>>                    "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
>> @@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
>>         case 0:
>>                 netif_trans_update(net);
>>                 __usbnet_queue_skb(&dev->txq, skb, tx_start);
>> +               netdev_sent_queue(net, skb->len);
>>                 if (dev->txq.qlen >= TX_QLEN (dev))
>>                         netif_stop_queue (net);
>>         }
>> @@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
>>  static void usbnet_bh(struct timer_list *t)
>>  {
>>         struct usbnet           *dev = timer_container_of(dev, t, delay);
>> +       unsigned int bytes_compl = 0, pkts_compl = 0;
>>         struct sk_buff          *skb;
>>         struct skb_data         *entry;
>>
>> @@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
>>                                 usb_free_skb(skb);
>>                         continue;
>>                 case tx_done:
>> +                       bytes_compl += skb->len;
>> +                       pkts_compl++;
>>                         kfree(entry->urb->sg);
>>                         fallthrough;
>>                 case rx_cleanup:
>> @@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
>>                 }
>>         }
>>
>> +       netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>> +
>>         /* restart RX again after disabling due to high error rate */
>>         clear_bit(EVENT_RX_KILL, &dev->flags);
>>
> 
> I think this is racy. usbnet_bh() can run from two different contexts,
> at the same time (from two cpus)
> 
> 1) From process context :
> usbnet_bh_work()
> 
> 2) From a timer. (dev->delay)
> 
> 
> To use BQL, you will need to add mutual exclusion.

Yeah, I missed that.

I guess synchronizing with the lock of the sk_buff_head dev->done makes
sense? The same locking is also done right before in skb_dequeue.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-04 17:02 ` [PATCH net-next v1 0/1] " Eric Dumazet
@ 2025-11-05 10:40   ` Simon Schippers
  2025-11-05 10:35     ` Daniele Palmas
  2025-11-05 11:34     ` Eric Dumazet
  0 siblings, 2 replies; 21+ messages in thread
From: Simon Schippers @ 2025-11-05 10:40 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On 11/4/25 18:02, Eric Dumazet wrote:
> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
> <simon.schippers@tu-dortmund.de> wrote:
>>
>> During recent testing, I observed significant latency spikes when using
>> Quectel 5G modems under load. Investigation revealed that the issue was
>> caused by bufferbloat in the usbnet driver.
>>
>> In the current implementation, usbnet uses a fixed tx_qlen of:
>>
>> USB2: 60 * 1518 bytes = 91.08 KB
>> USB3: 60 * 5 * 1518 bytes = 454.80 KB
>>
>> Such large transmit queues can be problematic, especially for cellular
>> modems. For example, with a typical celluar link speed of 10 Mbit/s, a
>> fully occupied USB3 transmit queue results in:
>>
>> 454.80 KB / (10 Mbit/s / 8 bit/byte) = 363.84 ms
>>
>> of additional latency.
> 
> Doesn't 5G need to push more packets to the driver to get good aggregation ?
> 

Yes, but not 455 KB for low speeds. 5G requires a queue of a few ms to
aggregate enough packets for a frame but not of several hundred ms as
calculated in my example. And yes, there are situations where 5G,
especially FR2 mmWave, reaches Gbit/s speeds where a big queue is
required. But the dynamic queue limit approach of BQL should be well
suited for these varying speeds.

>>
>> To address this issue, this patch introduces support for
>> Byte Queue Limits (BQL) [1][2] in the usbnet driver. BQL dynamically
>> limits the amount of data queued in the driver, effectively reducing
>> latency without impacting throughput.
>> This implementation was successfully tested on several devices as
>> described in the commit.
>>
>>
>>
>> Future work
>>
>> Due to offloading, TCP often produces SKBs up to 64 KB in size.
> 
> Only for rates > 500 Mbit. After BQL, we had many more improvements in
> the stack.
> https://lwn.net/Articles/564978/
> 
> 

I also saw these large SKBs, for example, for my USB2 Android tethering,
which advertises a network speed of < 500 Mbit/s.
I saw these large SKBs by looking at the file:

cat /sys/class/net/INTERFACE/queues/tx-0/byte_queue_limits/inflight

For UDP-only traffic, inflight always maxed out at MTU size.

Thank you for your replies!

>> To
>> further decrease buffer bloat, I tried to disable TSO, GSO and LRO but it
>> did not have the intended effect in my tests. The only dirty workaround I
>> found so far was to call netif_stop_queue() whenever BQL sets
>> __QUEUE_STATE_STACK_XOFF. However, a proper solution to this issue would
>> be desirable.
>>
>> I also plan to publish a scientific paper on this topic in the near
>> future.
>>
>> Thanks,
>> Simon
>>
>> [1] https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83
>> [2] https://lwn.net/Articles/469652/
>>
>> Simon Schippers (1):
>>   usbnet: Add support for Byte Queue Limits (BQL)
>>
>>  drivers/net/usb/usbnet.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> --
>> 2.43.0
>>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 10:35     ` Daniele Palmas
@ 2025-11-05 11:04       ` Simon Schippers
  2025-11-06  8:38         ` Daniele Palmas
  0 siblings, 1 reply; 21+ messages in thread
From: Simon Schippers @ 2025-11-05 11:04 UTC (permalink / raw)
  To: Daniele Palmas
  Cc: Eric Dumazet, oneukum, andrew+netdev, davem, kuba, pabeni,
	netdev, linux-usb, linux-kernel

On 11/5/25 11:35, Daniele Palmas wrote:
> Hello Simon,
> 
> Il giorno mer 5 nov 2025 alle ore 11:40 Simon Schippers
> <simon.schippers@tu-dortmund.de> ha scritto:
>>
>> On 11/4/25 18:02, Eric Dumazet wrote:
>>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>
>>>> During recent testing, I observed significant latency spikes when using
>>>> Quectel 5G modems under load. Investigation revealed that the issue was
>>>> caused by bufferbloat in the usbnet driver.
>>>>
>>>> In the current implementation, usbnet uses a fixed tx_qlen of:
>>>>
>>>> USB2: 60 * 1518 bytes = 91.08 KB
>>>> USB3: 60 * 5 * 1518 bytes = 454.80 KB
>>>>
>>>> Such large transmit queues can be problematic, especially for cellular
>>>> modems. For example, with a typical celluar link speed of 10 Mbit/s, a
>>>> fully occupied USB3 transmit queue results in:
>>>>
>>>> 454.80 KB / (10 Mbit/s / 8 bit/byte) = 363.84 ms
>>>>
>>>> of additional latency.
>>>
>>> Doesn't 5G need to push more packets to the driver to get good aggregation ?
>>>
>>
>> Yes, but not 455 KB for low speeds. 5G requires a queue of a few ms to
>> aggregate enough packets for a frame but not of several hundred ms as
>> calculated in my example. And yes, there are situations where 5G,
>> especially FR2 mmWave, reaches Gbit/s speeds where a big queue is
>> required. But the dynamic queue limit approach of BQL should be well
>> suited for these varying speeds.
>>
> 
> out of curiosity, related to the test with 5G Quectel, did you test
> enabling aggregation through QMAP (kernel module rmnet) or simply
> qmi_wwan raw_ip ?
> 
> Regards,
> Daniele
> 

Hi Daniele,

I simply used qmi_wwan. I actually never touched rmnet before.
Is the aggregation through QMAP what you and Eric mean with aggregation?
Because then I misunderstood it, because I was thinking about aggregating
enough (and not too many) packets in the usbnet queue.

Thanks

>>>>
>>>> To address this issue, this patch introduces support for
>>>> Byte Queue Limits (BQL) [1][2] in the usbnet driver. BQL dynamically
>>>> limits the amount of data queued in the driver, effectively reducing
>>>> latency without impacting throughput.
>>>> This implementation was successfully tested on several devices as
>>>> described in the commit.
>>>>
>>>>
>>>>
>>>> Future work
>>>>
>>>> Due to offloading, TCP often produces SKBs up to 64 KB in size.
>>>
>>> Only for rates > 500 Mbit. After BQL, we had many more improvements in
>>> the stack.
>>> https://lwn.net/Articles/564978/
>>>
>>>
>>
>> I also saw these large SKBs, for example, for my USB2 Android tethering,
>> which advertises a network speed of < 500 Mbit/s.
>> I saw these large SKBs by looking at the file:
>>
>> cat /sys/class/net/INTERFACE/queues/tx-0/byte_queue_limits/inflight
>>
>> For UDP-only traffic, inflight always maxed out at MTU size.
>>
>> Thank you for your replies!
>>
>>>> To
>>>> further decrease buffer bloat, I tried to disable TSO, GSO and LRO but it
>>>> did not have the intended effect in my tests. The only dirty workaround I
>>>> found so far was to call netif_stop_queue() whenever BQL sets
>>>> __QUEUE_STATE_STACK_XOFF. However, a proper solution to this issue would
>>>> be desirable.
>>>>
>>>> I also plan to publish a scientific paper on this topic in the near
>>>> future.
>>>>
>>>> Thanks,
>>>> Simon
>>>>
>>>> [1] https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83
>>>> [2] https://lwn.net/Articles/469652/
>>>>
>>>> Simon Schippers (1):
>>>>   usbnet: Add support for Byte Queue Limits (BQL)
>>>>
>>>>  drivers/net/usb/usbnet.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> --
>>>> 2.43.0
>>>>
>>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 10:35     ` Simon Schippers
@ 2025-11-05 11:28       ` Eric Dumazet
  2025-11-05 12:20         ` Simon Schippers
  0 siblings, 1 reply; 21+ messages in thread
From: Eric Dumazet @ 2025-11-05 11:28 UTC (permalink / raw)
  To: Simon Schippers
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On Wed, Nov 5, 2025 at 2:35 AM Simon Schippers
<simon.schippers@tu-dortmund.de> wrote:
>
> On 11/4/25 18:00, Eric Dumazet wrote:
> > On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
> > <simon.schippers@tu-dortmund.de> wrote:
> >>
> >> The usbnet driver currently relies on fixed transmit queue lengths, which
> >> can lead to bufferbloat and large latency spikes under load -
> >> particularly with cellular modems.
> >> This patch adds support for Byte Queue Limits (BQL) to dynamically manage
> >> the transmit queue size and reduce latency without sacrificing
> >> throughput.
> >>
> >> Testing was performed on various devices using the usbnet driver for
> >> packet transmission:
> >>
> >> - DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
> >> - DELOCK 61969: USB2 to 1 GbE adapter (asix)
> >> - Quectel RM520: 5G modem (qmi_wwan)
> >> - USB2 Android tethering (cdc_ncm)
> >>
> >> No performance degradation was observed for iperf3 TCP or UDP traffic,
> >> while latency for a prioritized ping application was significantly
> >> reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
> >> utilized by iperf3 UDP traffic, the prioritized ping was improved from
> >> 1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
> >> connection, the prioritized ping was improved from 35 ms to 5 ms.
> >>
> >> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
> >> ---
> >>  drivers/net/usb/usbnet.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> >> index 62a85dbad31a..1994f03a78ad 100644
> >> --- a/drivers/net/usb/usbnet.c
> >> +++ b/drivers/net/usb/usbnet.c
> >> @@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
> >>
> >>         clear_bit(EVENT_DEV_OPEN, &dev->flags);
> >>         netif_stop_queue (net);
> >> +       netdev_reset_queue(net);
> >>
> >>         netif_info(dev, ifdown, dev->net,
> >>                    "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
> >> @@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
> >>         }
> >>
> >>         set_bit(EVENT_DEV_OPEN, &dev->flags);
> >> +       netdev_reset_queue(net);
> >>         netif_start_queue (net);
> >>         netif_info(dev, ifup, dev->net,
> >>                    "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
> >> @@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
> >>         case 0:
> >>                 netif_trans_update(net);
> >>                 __usbnet_queue_skb(&dev->txq, skb, tx_start);
> >> +               netdev_sent_queue(net, skb->len);
> >>                 if (dev->txq.qlen >= TX_QLEN (dev))
> >>                         netif_stop_queue (net);
> >>         }
> >> @@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
> >>  static void usbnet_bh(struct timer_list *t)
> >>  {
> >>         struct usbnet           *dev = timer_container_of(dev, t, delay);
> >> +       unsigned int bytes_compl = 0, pkts_compl = 0;
> >>         struct sk_buff          *skb;
> >>         struct skb_data         *entry;
> >>
> >> @@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
> >>                                 usb_free_skb(skb);
> >>                         continue;
> >>                 case tx_done:
> >> +                       bytes_compl += skb->len;
> >> +                       pkts_compl++;
> >>                         kfree(entry->urb->sg);
> >>                         fallthrough;
> >>                 case rx_cleanup:
> >> @@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
> >>                 }
> >>         }
> >>
> >> +       netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> >> +
> >>         /* restart RX again after disabling due to high error rate */
> >>         clear_bit(EVENT_RX_KILL, &dev->flags);
> >>
> >
> > I think this is racy. usbnet_bh() can run from two different contexts,
> > at the same time (from two cpus)
> >
> > 1) From process context :
> > usbnet_bh_work()
> >
> > 2) From a timer. (dev->delay)
> >
> >
> > To use BQL, you will need to add mutual exclusion.
>
> Yeah, I missed that.
>
> I guess synchronizing with the lock of the sk_buff_head dev->done makes
> sense? The same locking is also done right before in skb_dequeue.

Or only protect the netdev_completed_queue(dev->net, pkts_compl,
bytes_compl) call,
adding a specific/dedicated spinlock for this purpose.

spin_lock_bh(&dev->bql_spinlock);
netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
spin_unlock_bh(&dev->bql_spinlock);

I am assuming no usbnet driver is setting dev->lltx = true (or plan to
in the future)
so usbnet_start_xmit() is protected by HARD_TX_LOCK() already.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 10:40   ` Simon Schippers
  2025-11-05 10:35     ` Daniele Palmas
@ 2025-11-05 11:34     ` Eric Dumazet
  1 sibling, 0 replies; 21+ messages in thread
From: Eric Dumazet @ 2025-11-05 11:34 UTC (permalink / raw)
  To: Simon Schippers
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On Wed, Nov 5, 2025 at 2:40 AM Simon Schippers
<simon.schippers@tu-dortmund.de> wrote:
>
> On 11/4/25 18:02, Eric Dumazet wrote:
> > On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
> > <simon.schippers@tu-dortmund.de> wrote:
> >>
> >> During recent testing, I observed significant latency spikes when using
> >> Quectel 5G modems under load. Investigation revealed that the issue was
> >> caused by bufferbloat in the usbnet driver.
> >>
> >> In the current implementation, usbnet uses a fixed tx_qlen of:
> >>
> >> USB2: 60 * 1518 bytes = 91.08 KB
> >> USB3: 60 * 5 * 1518 bytes = 454.80 KB
> >>
> >> Such large transmit queues can be problematic, especially for cellular
> >> modems. For example, with a typical celluar link speed of 10 Mbit/s, a
> >> fully occupied USB3 transmit queue results in:
> >>
> >> 454.80 KB / (10 Mbit/s / 8 bit/byte) = 363.84 ms
> >>
> >> of additional latency.
> >
> > Doesn't 5G need to push more packets to the driver to get good aggregation ?
> >
>
> Yes, but not 455 KB for low speeds. 5G requires a queue of a few ms to
> aggregate enough packets for a frame but not of several hundred ms as
> calculated in my example. And yes, there are situations where 5G,
> especially FR2 mmWave, reaches Gbit/s speeds where a big queue is
> required. But the dynamic queue limit approach of BQL should be well
> suited for these varying speeds.
>
> >>
> >> To address this issue, this patch introduces support for
> >> Byte Queue Limits (BQL) [1][2] in the usbnet driver. BQL dynamically
> >> limits the amount of data queued in the driver, effectively reducing
> >> latency without impacting throughput.
> >> This implementation was successfully tested on several devices as
> >> described in the commit.
> >>
> >>
> >>
> >> Future work
> >>
> >> Due to offloading, TCP often produces SKBs up to 64 KB in size.
> >
> > Only for rates > 500 Mbit. After BQL, we had many more improvements in
> > the stack.
> > https://lwn.net/Articles/564978/
> >
> >
>
> I also saw these large SKBs, for example, for my USB2 Android tethering,
> which advertises a network speed of < 500 Mbit/s.
> I saw these large SKBs by looking at the file:

TCP does not sense the underlying network speed. This would be moot if
a link is shared by one thousand flows...
The rate is determined by CWND * MSS / RTT.
Some congestion controls have a tendency to inflate CWND to a very big
value, hence bufferbloat.
One of BBR goal is to avoid bufferbloat.

BQL is only a part of the solution.

Disabling TSO/GSO is certainly not part of the solution, you can trust
me on this.

>
> cat /sys/class/net/INTERFACE/queues/tx-0/byte_queue_limits/inflight
>
> For UDP-only traffic, inflight always maxed out at MTU size.
>
> Thank you for your replies!
>
> >> To
> >> further decrease buffer bloat, I tried to disable TSO, GSO and LRO but it
> >> did not have the intended effect in my tests. The only dirty workaround I
> >> found so far was to call netif_stop_queue() whenever BQL sets
> >> __QUEUE_STATE_STACK_XOFF. However, a proper solution to this issue would
> >> be desirable.
> >>
> >> I also plan to publish a scientific paper on this topic in the near
> >> future.
> >>
> >> Thanks,
> >> Simon
> >>
> >> [1] https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83
> >> [2] https://lwn.net/Articles/469652/
> >>
> >> Simon Schippers (1):
> >>   usbnet: Add support for Byte Queue Limits (BQL)
> >>
> >>  drivers/net/usb/usbnet.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> --
> >> 2.43.0
> >>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 11:28       ` Eric Dumazet
@ 2025-11-05 12:20         ` Simon Schippers
  2025-11-05 12:34           ` Eric Dumazet
  0 siblings, 1 reply; 21+ messages in thread
From: Simon Schippers @ 2025-11-05 12:20 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On 11/5/25 12:28, Eric Dumazet wrote:
> On Wed, Nov 5, 2025 at 2:35 AM Simon Schippers
> <simon.schippers@tu-dortmund.de> wrote:
>>
>> On 11/4/25 18:00, Eric Dumazet wrote:
>>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>
>>>> The usbnet driver currently relies on fixed transmit queue lengths, which
>>>> can lead to bufferbloat and large latency spikes under load -
>>>> particularly with cellular modems.
>>>> This patch adds support for Byte Queue Limits (BQL) to dynamically manage
>>>> the transmit queue size and reduce latency without sacrificing
>>>> throughput.
>>>>
>>>> Testing was performed on various devices using the usbnet driver for
>>>> packet transmission:
>>>>
>>>> - DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
>>>> - DELOCK 61969: USB2 to 1 GbE adapter (asix)
>>>> - Quectel RM520: 5G modem (qmi_wwan)
>>>> - USB2 Android tethering (cdc_ncm)
>>>>
>>>> No performance degradation was observed for iperf3 TCP or UDP traffic,
>>>> while latency for a prioritized ping application was significantly
>>>> reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
>>>> utilized by iperf3 UDP traffic, the prioritized ping was improved from
>>>> 1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
>>>> connection, the prioritized ping was improved from 35 ms to 5 ms.
>>>>
>>>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
>>>> ---
>>>>  drivers/net/usb/usbnet.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>> index 62a85dbad31a..1994f03a78ad 100644
>>>> --- a/drivers/net/usb/usbnet.c
>>>> +++ b/drivers/net/usb/usbnet.c
>>>> @@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
>>>>
>>>>         clear_bit(EVENT_DEV_OPEN, &dev->flags);
>>>>         netif_stop_queue (net);
>>>> +       netdev_reset_queue(net);
>>>>
>>>>         netif_info(dev, ifdown, dev->net,
>>>>                    "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
>>>> @@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
>>>>         }
>>>>
>>>>         set_bit(EVENT_DEV_OPEN, &dev->flags);
>>>> +       netdev_reset_queue(net);
>>>>         netif_start_queue (net);
>>>>         netif_info(dev, ifup, dev->net,
>>>>                    "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
>>>> @@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
>>>>         case 0:
>>>>                 netif_trans_update(net);
>>>>                 __usbnet_queue_skb(&dev->txq, skb, tx_start);
>>>> +               netdev_sent_queue(net, skb->len);
>>>>                 if (dev->txq.qlen >= TX_QLEN (dev))
>>>>                         netif_stop_queue (net);
>>>>         }
>>>> @@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
>>>>  static void usbnet_bh(struct timer_list *t)
>>>>  {
>>>>         struct usbnet           *dev = timer_container_of(dev, t, delay);
>>>> +       unsigned int bytes_compl = 0, pkts_compl = 0;
>>>>         struct sk_buff          *skb;
>>>>         struct skb_data         *entry;
>>>>
>>>> @@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
>>>>                                 usb_free_skb(skb);
>>>>                         continue;
>>>>                 case tx_done:
>>>> +                       bytes_compl += skb->len;
>>>> +                       pkts_compl++;
>>>>                         kfree(entry->urb->sg);
>>>>                         fallthrough;
>>>>                 case rx_cleanup:
>>>> @@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
>>>>                 }
>>>>         }
>>>>
>>>> +       netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>>> +
>>>>         /* restart RX again after disabling due to high error rate */
>>>>         clear_bit(EVENT_RX_KILL, &dev->flags);
>>>>
>>>
>>> I think this is racy. usbnet_bh() can run from two different contexts,
>>> at the same time (from two cpus)
>>>
>>> 1) From process context :
>>> usbnet_bh_work()
>>>
>>> 2) From a timer. (dev->delay)
>>>
>>>
>>> To use BQL, you will need to add mutual exclusion.
>>
>> Yeah, I missed that.
>>
>> I guess synchronizing with the lock of the sk_buff_head dev->done makes
>> sense? The same locking is also done right before in skb_dequeue.
> 
> Or only protect the netdev_completed_queue(dev->net, pkts_compl,
> bytes_compl) call,
> adding a specific/dedicated spinlock for this purpose.
> 
> spin_lock_bh(&dev->bql_spinlock);
> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> spin_unlock_bh(&dev->bql_spinlock);
> 
> I am assuming no usbnet driver is setting dev->lltx = true (or plan to
> in the future)
> so usbnet_start_xmit() is protected by HARD_TX_LOCK() already.

Yes, I also want to only protect the netdev_completed_queue(dev->net,
pkts_compl, bytes_compl) call. However, I am wondering what you mean with

spin_lock_bh(&dev->bql_spinlock)
...


Do we want to protect against usbnet_start_xmit()? Maybe I am missing
something, but other BQL implementations also do not seem to protect
against their respective ndo_start_xmit.


My approach would just protect against usbnet_bh calls from another
context with the same locking as skb_dequeue():

spin_lock_irqsave(&list->lock, flags);
netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
spin_unlock_irqrestore(&list->lock, flags);

Thanks!

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 12:20         ` Simon Schippers
@ 2025-11-05 12:34           ` Eric Dumazet
  2025-11-05 12:58             ` Simon Schippers
  0 siblings, 1 reply; 21+ messages in thread
From: Eric Dumazet @ 2025-11-05 12:34 UTC (permalink / raw)
  To: Simon Schippers
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On Wed, Nov 5, 2025 at 4:20 AM Simon Schippers
<simon.schippers@tu-dortmund.de> wrote:
>
> On 11/5/25 12:28, Eric Dumazet wrote:
> > On Wed, Nov 5, 2025 at 2:35 AM Simon Schippers
> > <simon.schippers@tu-dortmund.de> wrote:
> >>
> >> On 11/4/25 18:00, Eric Dumazet wrote:
> >>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
> >>> <simon.schippers@tu-dortmund.de> wrote:
> >>>>
> >>>> The usbnet driver currently relies on fixed transmit queue lengths, which
> >>>> can lead to bufferbloat and large latency spikes under load -
> >>>> particularly with cellular modems.
> >>>> This patch adds support for Byte Queue Limits (BQL) to dynamically manage
> >>>> the transmit queue size and reduce latency without sacrificing
> >>>> throughput.
> >>>>
> >>>> Testing was performed on various devices using the usbnet driver for
> >>>> packet transmission:
> >>>>
> >>>> - DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
> >>>> - DELOCK 61969: USB2 to 1 GbE adapter (asix)
> >>>> - Quectel RM520: 5G modem (qmi_wwan)
> >>>> - USB2 Android tethering (cdc_ncm)
> >>>>
> >>>> No performance degradation was observed for iperf3 TCP or UDP traffic,
> >>>> while latency for a prioritized ping application was significantly
> >>>> reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
> >>>> utilized by iperf3 UDP traffic, the prioritized ping was improved from
> >>>> 1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
> >>>> connection, the prioritized ping was improved from 35 ms to 5 ms.
> >>>>
> >>>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
> >>>> ---
> >>>>  drivers/net/usb/usbnet.c | 8 ++++++++
> >>>>  1 file changed, 8 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> >>>> index 62a85dbad31a..1994f03a78ad 100644
> >>>> --- a/drivers/net/usb/usbnet.c
> >>>> +++ b/drivers/net/usb/usbnet.c
> >>>> @@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
> >>>>
> >>>>         clear_bit(EVENT_DEV_OPEN, &dev->flags);
> >>>>         netif_stop_queue (net);
> >>>> +       netdev_reset_queue(net);
> >>>>
> >>>>         netif_info(dev, ifdown, dev->net,
> >>>>                    "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
> >>>> @@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
> >>>>         }
> >>>>
> >>>>         set_bit(EVENT_DEV_OPEN, &dev->flags);
> >>>> +       netdev_reset_queue(net);
> >>>>         netif_start_queue (net);
> >>>>         netif_info(dev, ifup, dev->net,
> >>>>                    "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
> >>>> @@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
> >>>>         case 0:
> >>>>                 netif_trans_update(net);
> >>>>                 __usbnet_queue_skb(&dev->txq, skb, tx_start);
> >>>> +               netdev_sent_queue(net, skb->len);
> >>>>                 if (dev->txq.qlen >= TX_QLEN (dev))
> >>>>                         netif_stop_queue (net);
> >>>>         }
> >>>> @@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
> >>>>  static void usbnet_bh(struct timer_list *t)
> >>>>  {
> >>>>         struct usbnet           *dev = timer_container_of(dev, t, delay);
> >>>> +       unsigned int bytes_compl = 0, pkts_compl = 0;
> >>>>         struct sk_buff          *skb;
> >>>>         struct skb_data         *entry;
> >>>>
> >>>> @@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
> >>>>                                 usb_free_skb(skb);
> >>>>                         continue;
> >>>>                 case tx_done:
> >>>> +                       bytes_compl += skb->len;
> >>>> +                       pkts_compl++;
> >>>>                         kfree(entry->urb->sg);
> >>>>                         fallthrough;
> >>>>                 case rx_cleanup:
> >>>> @@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
> >>>>                 }
> >>>>         }
> >>>>
> >>>> +       netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> >>>> +
> >>>>         /* restart RX again after disabling due to high error rate */
> >>>>         clear_bit(EVENT_RX_KILL, &dev->flags);
> >>>>
> >>>
> >>> I think this is racy. usbnet_bh() can run from two different contexts,
> >>> at the same time (from two cpus)
> >>>
> >>> 1) From process context :
> >>> usbnet_bh_work()
> >>>
> >>> 2) From a timer. (dev->delay)
> >>>
> >>>
> >>> To use BQL, you will need to add mutual exclusion.
> >>
> >> Yeah, I missed that.
> >>
> >> I guess synchronizing with the lock of the sk_buff_head dev->done makes
> >> sense? The same locking is also done right before in skb_dequeue.
> >
> > Or only protect the netdev_completed_queue(dev->net, pkts_compl,
> > bytes_compl) call,
> > adding a specific/dedicated spinlock for this purpose.
> >
> > spin_lock_bh(&dev->bql_spinlock);
> > netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> > spin_unlock_bh(&dev->bql_spinlock);
> >
> > I am assuming no usbnet driver is setting dev->lltx = true (or plan to
> > in the future)
> > so usbnet_start_xmit() is protected by HARD_TX_LOCK() already.
>
> Yes, I also want to only protect the netdev_completed_queue(dev->net,
> pkts_compl, bytes_compl) call. However, I am wondering what you mean with
>
> spin_lock_bh(&dev->bql_spinlock)
> ...
>
>
> Do we want to protect against usbnet_start_xmit()? Maybe I am missing
> something, but other BQL implementations also do not seem to protect
> against their respective ndo_start_xmit.

BQL has been designed so that producer/consumer can run in //

However, all producers need exclusion (typically done with HARD_TX_LOCK)
All consumers need exclusion (typically done because of NAPI sched bit)

>
>
> My approach would just protect against usbnet_bh calls from another
> context with the same locking as skb_dequeue():
>
> spin_lock_irqsave(&list->lock, flags);
> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> spin_unlock_irqrestore(&list->lock, flags);

I tend to prefer not masking hard irq unless really necessary.

Also, reusing  a lock for different purposes makes things confusing
in terms of code maintenance.

usbnet is hardly performance critical, I would keep list->lock only to
protect the list of skbs :)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 12:34           ` Eric Dumazet
@ 2025-11-05 12:58             ` Simon Schippers
  2025-11-05 13:05               ` Eric Dumazet
  0 siblings, 1 reply; 21+ messages in thread
From: Simon Schippers @ 2025-11-05 12:58 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On 11/5/25 13:34, Eric Dumazet wrote:
> On Wed, Nov 5, 2025 at 4:20 AM Simon Schippers
> <simon.schippers@tu-dortmund.de> wrote:
>>
>> On 11/5/25 12:28, Eric Dumazet wrote:
>>> On Wed, Nov 5, 2025 at 2:35 AM Simon Schippers
>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>
>>>> On 11/4/25 18:00, Eric Dumazet wrote:
>>>>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
>>>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>>>
>>>>>> The usbnet driver currently relies on fixed transmit queue lengths, which
>>>>>> can lead to bufferbloat and large latency spikes under load -
>>>>>> particularly with cellular modems.
>>>>>> This patch adds support for Byte Queue Limits (BQL) to dynamically manage
>>>>>> the transmit queue size and reduce latency without sacrificing
>>>>>> throughput.
>>>>>>
>>>>>> Testing was performed on various devices using the usbnet driver for
>>>>>> packet transmission:
>>>>>>
>>>>>> - DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
>>>>>> - DELOCK 61969: USB2 to 1 GbE adapter (asix)
>>>>>> - Quectel RM520: 5G modem (qmi_wwan)
>>>>>> - USB2 Android tethering (cdc_ncm)
>>>>>>
>>>>>> No performance degradation was observed for iperf3 TCP or UDP traffic,
>>>>>> while latency for a prioritized ping application was significantly
>>>>>> reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
>>>>>> utilized by iperf3 UDP traffic, the prioritized ping was improved from
>>>>>> 1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
>>>>>> connection, the prioritized ping was improved from 35 ms to 5 ms.
>>>>>>
>>>>>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
>>>>>> ---
>>>>>>  drivers/net/usb/usbnet.c | 8 ++++++++
>>>>>>  1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>>>> index 62a85dbad31a..1994f03a78ad 100644
>>>>>> --- a/drivers/net/usb/usbnet.c
>>>>>> +++ b/drivers/net/usb/usbnet.c
>>>>>> @@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
>>>>>>
>>>>>>         clear_bit(EVENT_DEV_OPEN, &dev->flags);
>>>>>>         netif_stop_queue (net);
>>>>>> +       netdev_reset_queue(net);
>>>>>>
>>>>>>         netif_info(dev, ifdown, dev->net,
>>>>>>                    "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
>>>>>> @@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
>>>>>>         }
>>>>>>
>>>>>>         set_bit(EVENT_DEV_OPEN, &dev->flags);
>>>>>> +       netdev_reset_queue(net);
>>>>>>         netif_start_queue (net);
>>>>>>         netif_info(dev, ifup, dev->net,
>>>>>>                    "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
>>>>>> @@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
>>>>>>         case 0:
>>>>>>                 netif_trans_update(net);
>>>>>>                 __usbnet_queue_skb(&dev->txq, skb, tx_start);
>>>>>> +               netdev_sent_queue(net, skb->len);
>>>>>>                 if (dev->txq.qlen >= TX_QLEN (dev))
>>>>>>                         netif_stop_queue (net);
>>>>>>         }
>>>>>> @@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
>>>>>>  static void usbnet_bh(struct timer_list *t)
>>>>>>  {
>>>>>>         struct usbnet           *dev = timer_container_of(dev, t, delay);
>>>>>> +       unsigned int bytes_compl = 0, pkts_compl = 0;
>>>>>>         struct sk_buff          *skb;
>>>>>>         struct skb_data         *entry;
>>>>>>
>>>>>> @@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
>>>>>>                                 usb_free_skb(skb);
>>>>>>                         continue;
>>>>>>                 case tx_done:
>>>>>> +                       bytes_compl += skb->len;
>>>>>> +                       pkts_compl++;
>>>>>>                         kfree(entry->urb->sg);
>>>>>>                         fallthrough;
>>>>>>                 case rx_cleanup:
>>>>>> @@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
>>>>>>                 }
>>>>>>         }
>>>>>>
>>>>>> +       netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>>>>> +
>>>>>>         /* restart RX again after disabling due to high error rate */
>>>>>>         clear_bit(EVENT_RX_KILL, &dev->flags);
>>>>>>
>>>>>
>>>>> I think this is racy. usbnet_bh() can run from two different contexts,
>>>>> at the same time (from two cpus)
>>>>>
>>>>> 1) From process context :
>>>>> usbnet_bh_work()
>>>>>
>>>>> 2) From a timer. (dev->delay)
>>>>>
>>>>>
>>>>> To use BQL, you will need to add mutual exclusion.
>>>>
>>>> Yeah, I missed that.
>>>>
>>>> I guess synchronizing with the lock of the sk_buff_head dev->done makes
>>>> sense? The same locking is also done right before in skb_dequeue.
>>>
>>> Or only protect the netdev_completed_queue(dev->net, pkts_compl,
>>> bytes_compl) call,
>>> adding a specific/dedicated spinlock for this purpose.
>>>
>>> spin_lock_bh(&dev->bql_spinlock);
>>> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>> spin_unlock_bh(&dev->bql_spinlock);
>>>
>>> I am assuming no usbnet driver is setting dev->lltx = true (or plan to
>>> in the future)
>>> so usbnet_start_xmit() is protected by HARD_TX_LOCK() already.
>>
>> Yes, I also want to only protect the netdev_completed_queue(dev->net,
>> pkts_compl, bytes_compl) call. However, I am wondering what you mean with
>>
>> spin_lock_bh(&dev->bql_spinlock)
>> ...
>>
>>
>> Do we want to protect against usbnet_start_xmit()? Maybe I am missing
>> something, but other BQL implementations also do not seem to protect
>> against their respective ndo_start_xmit.
> 
> BQL has been designed so that producer/consumer can run in //
> 
> However, all producers need exclusion (typically done with HARD_TX_LOCK)
> All consumers need exclusion (typically done because of NAPI sched bit)
> 
>>
>>
>> My approach would just protect against usbnet_bh calls from another
>> context with the same locking as skb_dequeue():
>>
>> spin_lock_irqsave(&list->lock, flags);
>> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>> spin_unlock_irqrestore(&list->lock, flags);
> 
> I tend to prefer not masking hard irq unless really necessary.
> 
> Also, reusing  a lock for different purposes makes things confusing
> in terms of code maintenance.
> 
> usbnet is hardly performance critical, I would keep list->lock only to
> protect the list of skbs :)

Thanks for the clarification!


So in usbnet.h I will just 

#include <linux/spinlock.h>

and then save the new field

spinlock_t bql_spinlock;

in struct usbnet and will then call

spin_lock_bh(&dev->bql_spinlock);
netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
spin_unlock_bh(&dev->bql_spinlock);

in usbnet_bh. Am I right?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 12:58             ` Simon Schippers
@ 2025-11-05 13:05               ` Eric Dumazet
  2025-11-05 13:29                 ` Simon Schippers
  0 siblings, 1 reply; 21+ messages in thread
From: Eric Dumazet @ 2025-11-05 13:05 UTC (permalink / raw)
  To: Simon Schippers
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On Wed, Nov 5, 2025 at 4:58 AM Simon Schippers
<simon.schippers@tu-dortmund.de> wrote:
>
> On 11/5/25 13:34, Eric Dumazet wrote:
> > On Wed, Nov 5, 2025 at 4:20 AM Simon Schippers
> > <simon.schippers@tu-dortmund.de> wrote:
> >>
> >> On 11/5/25 12:28, Eric Dumazet wrote:
> >>> On Wed, Nov 5, 2025 at 2:35 AM Simon Schippers
> >>> <simon.schippers@tu-dortmund.de> wrote:
> >>>>
> >>>> On 11/4/25 18:00, Eric Dumazet wrote:
> >>>>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
> >>>>> <simon.schippers@tu-dortmund.de> wrote:
> >>>>>>
> >>>>>> The usbnet driver currently relies on fixed transmit queue lengths, which
> >>>>>> can lead to bufferbloat and large latency spikes under load -
> >>>>>> particularly with cellular modems.
> >>>>>> This patch adds support for Byte Queue Limits (BQL) to dynamically manage
> >>>>>> the transmit queue size and reduce latency without sacrificing
> >>>>>> throughput.
> >>>>>>
> >>>>>> Testing was performed on various devices using the usbnet driver for
> >>>>>> packet transmission:
> >>>>>>
> >>>>>> - DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
> >>>>>> - DELOCK 61969: USB2 to 1 GbE adapter (asix)
> >>>>>> - Quectel RM520: 5G modem (qmi_wwan)
> >>>>>> - USB2 Android tethering (cdc_ncm)
> >>>>>>
> >>>>>> No performance degradation was observed for iperf3 TCP or UDP traffic,
> >>>>>> while latency for a prioritized ping application was significantly
> >>>>>> reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
> >>>>>> utilized by iperf3 UDP traffic, the prioritized ping was improved from
> >>>>>> 1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
> >>>>>> connection, the prioritized ping was improved from 35 ms to 5 ms.
> >>>>>>
> >>>>>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
> >>>>>> ---
> >>>>>>  drivers/net/usb/usbnet.c | 8 ++++++++
> >>>>>>  1 file changed, 8 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> >>>>>> index 62a85dbad31a..1994f03a78ad 100644
> >>>>>> --- a/drivers/net/usb/usbnet.c
> >>>>>> +++ b/drivers/net/usb/usbnet.c
> >>>>>> @@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
> >>>>>>
> >>>>>>         clear_bit(EVENT_DEV_OPEN, &dev->flags);
> >>>>>>         netif_stop_queue (net);
> >>>>>> +       netdev_reset_queue(net);
> >>>>>>
> >>>>>>         netif_info(dev, ifdown, dev->net,
> >>>>>>                    "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
> >>>>>> @@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
> >>>>>>         }
> >>>>>>
> >>>>>>         set_bit(EVENT_DEV_OPEN, &dev->flags);
> >>>>>> +       netdev_reset_queue(net);
> >>>>>>         netif_start_queue (net);
> >>>>>>         netif_info(dev, ifup, dev->net,
> >>>>>>                    "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
> >>>>>> @@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
> >>>>>>         case 0:
> >>>>>>                 netif_trans_update(net);
> >>>>>>                 __usbnet_queue_skb(&dev->txq, skb, tx_start);
> >>>>>> +               netdev_sent_queue(net, skb->len);
> >>>>>>                 if (dev->txq.qlen >= TX_QLEN (dev))
> >>>>>>                         netif_stop_queue (net);
> >>>>>>         }
> >>>>>> @@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
> >>>>>>  static void usbnet_bh(struct timer_list *t)
> >>>>>>  {
> >>>>>>         struct usbnet           *dev = timer_container_of(dev, t, delay);
> >>>>>> +       unsigned int bytes_compl = 0, pkts_compl = 0;
> >>>>>>         struct sk_buff          *skb;
> >>>>>>         struct skb_data         *entry;
> >>>>>>
> >>>>>> @@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
> >>>>>>                                 usb_free_skb(skb);
> >>>>>>                         continue;
> >>>>>>                 case tx_done:
> >>>>>> +                       bytes_compl += skb->len;
> >>>>>> +                       pkts_compl++;
> >>>>>>                         kfree(entry->urb->sg);
> >>>>>>                         fallthrough;
> >>>>>>                 case rx_cleanup:
> >>>>>> @@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
> >>>>>>                 }
> >>>>>>         }
> >>>>>>
> >>>>>> +       netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> >>>>>> +
> >>>>>>         /* restart RX again after disabling due to high error rate */
> >>>>>>         clear_bit(EVENT_RX_KILL, &dev->flags);
> >>>>>>
> >>>>>
> >>>>> I think this is racy. usbnet_bh() can run from two different contexts,
> >>>>> at the same time (from two cpus)
> >>>>>
> >>>>> 1) From process context :
> >>>>> usbnet_bh_work()
> >>>>>
> >>>>> 2) From a timer. (dev->delay)
> >>>>>
> >>>>>
> >>>>> To use BQL, you will need to add mutual exclusion.
> >>>>
> >>>> Yeah, I missed that.
> >>>>
> >>>> I guess synchronizing with the lock of the sk_buff_head dev->done makes
> >>>> sense? The same locking is also done right before in skb_dequeue.
> >>>
> >>> Or only protect the netdev_completed_queue(dev->net, pkts_compl,
> >>> bytes_compl) call,
> >>> adding a specific/dedicated spinlock for this purpose.
> >>>
> >>> spin_lock_bh(&dev->bql_spinlock);
> >>> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> >>> spin_unlock_bh(&dev->bql_spinlock);
> >>>
> >>> I am assuming no usbnet driver is setting dev->lltx = true (or plan to
> >>> in the future)
> >>> so usbnet_start_xmit() is protected by HARD_TX_LOCK() already.
> >>
> >> Yes, I also want to only protect the netdev_completed_queue(dev->net,
> >> pkts_compl, bytes_compl) call. However, I am wondering what you mean with
> >>
> >> spin_lock_bh(&dev->bql_spinlock)
> >> ...
> >>
> >>
> >> Do we want to protect against usbnet_start_xmit()? Maybe I am missing
> >> something, but other BQL implementations also do not seem to protect
> >> against their respective ndo_start_xmit.
> >
> > BQL has been designed so that producer/consumer can run in //
> >
> > However, all producers need exclusion (typically done with HARD_TX_LOCK)
> > All consumers need exclusion (typically done because of NAPI sched bit)
> >
> >>
> >>
> >> My approach would just protect against usbnet_bh calls from another
> >> context with the same locking as skb_dequeue():
> >>
> >> spin_lock_irqsave(&list->lock, flags);
> >> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> >> spin_unlock_irqrestore(&list->lock, flags);
> >
> > I tend to prefer not masking hard irq unless really necessary.
> >
> > Also, reusing  a lock for different purposes makes things confusing
> > in terms of code maintenance.
> >
> > usbnet is hardly performance critical, I would keep list->lock only to
> > protect the list of skbs :)
>
> Thanks for the clarification!
>
>
> So in usbnet.h I will just
>
> #include <linux/spinlock.h>
>
> and then save the new field
>
> spinlock_t bql_spinlock;
>
> in struct usbnet and will then call
>
> spin_lock_bh(&dev->bql_spinlock);
> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
> spin_unlock_bh(&dev->bql_spinlock);
>
> in usbnet_bh. Am I right?

You also need to spin_lock_init() this new lock in setup phase (usbnet_probe)

Test/Run your code after enabling LOCKDEP in your .config
(CONFIG_PROVE_LOCKING=y)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 13:05               ` Eric Dumazet
@ 2025-11-05 13:29                 ` Simon Schippers
  2025-11-05 15:47                   ` Simon Schippers
  0 siblings, 1 reply; 21+ messages in thread
From: Simon Schippers @ 2025-11-05 13:29 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On 11/5/25 14:05, Eric Dumazet wrote:
> On Wed, Nov 5, 2025 at 4:58 AM Simon Schippers
> <simon.schippers@tu-dortmund.de> wrote:
>>
>> On 11/5/25 13:34, Eric Dumazet wrote:
>>> On Wed, Nov 5, 2025 at 4:20 AM Simon Schippers
>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>
>>>> On 11/5/25 12:28, Eric Dumazet wrote:
>>>>> On Wed, Nov 5, 2025 at 2:35 AM Simon Schippers
>>>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>>>
>>>>>> On 11/4/25 18:00, Eric Dumazet wrote:
>>>>>>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
>>>>>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>>>>>
>>>>>>>> The usbnet driver currently relies on fixed transmit queue lengths, which
>>>>>>>> can lead to bufferbloat and large latency spikes under load -
>>>>>>>> particularly with cellular modems.
>>>>>>>> This patch adds support for Byte Queue Limits (BQL) to dynamically manage
>>>>>>>> the transmit queue size and reduce latency without sacrificing
>>>>>>>> throughput.
>>>>>>>>
>>>>>>>> Testing was performed on various devices using the usbnet driver for
>>>>>>>> packet transmission:
>>>>>>>>
>>>>>>>> - DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
>>>>>>>> - DELOCK 61969: USB2 to 1 GbE adapter (asix)
>>>>>>>> - Quectel RM520: 5G modem (qmi_wwan)
>>>>>>>> - USB2 Android tethering (cdc_ncm)
>>>>>>>>
>>>>>>>> No performance degradation was observed for iperf3 TCP or UDP traffic,
>>>>>>>> while latency for a prioritized ping application was significantly
>>>>>>>> reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
>>>>>>>> utilized by iperf3 UDP traffic, the prioritized ping was improved from
>>>>>>>> 1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
>>>>>>>> connection, the prioritized ping was improved from 35 ms to 5 ms.
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
>>>>>>>> ---
>>>>>>>>  drivers/net/usb/usbnet.c | 8 ++++++++
>>>>>>>>  1 file changed, 8 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>>>>>> index 62a85dbad31a..1994f03a78ad 100644
>>>>>>>> --- a/drivers/net/usb/usbnet.c
>>>>>>>> +++ b/drivers/net/usb/usbnet.c
>>>>>>>> @@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
>>>>>>>>
>>>>>>>>         clear_bit(EVENT_DEV_OPEN, &dev->flags);
>>>>>>>>         netif_stop_queue (net);
>>>>>>>> +       netdev_reset_queue(net);
>>>>>>>>
>>>>>>>>         netif_info(dev, ifdown, dev->net,
>>>>>>>>                    "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
>>>>>>>> @@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
>>>>>>>>         }
>>>>>>>>
>>>>>>>>         set_bit(EVENT_DEV_OPEN, &dev->flags);
>>>>>>>> +       netdev_reset_queue(net);
>>>>>>>>         netif_start_queue (net);
>>>>>>>>         netif_info(dev, ifup, dev->net,
>>>>>>>>                    "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
>>>>>>>> @@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
>>>>>>>>         case 0:
>>>>>>>>                 netif_trans_update(net);
>>>>>>>>                 __usbnet_queue_skb(&dev->txq, skb, tx_start);
>>>>>>>> +               netdev_sent_queue(net, skb->len);
>>>>>>>>                 if (dev->txq.qlen >= TX_QLEN (dev))
>>>>>>>>                         netif_stop_queue (net);
>>>>>>>>         }
>>>>>>>> @@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
>>>>>>>>  static void usbnet_bh(struct timer_list *t)
>>>>>>>>  {
>>>>>>>>         struct usbnet           *dev = timer_container_of(dev, t, delay);
>>>>>>>> +       unsigned int bytes_compl = 0, pkts_compl = 0;
>>>>>>>>         struct sk_buff          *skb;
>>>>>>>>         struct skb_data         *entry;
>>>>>>>>
>>>>>>>> @@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
>>>>>>>>                                 usb_free_skb(skb);
>>>>>>>>                         continue;
>>>>>>>>                 case tx_done:
>>>>>>>> +                       bytes_compl += skb->len;
>>>>>>>> +                       pkts_compl++;
>>>>>>>>                         kfree(entry->urb->sg);
>>>>>>>>                         fallthrough;
>>>>>>>>                 case rx_cleanup:
>>>>>>>> @@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
>>>>>>>>                 }
>>>>>>>>         }
>>>>>>>>
>>>>>>>> +       netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>>>>>>> +
>>>>>>>>         /* restart RX again after disabling due to high error rate */
>>>>>>>>         clear_bit(EVENT_RX_KILL, &dev->flags);
>>>>>>>>
>>>>>>>
>>>>>>> I think this is racy. usbnet_bh() can run from two different contexts,
>>>>>>> at the same time (from two cpus)
>>>>>>>
>>>>>>> 1) From process context :
>>>>>>> usbnet_bh_work()
>>>>>>>
>>>>>>> 2) From a timer. (dev->delay)
>>>>>>>
>>>>>>>
>>>>>>> To use BQL, you will need to add mutual exclusion.
>>>>>>
>>>>>> Yeah, I missed that.
>>>>>>
>>>>>> I guess synchronizing with the lock of the sk_buff_head dev->done makes
>>>>>> sense? The same locking is also done right before in skb_dequeue.
>>>>>
>>>>> Or only protect the netdev_completed_queue(dev->net, pkts_compl,
>>>>> bytes_compl) call,
>>>>> adding a specific/dedicated spinlock for this purpose.
>>>>>
>>>>> spin_lock_bh(&dev->bql_spinlock);
>>>>> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>>>> spin_unlock_bh(&dev->bql_spinlock);
>>>>>
>>>>> I am assuming no usbnet driver is setting dev->lltx = true (or plan to
>>>>> in the future)
>>>>> so usbnet_start_xmit() is protected by HARD_TX_LOCK() already.
>>>>
>>>> Yes, I also want to only protect the netdev_completed_queue(dev->net,
>>>> pkts_compl, bytes_compl) call. However, I am wondering what you mean with
>>>>
>>>> spin_lock_bh(&dev->bql_spinlock)
>>>> ...
>>>>
>>>>
>>>> Do we want to protect against usbnet_start_xmit()? Maybe I am missing
>>>> something, but other BQL implementations also do not seem to protect
>>>> against their respective ndo_start_xmit.
>>>
>>> BQL has been designed so that producer/consumer can run in //
>>>
>>> However, all producers need exclusion (typically done with HARD_TX_LOCK)
>>> All consumers need exclusion (typically done because of NAPI sched bit)
>>>
>>>>
>>>>
>>>> My approach would just protect against usbnet_bh calls from another
>>>> context with the same locking as skb_dequeue():
>>>>
>>>> spin_lock_irqsave(&list->lock, flags);
>>>> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>>> spin_unlock_irqrestore(&list->lock, flags);
>>>
>>> I tend to prefer not masking hard irq unless really necessary.
>>>
>>> Also, reusing  a lock for different purposes makes things confusing
>>> in terms of code maintenance.
>>>
>>> usbnet is hardly performance critical, I would keep list->lock only to
>>> protect the list of skbs :)
>>
>> Thanks for the clarification!
>>
>>
>> So in usbnet.h I will just
>>
>> #include <linux/spinlock.h>
>>
>> and then save the new field
>>
>> spinlock_t bql_spinlock;
>>
>> in struct usbnet and will then call
>>
>> spin_lock_bh(&dev->bql_spinlock);
>> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>> spin_unlock_bh(&dev->bql_spinlock);
>>
>> in usbnet_bh. Am I right?
> 
> You also need to spin_lock_init() this new lock in setup phase (usbnet_probe)
> 

Yes, thanks.

> Test/Run your code after enabling LOCKDEP in your .config
> (CONFIG_PROVE_LOCKING=y)

Okay, I will test the new code :)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 13:29                 ` Simon Schippers
@ 2025-11-05 15:47                   ` Simon Schippers
  2025-11-06 10:40                     ` Eric Dumazet
  0 siblings, 1 reply; 21+ messages in thread
From: Simon Schippers @ 2025-11-05 15:47 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On 11/5/25 14:29, Simon Schippers wrote:
> On 11/5/25 14:05, Eric Dumazet wrote:
>> On Wed, Nov 5, 2025 at 4:58 AM Simon Schippers
>> <simon.schippers@tu-dortmund.de> wrote:
>>>
>>> On 11/5/25 13:34, Eric Dumazet wrote:
>>>> On Wed, Nov 5, 2025 at 4:20 AM Simon Schippers
>>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>>
>>>>> On 11/5/25 12:28, Eric Dumazet wrote:
>>>>>> On Wed, Nov 5, 2025 at 2:35 AM Simon Schippers
>>>>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>>>>
>>>>>>> On 11/4/25 18:00, Eric Dumazet wrote:
>>>>>>>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
>>>>>>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>>>>>>
>>>>>>>>> The usbnet driver currently relies on fixed transmit queue lengths, which
>>>>>>>>> can lead to bufferbloat and large latency spikes under load -
>>>>>>>>> particularly with cellular modems.
>>>>>>>>> This patch adds support for Byte Queue Limits (BQL) to dynamically manage
>>>>>>>>> the transmit queue size and reduce latency without sacrificing
>>>>>>>>> throughput.
>>>>>>>>>
>>>>>>>>> Testing was performed on various devices using the usbnet driver for
>>>>>>>>> packet transmission:
>>>>>>>>>
>>>>>>>>> - DELOCK 66045: USB3 to 2.5 GbE adapter (ax88179_178a)
>>>>>>>>> - DELOCK 61969: USB2 to 1 GbE adapter (asix)
>>>>>>>>> - Quectel RM520: 5G modem (qmi_wwan)
>>>>>>>>> - USB2 Android tethering (cdc_ncm)
>>>>>>>>>
>>>>>>>>> No performance degradation was observed for iperf3 TCP or UDP traffic,
>>>>>>>>> while latency for a prioritized ping application was significantly
>>>>>>>>> reduced. For example, using the USB3 to 2.5 GbE adapter, which was fully
>>>>>>>>> utilized by iperf3 UDP traffic, the prioritized ping was improved from
>>>>>>>>> 1.6 ms to 0.6 ms. With the same setup but with a 100 Mbit/s Ethernet
>>>>>>>>> connection, the prioritized ping was improved from 35 ms to 5 ms.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
>>>>>>>>> ---
>>>>>>>>>  drivers/net/usb/usbnet.c | 8 ++++++++
>>>>>>>>>  1 file changed, 8 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>>>>>>> index 62a85dbad31a..1994f03a78ad 100644
>>>>>>>>> --- a/drivers/net/usb/usbnet.c
>>>>>>>>> +++ b/drivers/net/usb/usbnet.c
>>>>>>>>> @@ -831,6 +831,7 @@ int usbnet_stop(struct net_device *net)
>>>>>>>>>
>>>>>>>>>         clear_bit(EVENT_DEV_OPEN, &dev->flags);
>>>>>>>>>         netif_stop_queue (net);
>>>>>>>>> +       netdev_reset_queue(net);
>>>>>>>>>
>>>>>>>>>         netif_info(dev, ifdown, dev->net,
>>>>>>>>>                    "stop stats: rx/tx %lu/%lu, errs %lu/%lu\n",
>>>>>>>>> @@ -939,6 +940,7 @@ int usbnet_open(struct net_device *net)
>>>>>>>>>         }
>>>>>>>>>
>>>>>>>>>         set_bit(EVENT_DEV_OPEN, &dev->flags);
>>>>>>>>> +       netdev_reset_queue(net);
>>>>>>>>>         netif_start_queue (net);
>>>>>>>>>         netif_info(dev, ifup, dev->net,
>>>>>>>>>                    "open: enable queueing (rx %d, tx %d) mtu %d %s framing\n",
>>>>>>>>> @@ -1500,6 +1502,7 @@ netdev_tx_t usbnet_start_xmit(struct sk_buff *skb, struct net_device *net)
>>>>>>>>>         case 0:
>>>>>>>>>                 netif_trans_update(net);
>>>>>>>>>                 __usbnet_queue_skb(&dev->txq, skb, tx_start);
>>>>>>>>> +               netdev_sent_queue(net, skb->len);
>>>>>>>>>                 if (dev->txq.qlen >= TX_QLEN (dev))
>>>>>>>>>                         netif_stop_queue (net);
>>>>>>>>>         }
>>>>>>>>> @@ -1563,6 +1566,7 @@ static inline void usb_free_skb(struct sk_buff *skb)
>>>>>>>>>  static void usbnet_bh(struct timer_list *t)
>>>>>>>>>  {
>>>>>>>>>         struct usbnet           *dev = timer_container_of(dev, t, delay);
>>>>>>>>> +       unsigned int bytes_compl = 0, pkts_compl = 0;
>>>>>>>>>         struct sk_buff          *skb;
>>>>>>>>>         struct skb_data         *entry;
>>>>>>>>>
>>>>>>>>> @@ -1574,6 +1578,8 @@ static void usbnet_bh(struct timer_list *t)
>>>>>>>>>                                 usb_free_skb(skb);
>>>>>>>>>                         continue;
>>>>>>>>>                 case tx_done:
>>>>>>>>> +                       bytes_compl += skb->len;
>>>>>>>>> +                       pkts_compl++;
>>>>>>>>>                         kfree(entry->urb->sg);
>>>>>>>>>                         fallthrough;
>>>>>>>>>                 case rx_cleanup:
>>>>>>>>> @@ -1584,6 +1590,8 @@ static void usbnet_bh(struct timer_list *t)
>>>>>>>>>                 }
>>>>>>>>>         }
>>>>>>>>>
>>>>>>>>> +       netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>>>>>>>> +
>>>>>>>>>         /* restart RX again after disabling due to high error rate */
>>>>>>>>>         clear_bit(EVENT_RX_KILL, &dev->flags);
>>>>>>>>>
>>>>>>>>
>>>>>>>> I think this is racy. usbnet_bh() can run from two different contexts,
>>>>>>>> at the same time (from two cpus)
>>>>>>>>
>>>>>>>> 1) From process context :
>>>>>>>> usbnet_bh_work()
>>>>>>>>
>>>>>>>> 2) From a timer. (dev->delay)
>>>>>>>>
>>>>>>>>
>>>>>>>> To use BQL, you will need to add mutual exclusion.
>>>>>>>
>>>>>>> Yeah, I missed that.
>>>>>>>
>>>>>>> I guess synchronizing with the lock of the sk_buff_head dev->done makes
>>>>>>> sense? The same locking is also done right before in skb_dequeue.
>>>>>>
>>>>>> Or only protect the netdev_completed_queue(dev->net, pkts_compl,
>>>>>> bytes_compl) call,
>>>>>> adding a specific/dedicated spinlock for this purpose.
>>>>>>
>>>>>> spin_lock_bh(&dev->bql_spinlock);
>>>>>> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>>>>> spin_unlock_bh(&dev->bql_spinlock);
>>>>>>
>>>>>> I am assuming no usbnet driver is setting dev->lltx = true (or plan to
>>>>>> in the future)
>>>>>> so usbnet_start_xmit() is protected by HARD_TX_LOCK() already.
>>>>>
>>>>> Yes, I also want to only protect the netdev_completed_queue(dev->net,
>>>>> pkts_compl, bytes_compl) call. However, I am wondering what you mean with
>>>>>
>>>>> spin_lock_bh(&dev->bql_spinlock)
>>>>> ...
>>>>>
>>>>>
>>>>> Do we want to protect against usbnet_start_xmit()? Maybe I am missing
>>>>> something, but other BQL implementations also do not seem to protect
>>>>> against their respective ndo_start_xmit.
>>>>
>>>> BQL has been designed so that producer/consumer can run in //
>>>>
>>>> However, all producers need exclusion (typically done with HARD_TX_LOCK)
>>>> All consumers need exclusion (typically done because of NAPI sched bit)
>>>>
>>>>>
>>>>>
>>>>> My approach would just protect against usbnet_bh calls from another
>>>>> context with the same locking as skb_dequeue():
>>>>>
>>>>> spin_lock_irqsave(&list->lock, flags);
>>>>> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>>>> spin_unlock_irqrestore(&list->lock, flags);
>>>>
>>>> I tend to prefer not masking hard irq unless really necessary.
>>>>
>>>> Also, reusing  a lock for different purposes makes things confusing
>>>> in terms of code maintenance.
>>>>
>>>> usbnet is hardly performance critical, I would keep list->lock only to
>>>> protect the list of skbs :)
>>>
>>> Thanks for the clarification!
>>>
>>>
>>> So in usbnet.h I will just
>>>
>>> #include <linux/spinlock.h>
>>>
>>> and then save the new field
>>>
>>> spinlock_t bql_spinlock;
>>>
>>> in struct usbnet and will then call
>>>
>>> spin_lock_bh(&dev->bql_spinlock);
>>> netdev_completed_queue(dev->net, pkts_compl, bytes_compl);
>>> spin_unlock_bh(&dev->bql_spinlock);
>>>
>>> in usbnet_bh. Am I right?
>>
>> You also need to spin_lock_init() this new lock in setup phase (usbnet_probe)
>>
> 
> Yes, thanks.
> 
>> Test/Run your code after enabling LOCKDEP in your .config
>> (CONFIG_PROVE_LOCKING=y)
> 
> Okay, I will test the new code :)

So, I did my first tests.

I compiled it with CONFIG_PROVE_LOCKING and ran iperf3 TCP tests on my
USB2 to Gbit Ethernet adapter I had at hand. dmesg shows no lockdep
warnings. What else should I test?

Thanks

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 11:04       ` Simon Schippers
@ 2025-11-06  8:38         ` Daniele Palmas
  2025-11-06 10:00           ` Simon Schippers
  0 siblings, 1 reply; 21+ messages in thread
From: Daniele Palmas @ 2025-11-06  8:38 UTC (permalink / raw)
  To: Simon Schippers
  Cc: Eric Dumazet, oneukum, andrew+netdev, davem, kuba, pabeni,
	netdev, linux-usb, linux-kernel

Hi Simon,

Il giorno mer 5 nov 2025 alle ore 12:05 Simon Schippers
<simon.schippers@tu-dortmund.de> ha scritto:
>
> On 11/5/25 11:35, Daniele Palmas wrote:
> > Hello Simon,
> >
> > Il giorno mer 5 nov 2025 alle ore 11:40 Simon Schippers
> > <simon.schippers@tu-dortmund.de> ha scritto:
> >>
> >> On 11/4/25 18:02, Eric Dumazet wrote:
> >>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
> >>> <simon.schippers@tu-dortmund.de> wrote:
> >>>>
> >>>> During recent testing, I observed significant latency spikes when using
> >>>> Quectel 5G modems under load. Investigation revealed that the issue was
> >>>> caused by bufferbloat in the usbnet driver.
> >>>>
> >>>> In the current implementation, usbnet uses a fixed tx_qlen of:
> >>>>
> >>>> USB2: 60 * 1518 bytes = 91.08 KB
> >>>> USB3: 60 * 5 * 1518 bytes = 454.80 KB
> >>>>
> >>>> Such large transmit queues can be problematic, especially for cellular
> >>>> modems. For example, with a typical celluar link speed of 10 Mbit/s, a
> >>>> fully occupied USB3 transmit queue results in:
> >>>>
> >>>> 454.80 KB / (10 Mbit/s / 8 bit/byte) = 363.84 ms
> >>>>
> >>>> of additional latency.
> >>>
> >>> Doesn't 5G need to push more packets to the driver to get good aggregation ?
> >>>
> >>
> >> Yes, but not 455 KB for low speeds. 5G requires a queue of a few ms to
> >> aggregate enough packets for a frame but not of several hundred ms as
> >> calculated in my example. And yes, there are situations where 5G,
> >> especially FR2 mmWave, reaches Gbit/s speeds where a big queue is
> >> required. But the dynamic queue limit approach of BQL should be well
> >> suited for these varying speeds.
> >>
> >
> > out of curiosity, related to the test with 5G Quectel, did you test
> > enabling aggregation through QMAP (kernel module rmnet) or simply
> > qmi_wwan raw_ip ?
> >
> > Regards,
> > Daniele
> >
>
> Hi Daniele,
>
> I simply used qmi_wwan. I actually never touched rmnet before.
> Is the aggregation through QMAP what you and Eric mean with aggregation?
> Because then I misunderstood it, because I was thinking about aggregating
> enough (and not too many) packets in the usbnet queue.
>

I can't speak for Eric, but, yes, that is what I meant for
aggregation, this is the common way those high-cat modems are used:
it's not clear to me if the change you are proposing could have any
impact when rmnet is used, that's why I was asking the test
conditions.

Thanks,
Daniele

> Thanks
>
> >>>>
> >>>> To address this issue, this patch introduces support for
> >>>> Byte Queue Limits (BQL) [1][2] in the usbnet driver. BQL dynamically
> >>>> limits the amount of data queued in the driver, effectively reducing
> >>>> latency without impacting throughput.
> >>>> This implementation was successfully tested on several devices as
> >>>> described in the commit.
> >>>>
> >>>>
> >>>>
> >>>> Future work
> >>>>
> >>>> Due to offloading, TCP often produces SKBs up to 64 KB in size.
> >>>
> >>> Only for rates > 500 Mbit. After BQL, we had many more improvements in
> >>> the stack.
> >>> https://lwn.net/Articles/564978/
> >>>
> >>>
> >>
> >> I also saw these large SKBs, for example, for my USB2 Android tethering,
> >> which advertises a network speed of < 500 Mbit/s.
> >> I saw these large SKBs by looking at the file:
> >>
> >> cat /sys/class/net/INTERFACE/queues/tx-0/byte_queue_limits/inflight
> >>
> >> For UDP-only traffic, inflight always maxed out at MTU size.
> >>
> >> Thank you for your replies!
> >>
> >>>> To
> >>>> further decrease buffer bloat, I tried to disable TSO, GSO and LRO but it
> >>>> did not have the intended effect in my tests. The only dirty workaround I
> >>>> found so far was to call netif_stop_queue() whenever BQL sets
> >>>> __QUEUE_STATE_STACK_XOFF. However, a proper solution to this issue would
> >>>> be desirable.
> >>>>
> >>>> I also plan to publish a scientific paper on this topic in the near
> >>>> future.
> >>>>
> >>>> Thanks,
> >>>> Simon
> >>>>
> >>>> [1] https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83
> >>>> [2] https://lwn.net/Articles/469652/
> >>>>
> >>>> Simon Schippers (1):
> >>>>   usbnet: Add support for Byte Queue Limits (BQL)
> >>>>
> >>>>  drivers/net/usb/usbnet.c | 8 ++++++++
> >>>>  1 file changed, 8 insertions(+)
> >>>>
> >>>> --
> >>>> 2.43.0
> >>>>
> >>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-06  8:38         ` Daniele Palmas
@ 2025-11-06 10:00           ` Simon Schippers
  2025-11-07 13:34             ` Daniele Palmas
  0 siblings, 1 reply; 21+ messages in thread
From: Simon Schippers @ 2025-11-06 10:00 UTC (permalink / raw)
  To: Daniele Palmas
  Cc: Eric Dumazet, oneukum, andrew+netdev, davem, kuba, pabeni,
	netdev, linux-usb, linux-kernel

On 11/6/25 09:38, Daniele Palmas wrote:
> Hi Simon,
> 
> Il giorno mer 5 nov 2025 alle ore 12:05 Simon Schippers
> <simon.schippers@tu-dortmund.de> ha scritto:
>>
>> On 11/5/25 11:35, Daniele Palmas wrote:
>>> Hello Simon,
>>>
>>> Il giorno mer 5 nov 2025 alle ore 11:40 Simon Schippers
>>> <simon.schippers@tu-dortmund.de> ha scritto:
>>>>
>>>> On 11/4/25 18:02, Eric Dumazet wrote:
>>>>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
>>>>> <simon.schippers@tu-dortmund.de> wrote:
>>>>>>
>>>>>> During recent testing, I observed significant latency spikes when using
>>>>>> Quectel 5G modems under load. Investigation revealed that the issue was
>>>>>> caused by bufferbloat in the usbnet driver.
>>>>>>
>>>>>> In the current implementation, usbnet uses a fixed tx_qlen of:
>>>>>>
>>>>>> USB2: 60 * 1518 bytes = 91.08 KB
>>>>>> USB3: 60 * 5 * 1518 bytes = 454.80 KB
>>>>>>
>>>>>> Such large transmit queues can be problematic, especially for cellular
>>>>>> modems. For example, with a typical celluar link speed of 10 Mbit/s, a
>>>>>> fully occupied USB3 transmit queue results in:
>>>>>>
>>>>>> 454.80 KB / (10 Mbit/s / 8 bit/byte) = 363.84 ms
>>>>>>
>>>>>> of additional latency.
>>>>>
>>>>> Doesn't 5G need to push more packets to the driver to get good aggregation ?
>>>>>
>>>>
>>>> Yes, but not 455 KB for low speeds. 5G requires a queue of a few ms to
>>>> aggregate enough packets for a frame but not of several hundred ms as
>>>> calculated in my example. And yes, there are situations where 5G,
>>>> especially FR2 mmWave, reaches Gbit/s speeds where a big queue is
>>>> required. But the dynamic queue limit approach of BQL should be well
>>>> suited for these varying speeds.
>>>>
>>>
>>> out of curiosity, related to the test with 5G Quectel, did you test
>>> enabling aggregation through QMAP (kernel module rmnet) or simply
>>> qmi_wwan raw_ip ?
>>>
>>> Regards,
>>> Daniele
>>>
>>
>> Hi Daniele,
>>
>> I simply used qmi_wwan. I actually never touched rmnet before.
>> Is the aggregation through QMAP what you and Eric mean with aggregation?
>> Because then I misunderstood it, because I was thinking about aggregating
>> enough (and not too many) packets in the usbnet queue.
>>
> 
> I can't speak for Eric, but, yes, that is what I meant for
> aggregation, this is the common way those high-cat modems are used:

Hi Daniele,

I think I *really* have to take a look at rmnet and aggregation through
QMAP for future projects :)

> it's not clear to me if the change you are proposing could have any
> impact when rmnet is used, that's why I was asking the test
> conditions.
> 
> Thanks,
> Daniele
> 

This patch has an impact on the underlying USB physical transport of
rmnet. From my understanding, the call stack is as follows:

rmnet_map_tx_aggregate or rmnet_send_skb

|
| Calling dev_queue_xmit(skb)
V

qmi_wwan used for USB modem

|
|  ndo_start_xmit(skb, net) is called
V

usbnet_start_xmit is executed where the size of the internal queue is
dynamically changed using the Byte Queue Limits algorithm by this patch.

Correct me if I am wrong, but I think in the end usbnet is used.

Thanks,
Simon

>> Thanks
>>
>>>>>>
>>>>>> To address this issue, this patch introduces support for
>>>>>> Byte Queue Limits (BQL) [1][2] in the usbnet driver. BQL dynamically
>>>>>> limits the amount of data queued in the driver, effectively reducing
>>>>>> latency without impacting throughput.
>>>>>> This implementation was successfully tested on several devices as
>>>>>> described in the commit.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Future work
>>>>>>
>>>>>> Due to offloading, TCP often produces SKBs up to 64 KB in size.
>>>>>
>>>>> Only for rates > 500 Mbit. After BQL, we had many more improvements in
>>>>> the stack.
>>>>> https://lwn.net/Articles/564978/
>>>>>
>>>>>
>>>>
>>>> I also saw these large SKBs, for example, for my USB2 Android tethering,
>>>> which advertises a network speed of < 500 Mbit/s.
>>>> I saw these large SKBs by looking at the file:
>>>>
>>>> cat /sys/class/net/INTERFACE/queues/tx-0/byte_queue_limits/inflight
>>>>
>>>> For UDP-only traffic, inflight always maxed out at MTU size.
>>>>
>>>> Thank you for your replies!
>>>>
>>>>>> To
>>>>>> further decrease buffer bloat, I tried to disable TSO, GSO and LRO but it
>>>>>> did not have the intended effect in my tests. The only dirty workaround I
>>>>>> found so far was to call netif_stop_queue() whenever BQL sets
>>>>>> __QUEUE_STATE_STACK_XOFF. However, a proper solution to this issue would
>>>>>> be desirable.
>>>>>>
>>>>>> I also plan to publish a scientific paper on this topic in the near
>>>>>> future.
>>>>>>
>>>>>> Thanks,
>>>>>> Simon
>>>>>>
>>>>>> [1] https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83
>>>>>> [2] https://lwn.net/Articles/469652/
>>>>>>
>>>>>> Simon Schippers (1):
>>>>>>   usbnet: Add support for Byte Queue Limits (BQL)
>>>>>>
>>>>>>  drivers/net/usb/usbnet.c | 8 ++++++++
>>>>>>  1 file changed, 8 insertions(+)
>>>>>>
>>>>>> --
>>>>>> 2.43.0
>>>>>>
>>>>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-05 15:47                   ` Simon Schippers
@ 2025-11-06 10:40                     ` Eric Dumazet
  2025-11-06 11:07                       ` Eric Dumazet
  0 siblings, 1 reply; 21+ messages in thread
From: Eric Dumazet @ 2025-11-06 10:40 UTC (permalink / raw)
  To: Simon Schippers
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

>
> I compiled it with CONFIG_PROVE_LOCKING and ran iperf3 TCP tests on my
> USB2 to Gbit Ethernet adapter I had at hand. dmesg shows no lockdep
> warnings. What else should I test?

That should be fine, please send a V2

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-06 10:40                     ` Eric Dumazet
@ 2025-11-06 11:07                       ` Eric Dumazet
  0 siblings, 0 replies; 21+ messages in thread
From: Eric Dumazet @ 2025-11-06 11:07 UTC (permalink / raw)
  To: Simon Schippers
  Cc: oneukum, andrew+netdev, davem, kuba, pabeni, netdev, linux-usb,
	linux-kernel

On Thu, Nov 6, 2025 at 2:40 AM Eric Dumazet <edumazet@google.com> wrote:
>
> >
> > I compiled it with CONFIG_PROVE_LOCKING and ran iperf3 TCP tests on my
> > USB2 to Gbit Ethernet adapter I had at hand. dmesg shows no lockdep
> > warnings. What else should I test?
>
> That should be fine, please send a V2

BW, no need for a cover letter when sending a single patch.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL)
  2025-11-06 10:00           ` Simon Schippers
@ 2025-11-07 13:34             ` Daniele Palmas
  0 siblings, 0 replies; 21+ messages in thread
From: Daniele Palmas @ 2025-11-07 13:34 UTC (permalink / raw)
  To: Simon Schippers
  Cc: Eric Dumazet, oneukum, andrew+netdev, davem, kuba, pabeni,
	netdev, linux-usb, linux-kernel

Hi Simon,

Il giorno gio 6 nov 2025 alle ore 11:00 Simon Schippers
<simon.schippers@tu-dortmund.de> ha scritto:
>
> On 11/6/25 09:38, Daniele Palmas wrote:
> > Hi Simon,
> >
> > Il giorno mer 5 nov 2025 alle ore 12:05 Simon Schippers
> > <simon.schippers@tu-dortmund.de> ha scritto:
> >>
> >> On 11/5/25 11:35, Daniele Palmas wrote:
> >>> Hello Simon,
> >>>
> >>> Il giorno mer 5 nov 2025 alle ore 11:40 Simon Schippers
> >>> <simon.schippers@tu-dortmund.de> ha scritto:
> >>>>
> >>>> On 11/4/25 18:02, Eric Dumazet wrote:
> >>>>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers
> >>>>> <simon.schippers@tu-dortmund.de> wrote:
> >>>>>>
> >>>>>> During recent testing, I observed significant latency spikes when using
> >>>>>> Quectel 5G modems under load. Investigation revealed that the issue was
> >>>>>> caused by bufferbloat in the usbnet driver.
> >>>>>>
> >>>>>> In the current implementation, usbnet uses a fixed tx_qlen of:
> >>>>>>
> >>>>>> USB2: 60 * 1518 bytes = 91.08 KB
> >>>>>> USB3: 60 * 5 * 1518 bytes = 454.80 KB
> >>>>>>
> >>>>>> Such large transmit queues can be problematic, especially for cellular
> >>>>>> modems. For example, with a typical celluar link speed of 10 Mbit/s, a
> >>>>>> fully occupied USB3 transmit queue results in:
> >>>>>>
> >>>>>> 454.80 KB / (10 Mbit/s / 8 bit/byte) = 363.84 ms
> >>>>>>
> >>>>>> of additional latency.
> >>>>>
> >>>>> Doesn't 5G need to push more packets to the driver to get good aggregation ?
> >>>>>
> >>>>
> >>>> Yes, but not 455 KB for low speeds. 5G requires a queue of a few ms to
> >>>> aggregate enough packets for a frame but not of several hundred ms as
> >>>> calculated in my example. And yes, there are situations where 5G,
> >>>> especially FR2 mmWave, reaches Gbit/s speeds where a big queue is
> >>>> required. But the dynamic queue limit approach of BQL should be well
> >>>> suited for these varying speeds.
> >>>>
> >>>
> >>> out of curiosity, related to the test with 5G Quectel, did you test
> >>> enabling aggregation through QMAP (kernel module rmnet) or simply
> >>> qmi_wwan raw_ip ?
> >>>
> >>> Regards,
> >>> Daniele
> >>>
> >>
> >> Hi Daniele,
> >>
> >> I simply used qmi_wwan. I actually never touched rmnet before.
> >> Is the aggregation through QMAP what you and Eric mean with aggregation?
> >> Because then I misunderstood it, because I was thinking about aggregating
> >> enough (and not too many) packets in the usbnet queue.
> >>
> >
> > I can't speak for Eric, but, yes, that is what I meant for
> > aggregation, this is the common way those high-cat modems are used:
>
> Hi Daniele,
>
> I think I *really* have to take a look at rmnet and aggregation through
> QMAP for future projects :)
>
> > it's not clear to me if the change you are proposing could have any
> > impact when rmnet is used, that's why I was asking the test
> > conditions.
> >
> > Thanks,
> > Daniele
> >
>
> This patch has an impact on the underlying USB physical transport of
> rmnet. From my understanding, the call stack is as follows:
>
> rmnet_map_tx_aggregate or rmnet_send_skb
>
> |
> | Calling dev_queue_xmit(skb)
> V
>
> qmi_wwan used for USB modem
>
> |
> |  ndo_start_xmit(skb, net) is called
> V
>
> usbnet_start_xmit is executed where the size of the internal queue is
> dynamically changed using the Byte Queue Limits algorithm by this patch.
>
> Correct me if I am wrong, but I think in the end usbnet is used.
>

Exactly, I was just wondering if this patch had any effect on the
overall throughput performance once the aggregation is enabled.

Hopefully I'll be able to perform some tests once the patch is merged.

Thanks,
Daniele

> Thanks,
> Simon
>
> >> Thanks
> >>
> >>>>>>
> >>>>>> To address this issue, this patch introduces support for
> >>>>>> Byte Queue Limits (BQL) [1][2] in the usbnet driver. BQL dynamically
> >>>>>> limits the amount of data queued in the driver, effectively reducing
> >>>>>> latency without impacting throughput.
> >>>>>> This implementation was successfully tested on several devices as
> >>>>>> described in the commit.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Future work
> >>>>>>
> >>>>>> Due to offloading, TCP often produces SKBs up to 64 KB in size.
> >>>>>
> >>>>> Only for rates > 500 Mbit. After BQL, we had many more improvements in
> >>>>> the stack.
> >>>>> https://lwn.net/Articles/564978/
> >>>>>
> >>>>>
> >>>>
> >>>> I also saw these large SKBs, for example, for my USB2 Android tethering,
> >>>> which advertises a network speed of < 500 Mbit/s.
> >>>> I saw these large SKBs by looking at the file:
> >>>>
> >>>> cat /sys/class/net/INTERFACE/queues/tx-0/byte_queue_limits/inflight
> >>>>
> >>>> For UDP-only traffic, inflight always maxed out at MTU size.
> >>>>
> >>>> Thank you for your replies!
> >>>>
> >>>>>> To
> >>>>>> further decrease buffer bloat, I tried to disable TSO, GSO and LRO but it
> >>>>>> did not have the intended effect in my tests. The only dirty workaround I
> >>>>>> found so far was to call netif_stop_queue() whenever BQL sets
> >>>>>> __QUEUE_STATE_STACK_XOFF. However, a proper solution to this issue would
> >>>>>> be desirable.
> >>>>>>
> >>>>>> I also plan to publish a scientific paper on this topic in the near
> >>>>>> future.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Simon
> >>>>>>
> >>>>>> [1] https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83
> >>>>>> [2] https://lwn.net/Articles/469652/
> >>>>>>
> >>>>>> Simon Schippers (1):
> >>>>>>   usbnet: Add support for Byte Queue Limits (BQL)
> >>>>>>
> >>>>>>  drivers/net/usb/usbnet.c | 8 ++++++++
> >>>>>>  1 file changed, 8 insertions(+)
> >>>>>>
> >>>>>> --
> >>>>>> 2.43.0
> >>>>>>
> >>>>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2025-11-07 13:47 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-11-04 16:13 [PATCH net-next v1 0/1] usbnet: Add support for Byte Queue Limits (BQL) Simon Schippers
2025-11-04 16:13 ` [PATCH net-next v1 1/1] " Simon Schippers
2025-11-04 17:00   ` Eric Dumazet
2025-11-05 10:35     ` Simon Schippers
2025-11-05 11:28       ` Eric Dumazet
2025-11-05 12:20         ` Simon Schippers
2025-11-05 12:34           ` Eric Dumazet
2025-11-05 12:58             ` Simon Schippers
2025-11-05 13:05               ` Eric Dumazet
2025-11-05 13:29                 ` Simon Schippers
2025-11-05 15:47                   ` Simon Schippers
2025-11-06 10:40                     ` Eric Dumazet
2025-11-06 11:07                       ` Eric Dumazet
2025-11-04 17:02 ` [PATCH net-next v1 0/1] " Eric Dumazet
2025-11-05 10:40   ` Simon Schippers
2025-11-05 10:35     ` Daniele Palmas
2025-11-05 11:04       ` Simon Schippers
2025-11-06  8:38         ` Daniele Palmas
2025-11-06 10:00           ` Simon Schippers
2025-11-07 13:34             ` Daniele Palmas
2025-11-05 11:34     ` Eric Dumazet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®