| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c01c12a8-c19c-4b9f-94d1-2a106e65a074@tu-dortmund.de>
Date: Wed, 5 Nov 2025 13:58:38 +0100
From: Simon Schippers <simon.schippers@...dortmund.de>
To: Eric Dumazet <edumazet@...gle.com>
Cc: oneukum@...e.com, andrew+netdev@...n.ch, davem@...emloft.net,
kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits
(BQL)
On 11/5/25 13:34, Eric Dumazet wrote:
> On Wed, Nov 5, 2025 at 4:20 AM Simon Schippers
> <simon.schippers@...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@...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@...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@...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?
Powered by blists - more mailing lists