| 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: <9ebd72d0-5ae9-4844-b0be-5629c52e6df8@tu-dortmund.de>
Date: Wed, 5 Nov 2025 14:29:05 +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 14:05, Eric Dumazet wrote:
> On Wed, Nov 5, 2025 at 4:58 AM Simon Schippers
> <simon.schippers@...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@...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?
>
> 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 :)
Powered by blists - more mailing lists