| 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: <CANn89iJpXwmvg0MOvLo8+hVAhaMTL_1_62Afk_6dG1ZEL3tORQ@mail.gmail.com>
Date: Wed, 5 Nov 2025 05:05:01 -0800
From: Eric Dumazet <edumazet@...gle.com>
To: Simon Schippers <simon.schippers@...dortmund.de>
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: Re: [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL)
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)
Test/Run your code after enabling LOCKDEP in your .config
(CONFIG_PROVE_LOCKING=y)
Powered by blists - more mailing lists