From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from unimail.uni-dortmund.de (mx1.hrz.uni-dortmund.de [129.217.128.51]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5BBFB33B96D; Wed, 5 Nov 2025 15:47:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=129.217.128.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1762357640; cv=none; b=Eo233dCG2B5D+dC7QAQb2+rJ6DH3JwP2iFr5Bo0g2VSBb26ddheSFBFgeiIJwQqTtjpimYKLtUnIxh/C8PY8/Yef/yEcUHsGNzJ/M49SrXApGDmE57Mo2Lycu8YreDfuPltm5M4XkgDUU2CvRH39Um1YEVLjkkk0IoXM0q1D+yY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1762357640; c=relaxed/simple; bh=O2PvKUUds593hhJv0sfT6cPaqkLEQtmgvK+gnKL+Ibs=; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:References: In-Reply-To:Content-Type; b=DT3vZf6hQ7+Dgbt4aKDVVWk2i4jjR+tyA2uYwKDDEsJxkgwV+uzeVnnbiLJDtelxHGdG1BXHmA0Xy7Be0GpqNsNoNVD3dRmv06ODyauLWJX3nizk/hTTpRQ/ZpVRD30y16FKEXGzVpM/2A3JASHDZcnEee5D16A8zVg2urug4Y4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=tu-dortmund.de; spf=pass smtp.mailfrom=tu-dortmund.de; dkim=pass (1024-bit key) header.d=tu-dortmund.de header.i=@tu-dortmund.de header.b=Yjzp2hI/; arc=none smtp.client-ip=129.217.128.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=tu-dortmund.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=tu-dortmund.de Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=tu-dortmund.de header.i=@tu-dortmund.de header.b="Yjzp2hI/" Received: from [129.217.186.110] ([129.217.186.110]) (authenticated bits=0) by unimail.uni-dortmund.de (8.18.1.10/8.18.1.10) with ESMTPSA id 5A5Fl8Vb013383 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 5 Nov 2025 16:47:08 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tu-dortmund.de; s=unimail; t=1762357629; bh=O2PvKUUds593hhJv0sfT6cPaqkLEQtmgvK+gnKL+Ibs=; h=Date:Subject:From:To:Cc:References:In-Reply-To; b=Yjzp2hI/n4ZMbEDeRY9upUk9Ju/pFdronBalBQiy5hGTia20nbzSeFXIICDmG1qBB krZ3VXPCBn7h46/nXkOO8ckPywoCKDTFUgSFQoIBMf1lwQxJXsBUgQn4HHK6aY9gJb JmSf/HphExjkbHGBi1tzXQE8LWAHGPkHMO2Ztl44= Message-ID: <64a963ed-400e-4bd2-a4e3-6357f3480367@tu-dortmund.de> Date: Wed, 5 Nov 2025 16:47:08 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: [PATCH net-next v1 1/1] usbnet: Add support for Byte Queue Limits (BQL) From: Simon Schippers To: Eric Dumazet Cc: oneukum@suse.com, andrew+netdev@lunn.ch, davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org References: <20251104161327.41004-1-simon.schippers@tu-dortmund.de> <20251104161327.41004-2-simon.schippers@tu-dortmund.de> <66d22955-bb20-44cf-8ad3-743ae272fec7@tu-dortmund.de> <9ebd72d0-5ae9-4844-b0be-5629c52e6df8@tu-dortmund.de> Content-Language: en-US In-Reply-To: <9ebd72d0-5ae9-4844-b0be-5629c52e6df8@tu-dortmund.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 >> wrote: >>> >>> On 11/5/25 13:34, Eric Dumazet wrote: >>>> On Wed, Nov 5, 2025 at 4:20 AM Simon Schippers >>>> wrote: >>>>> >>>>> On 11/5/25 12:28, Eric Dumazet wrote: >>>>>> On Wed, Nov 5, 2025 at 2:35 AM Simon Schippers >>>>>> wrote: >>>>>>> >>>>>>> On 11/4/25 18:00, Eric Dumazet wrote: >>>>>>>> On Tue, Nov 4, 2025 at 8:14 AM Simon Schippers >>>>>>>> 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 >>>>>>>>> --- >>>>>>>>> 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 >>> >>> 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