| 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: <64a963ed-400e-4bd2-a4e3-6357f3480367@tu-dortmund.de>
Date: Wed, 5 Nov 2025 16:47:08 +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: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@...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 :)
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
Powered by blists - more mailing lists