linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb
@ 2024-12-18 22:33 Bitterblue Smith
  2024-12-18 22:34 ` [PATCH v2 2/3] wifi: rtw88: Handle C2H_ADAPTIVITY in rtw_fw_c2h_cmd_handle() Bitterblue Smith
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Bitterblue Smith @ 2024-12-18 22:33 UTC (permalink / raw)
  To: linux-wireless@vger.kernel.org; +Cc: Ping-Ke Shih, Sascha Hauer

"iperf3 -c 192.168.0.1 -R --udp -b 0" shows about 40% of datagrams
are lost. Many torrents don't download faster than 3 MiB/s, probably
because the Bittorrent protocol uses UDP. This is somehow related to
the use of skb_clone() in the RX path.

Don't use skb_clone(). Instead allocate a new skb for each 802.11 frame
received and copy the data from the big (32768 byte) skb.

With this patch, "iperf3 -c 192.168.0.1 -R --udp -b 0" shows only 1-2%
of datagrams are lost, and torrents can reach download speeds of 36
MiB/s.

Tested with RTL8812AU and RTL8822CU.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
v2:
 - No change.
---
 drivers/net/wireless/realtek/rtw88/usb.c | 52 ++++++++++++++----------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index 34df371e599b..4dbd5167faa4 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -7,6 +7,7 @@
 #include <linux/mutex.h>
 #include "main.h"
 #include "debug.h"
+#include "mac.h"
 #include "reg.h"
 #include "tx.h"
 #include "rx.h"
@@ -546,49 +547,58 @@ static void rtw_usb_rx_handler(struct work_struct *work)
 {
 	struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_work);
 	struct rtw_dev *rtwdev = rtwusb->rtwdev;
-	const struct rtw_chip_info *chip = rtwdev->chip;
-	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
 	struct ieee80211_rx_status rx_status;
-	u32 pkt_offset, next_pkt, urb_len;
 	struct rtw_rx_pkt_stat pkt_stat;
-	struct sk_buff *next_skb;
+	struct sk_buff *rx_skb;
 	struct sk_buff *skb;
+	u32 pkt_desc_sz = rtwdev->chip->rx_pkt_desc_sz;
+	u32 max_skb_len = pkt_desc_sz + PHY_STATUS_SIZE * 8 +
+			  IEEE80211_MAX_MPDU_LEN_VHT_11454;
+	u32 pkt_offset, next_pkt, skb_len;
 	u8 *rx_desc;
 	int limit;
 
 	for (limit = 0; limit < 200; limit++) {
-		skb = skb_dequeue(&rtwusb->rx_queue);
-		if (!skb)
+		rx_skb = skb_dequeue(&rtwusb->rx_queue);
+		if (!rx_skb)
 			break;
 
 		if (skb_queue_len(&rtwusb->rx_queue) >= RTW_USB_MAX_RXQ_LEN) {
 			dev_dbg_ratelimited(rtwdev->dev, "failed to get rx_queue, overflow\n");
-			dev_kfree_skb_any(skb);
+			dev_kfree_skb_any(rx_skb);
 			continue;
 		}
 
-		urb_len = skb->len;
+		rx_desc = rx_skb->data;
 
 		do {
-			rx_desc = skb->data;
 			rtw_rx_query_rx_desc(rtwdev, rx_desc, &pkt_stat,
 					     &rx_status);
 			pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
 				     pkt_stat.shift;
 
-			next_pkt = round_up(pkt_stat.pkt_len + pkt_offset, 8);
+			skb_len = pkt_stat.pkt_len + pkt_offset;
+			if (skb_len > max_skb_len) {
+				rtw_dbg(rtwdev, RTW_DBG_USB,
+					"skipping too big packet: %u\n",
+					skb_len);
+				goto skip_packet;
+			}
 
-			if (urb_len >= next_pkt + pkt_desc_sz)
-				next_skb = skb_clone(skb, GFP_KERNEL);
-			else
-				next_skb = NULL;
+			skb = alloc_skb(skb_len, GFP_KERNEL);
+			if (!skb) {
+				rtw_dbg(rtwdev, RTW_DBG_USB,
+					"failed to allocate RX skb of size %u\n",
+					skb_len);
+				goto skip_packet;
+			}
+
+			skb_put_data(skb, rx_desc, skb_len);
 
 			if (pkt_stat.is_c2h) {
-				skb_trim(skb, pkt_stat.pkt_len + pkt_offset);
 				rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
 			} else {
 				skb_pull(skb, pkt_offset);
-				skb_trim(skb, pkt_stat.pkt_len);
 				rtw_update_rx_freq_for_invalid(rtwdev, skb,
 							       &rx_status,
 							       &pkt_stat);
@@ -597,12 +607,12 @@ static void rtw_usb_rx_handler(struct work_struct *work)
 				ieee80211_rx_irqsafe(rtwdev->hw, skb);
 			}
 
-			skb = next_skb;
-			if (skb)
-				skb_pull(skb, next_pkt);
+skip_packet:
+			next_pkt = round_up(skb_len, 8);
+			rx_desc += next_pkt;
+		} while (rx_desc + pkt_desc_sz < rx_skb->data + rx_skb->len);
 
-			urb_len -= next_pkt;
-		} while (skb);
+		dev_kfree_skb_any(rx_skb);
 	}
 }
 
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] wifi: rtw88: Handle C2H_ADAPTIVITY in rtw_fw_c2h_cmd_handle()
  2024-12-18 22:33 [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb Bitterblue Smith
@ 2024-12-18 22:34 ` Bitterblue Smith
  2024-12-19  6:19   ` Ping-Ke Shih
  2024-12-18 22:35 ` [PATCH v2 3/3] wifi: rtw88: usb: Preallocate and reuse the RX skbs Bitterblue Smith
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Bitterblue Smith @ 2024-12-18 22:34 UTC (permalink / raw)
  To: linux-wireless@vger.kernel.org; +Cc: Ping-Ke Shih, Sascha Hauer

The firmware message C2H_ADAPTIVITY is currently handled in
rtw_fw_c2h_cmd_rx_irqsafe(), which runs in the RX workqueue, but it's
not "irqsafe" with USB because it sleeps (reads hardware registers).
This becomes a problem after the next patch, which will create the RX
workqueue with the flag WQ_BH.

To avoid sleeping when it's not allowed, handle C2H_ADAPTIVITY in
rtw_fw_c2h_cmd_handle(), which runs in the c2h workqueue.

Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
v2:
 - Patch is new in v2.
---
 drivers/net/wireless/realtek/rtw88/fw.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
index e6e9946fbf44..02389b7c6876 100644
--- a/drivers/net/wireless/realtek/rtw88/fw.c
+++ b/drivers/net/wireless/realtek/rtw88/fw.c
@@ -332,6 +332,9 @@ void rtw_fw_c2h_cmd_handle(struct rtw_dev *rtwdev, struct sk_buff *skb)
 	case C2H_RA_RPT:
 		rtw_fw_ra_report_handle(rtwdev, c2h->payload, len);
 		break;
+	case C2H_ADAPTIVITY:
+		rtw_fw_adaptivity_result(rtwdev, c2h->payload, len);
+		break;
 	default:
 		rtw_dbg(rtwdev, RTW_DBG_FW, "C2H 0x%x isn't handled\n", c2h->id);
 		break;
@@ -367,10 +370,6 @@ void rtw_fw_c2h_cmd_rx_irqsafe(struct rtw_dev *rtwdev, u32 pkt_offset,
 		rtw_fw_scan_result(rtwdev, c2h->payload, len);
 		dev_kfree_skb_any(skb);
 		break;
-	case C2H_ADAPTIVITY:
-		rtw_fw_adaptivity_result(rtwdev, c2h->payload, len);
-		dev_kfree_skb_any(skb);
-		break;
 	default:
 		/* pass offset for further operation */
 		*((u32 *)skb->cb) = pkt_offset;
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] wifi: rtw88: usb: Preallocate and reuse the RX skbs
  2024-12-18 22:33 [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb Bitterblue Smith
  2024-12-18 22:34 ` [PATCH v2 2/3] wifi: rtw88: Handle C2H_ADAPTIVITY in rtw_fw_c2h_cmd_handle() Bitterblue Smith
@ 2024-12-18 22:35 ` Bitterblue Smith
  2024-12-19  7:18   ` Ping-Ke Shih
  2024-12-19  6:17 ` [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb Ping-Ke Shih
  2024-12-23  8:07 ` Ping-Ke Shih
  3 siblings, 1 reply; 9+ messages in thread
From: Bitterblue Smith @ 2024-12-18 22:35 UTC (permalink / raw)
  To: linux-wireless@vger.kernel.org; +Cc: Ping-Ke Shih, Sascha Hauer

The USB driver uses four USB Request Blocks for RX. Before submitting
one, it allocates a 32768 byte skb for the RX data. This allocation can
fail, maybe due to temporary memory fragmentation. When the allocation
fails, the corresponding URB is never submitted again. After four such
allocation failures, all RX stops because the driver is not requesting
data from the device anymore.

Don't allocate a 32768 byte skb when submitting a USB Request Block
(which happens very often). Instead preallocate 8 such skbs, and reuse
them over and over. If all 8 are busy, allocate a new one. This is
pretty rare. If the allocation fails, use a work to try again later.
When there are enough free skbs again, free the excess skbs.

Also, use WQ_BH for the RX workqueue. With a normal or high priority
workqueue the skbs are processed too slowly when the system is even a
little busy, like when opening a new page in a browser, and the driver
runs out of free skbs and allocates a lot of new ones.

This is more or less what the out-of-tree Realtek drivers do, except
they use a tasklet instead of a BH workqueue.

Tested with RTL8723DU, RTL8821AU, RTL8812AU, RTL8812BU, RTL8822CU,
RTL8811CU.

Closes: https://lore.kernel.org/linux-wireless/6e7ecb47-7ea0-433a-a19f-05f88a2edf6b@gmail.com/
Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
---
v2:
 - Pass the appropriate gfp_t flags as a parameter to
   rtw_usb_rx_resubmit() instead of using in_interrupt(). Call it "gfp"
   instead of "priority".
 - Initialise rx_skb a little earlier in rtw_usb_rx_resubmit().
 - Move C2H_ADAPTIVITY in a new patch (2/3).
 - Allow the driver to allocate 4 more persistent skbs after the initial
   8.
---
 drivers/net/wireless/realtek/rtw88/usb.c | 79 +++++++++++++++++++-----
 drivers/net/wireless/realtek/rtw88/usb.h |  3 +
 2 files changed, 67 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index 4dbd5167faa4..b405f8317021 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -585,7 +585,7 @@ static void rtw_usb_rx_handler(struct work_struct *work)
 				goto skip_packet;
 			}
 
-			skb = alloc_skb(skb_len, GFP_KERNEL);
+			skb = alloc_skb(skb_len, GFP_ATOMIC);
 			if (!skb) {
 				rtw_dbg(rtwdev, RTW_DBG_USB,
 					"failed to allocate RX skb of size %u\n",
@@ -612,32 +612,70 @@ static void rtw_usb_rx_handler(struct work_struct *work)
 			rx_desc += next_pkt;
 		} while (rx_desc + pkt_desc_sz < rx_skb->data + rx_skb->len);
 
-		dev_kfree_skb_any(rx_skb);
+		if (skb_queue_len(&rtwusb->rx_free_queue) >= RTW_USB_RX_SKB_NUM)
+			dev_kfree_skb_any(rx_skb);
+		else
+			skb_queue_tail(&rtwusb->rx_free_queue, rx_skb);
 	}
 }
 
 static void rtw_usb_read_port_complete(struct urb *urb);
 
-static void rtw_usb_rx_resubmit(struct rtw_usb *rtwusb, struct rx_usb_ctrl_block *rxcb)
+static void rtw_usb_rx_resubmit(struct rtw_usb *rtwusb,
+				struct rx_usb_ctrl_block *rxcb,
+				gfp_t gfp)
 {
 	struct rtw_dev *rtwdev = rtwusb->rtwdev;
+	struct sk_buff *rx_skb;
 	int error;
 
-	rxcb->rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, GFP_ATOMIC);
-	if (!rxcb->rx_skb)
-		return;
+	rx_skb = skb_dequeue(&rtwusb->rx_free_queue);
+	if (!rx_skb)
+		rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, gfp);
+
+	if (!rx_skb)
+		goto try_later;
+
+	skb_reset_tail_pointer(rx_skb);
+	rx_skb->len = 0;
+
+	rxcb->rx_skb = rx_skb;
 
 	usb_fill_bulk_urb(rxcb->rx_urb, rtwusb->udev,
 			  usb_rcvbulkpipe(rtwusb->udev, rtwusb->pipe_in),
 			  rxcb->rx_skb->data, RTW_USB_MAX_RECVBUF_SZ,
 			  rtw_usb_read_port_complete, rxcb);
 
-	error = usb_submit_urb(rxcb->rx_urb, GFP_ATOMIC);
+	error = usb_submit_urb(rxcb->rx_urb, gfp);
 	if (error) {
-		kfree_skb(rxcb->rx_skb);
+		skb_queue_tail(&rtwusb->rx_free_queue, rxcb->rx_skb);
+
 		if (error != -ENODEV)
 			rtw_err(rtwdev, "Err sending rx data urb %d\n",
 				error);
+
+		if (error == -ENOMEM)
+			goto try_later;
+	}
+
+	return;
+
+try_later:
+	rxcb->rx_skb = NULL;
+	queue_work(rtwusb->rxwq, &rtwusb->rx_urb_work);
+}
+
+static void rtw_usb_rx_resubmit_work(struct work_struct *work)
+{
+	struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_urb_work);
+	struct rx_usb_ctrl_block *rxcb;
+	int i;
+
+	for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
+		rxcb = &rtwusb->rx_cb[i];
+
+		if (!rxcb->rx_skb)
+			rtw_usb_rx_resubmit(rtwusb, rxcb, GFP_ATOMIC);
 	}
 }
 
@@ -653,15 +691,16 @@ static void rtw_usb_read_port_complete(struct urb *urb)
 		    urb->actual_length < 24) {
 			rtw_err(rtwdev, "failed to get urb length:%d\n",
 				urb->actual_length);
-			if (skb)
-				dev_kfree_skb_any(skb);
+			skb_queue_tail(&rtwusb->rx_free_queue, skb);
 		} else {
 			skb_put(skb, urb->actual_length);
 			skb_queue_tail(&rtwusb->rx_queue, skb);
 			queue_work(rtwusb->rxwq, &rtwusb->rx_work);
 		}
-		rtw_usb_rx_resubmit(rtwusb, rxcb);
+		rtw_usb_rx_resubmit(rtwusb, rxcb, GFP_ATOMIC);
 	} else {
+		skb_queue_tail(&rtwusb->rx_free_queue, skb);
+
 		switch (urb->status) {
 		case -EINVAL:
 		case -EPIPE:
@@ -679,8 +718,6 @@ static void rtw_usb_read_port_complete(struct urb *urb)
 			rtw_err(rtwdev, "status %d\n", urb->status);
 			break;
 		}
-		if (skb)
-			dev_kfree_skb_any(skb);
 	}
 }
 
@@ -868,16 +905,26 @@ static struct rtw_hci_ops rtw_usb_ops = {
 static int rtw_usb_init_rx(struct rtw_dev *rtwdev)
 {
 	struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
+	struct sk_buff *rx_skb;
+	int i;
 
-	rtwusb->rxwq = create_singlethread_workqueue("rtw88_usb: rx wq");
+	rtwusb->rxwq = alloc_workqueue("rtw88_usb: rx wq", WQ_BH, 0);
 	if (!rtwusb->rxwq) {
 		rtw_err(rtwdev, "failed to create RX work queue\n");
 		return -ENOMEM;
 	}
 
 	skb_queue_head_init(&rtwusb->rx_queue);
+	skb_queue_head_init(&rtwusb->rx_free_queue);
 
 	INIT_WORK(&rtwusb->rx_work, rtw_usb_rx_handler);
+	INIT_WORK(&rtwusb->rx_urb_work, rtw_usb_rx_resubmit_work);
+
+	for (i = 0; i < RTW_USB_RX_SKB_NUM; i++) {
+		rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, GFP_KERNEL);
+		if (rx_skb)
+			skb_queue_tail(&rtwusb->rx_free_queue, rx_skb);
+	}
 
 	return 0;
 }
@@ -890,7 +937,7 @@ static void rtw_usb_setup_rx(struct rtw_dev *rtwdev)
 	for (i = 0; i < RTW_USB_RXCB_NUM; i++) {
 		struct rx_usb_ctrl_block *rxcb = &rtwusb->rx_cb[i];
 
-		rtw_usb_rx_resubmit(rtwusb, rxcb);
+		rtw_usb_rx_resubmit(rtwusb, rxcb, GFP_KERNEL);
 	}
 }
 
@@ -902,6 +949,8 @@ static void rtw_usb_deinit_rx(struct rtw_dev *rtwdev)
 
 	flush_workqueue(rtwusb->rxwq);
 	destroy_workqueue(rtwusb->rxwq);
+
+	skb_queue_purge(&rtwusb->rx_free_queue);
 }
 
 static int rtw_usb_init_tx(struct rtw_dev *rtwdev)
diff --git a/drivers/net/wireless/realtek/rtw88/usb.h b/drivers/net/wireless/realtek/rtw88/usb.h
index 86697a5c0103..9b695b688b24 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.h
+++ b/drivers/net/wireless/realtek/rtw88/usb.h
@@ -38,6 +38,7 @@
 #define RTW_USB_RXAGG_TIMEOUT		10
 
 #define RTW_USB_RXCB_NUM		4
+#define RTW_USB_RX_SKB_NUM		8
 
 #define RTW_USB_EP_MAX			4
 
@@ -81,7 +82,9 @@ struct rtw_usb {
 
 	struct rx_usb_ctrl_block rx_cb[RTW_USB_RXCB_NUM];
 	struct sk_buff_head rx_queue;
+	struct sk_buff_head rx_free_queue;
 	struct work_struct rx_work;
+	struct work_struct rx_urb_work;
 };
 
 static inline struct rtw_usb_tx_data *rtw_usb_get_tx_data(struct sk_buff *skb)
-- 
2.47.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb
  2024-12-18 22:33 [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb Bitterblue Smith
  2024-12-18 22:34 ` [PATCH v2 2/3] wifi: rtw88: Handle C2H_ADAPTIVITY in rtw_fw_c2h_cmd_handle() Bitterblue Smith
  2024-12-18 22:35 ` [PATCH v2 3/3] wifi: rtw88: usb: Preallocate and reuse the RX skbs Bitterblue Smith
@ 2024-12-19  6:17 ` Ping-Ke Shih
  2024-12-19 11:15   ` Bitterblue Smith
  2024-12-23  8:07 ` Ping-Ke Shih
  3 siblings, 1 reply; 9+ messages in thread
From: Ping-Ke Shih @ 2024-12-19  6:17 UTC (permalink / raw)
  To: Bitterblue Smith, linux-wireless@vger.kernel.org; +Cc: Sascha Hauer

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:

> +                       skb = alloc_skb(skb_len, GFP_KERNEL);
> +                       if (!skb) {
> +                               rtw_dbg(rtwdev, RTW_DBG_USB,
> +                                       "failed to allocate RX skb of size %u\n",
> +                                       skb_len);

checkpatch warns this is unnecessary. 

WARNING: Possible unnecessary 'out of memory' message
#94: FILE: drivers/net/wireless/realtek/rtw88/usb.c:591:
+                       if (!skb) {
+                               rtw_dbg(rtwdev, RTW_DBG_USB,




^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v2 2/3] wifi: rtw88: Handle C2H_ADAPTIVITY in rtw_fw_c2h_cmd_handle()
  2024-12-18 22:34 ` [PATCH v2 2/3] wifi: rtw88: Handle C2H_ADAPTIVITY in rtw_fw_c2h_cmd_handle() Bitterblue Smith
@ 2024-12-19  6:19   ` Ping-Ke Shih
  0 siblings, 0 replies; 9+ messages in thread
From: Ping-Ke Shih @ 2024-12-19  6:19 UTC (permalink / raw)
  To: Bitterblue Smith, linux-wireless@vger.kernel.org; +Cc: Sascha Hauer

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> The firmware message C2H_ADAPTIVITY is currently handled in
> rtw_fw_c2h_cmd_rx_irqsafe(), which runs in the RX workqueue, but it's
> not "irqsafe" with USB because it sleeps (reads hardware registers).
> This becomes a problem after the next patch, which will create the RX
> workqueue with the flag WQ_BH.
> 
> To avoid sleeping when it's not allowed, handle C2H_ADAPTIVITY in
> rtw_fw_c2h_cmd_handle(), which runs in the c2h workqueue.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v2 3/3] wifi: rtw88: usb: Preallocate and reuse the RX skbs
  2024-12-18 22:35 ` [PATCH v2 3/3] wifi: rtw88: usb: Preallocate and reuse the RX skbs Bitterblue Smith
@ 2024-12-19  7:18   ` Ping-Ke Shih
  0 siblings, 0 replies; 9+ messages in thread
From: Ping-Ke Shih @ 2024-12-19  7:18 UTC (permalink / raw)
  To: Bitterblue Smith, linux-wireless@vger.kernel.org; +Cc: Sascha Hauer

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> The USB driver uses four USB Request Blocks for RX. Before submitting
> one, it allocates a 32768 byte skb for the RX data. This allocation can
> fail, maybe due to temporary memory fragmentation. When the allocation
> fails, the corresponding URB is never submitted again. After four such
> allocation failures, all RX stops because the driver is not requesting
> data from the device anymore.
> 
> Don't allocate a 32768 byte skb when submitting a USB Request Block
> (which happens very often). Instead preallocate 8 such skbs, and reuse
> them over and over. If all 8 are busy, allocate a new one. This is
> pretty rare. If the allocation fails, use a work to try again later.
> When there are enough free skbs again, free the excess skbs.
> 
> Also, use WQ_BH for the RX workqueue. With a normal or high priority
> workqueue the skbs are processed too slowly when the system is even a
> little busy, like when opening a new page in a browser, and the driver
> runs out of free skbs and allocates a lot of new ones.
> 
> This is more or less what the out-of-tree Realtek drivers do, except
> they use a tasklet instead of a BH workqueue.
> 
> Tested with RTL8723DU, RTL8821AU, RTL8812AU, RTL8812BU, RTL8822CU,
> RTL8811CU.
> 
> Closes: https://lore.kernel.org/linux-wireless/6e7ecb47-7ea0-433a-a19f-05f88a2edf6b@gmail.com/
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>


> 
> -       error = usb_submit_urb(rxcb->rx_urb, GFP_ATOMIC);
> +       error = usb_submit_urb(rxcb->rx_urb, gfp);
>         if (error) {
> -               kfree_skb(rxcb->rx_skb);
> +               skb_queue_tail(&rtwusb->rx_free_queue, rxcb->rx_skb);
> +
>                 if (error != -ENODEV)
>                         rtw_err(rtwdev, "Err sending rx data urb %d\n",
>                                 error);

Looking into usb_submit_urb(), I think ENODEV means USB device becomes
unavailable. For this case, it seems well for rxcb missed to attach RX URB
anymore, not 'goto try_later'.

> +
> +               if (error == -ENOMEM)
> +                       goto try_later;
> +       }
> +
> +       return;
> +
> +try_later:
> +       rxcb->rx_skb = NULL;
> +       queue_work(rtwusb->rxwq, &rtwusb->rx_urb_work);
> +}



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb
  2024-12-19  6:17 ` [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb Ping-Ke Shih
@ 2024-12-19 11:15   ` Bitterblue Smith
  2024-12-20  0:13     ` Ping-Ke Shih
  0 siblings, 1 reply; 9+ messages in thread
From: Bitterblue Smith @ 2024-12-19 11:15 UTC (permalink / raw)
  To: Ping-Ke Shih, linux-wireless@vger.kernel.org; +Cc: Sascha Hauer

On 19/12/2024 08:17, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> 
>> +                       skb = alloc_skb(skb_len, GFP_KERNEL);
>> +                       if (!skb) {
>> +                               rtw_dbg(rtwdev, RTW_DBG_USB,
>> +                                       "failed to allocate RX skb of size %u\n",
>> +                                       skb_len);
> 
> checkpatch warns this is unnecessary. 
> 
> WARNING: Possible unnecessary 'out of memory' message
> #94: FILE: drivers/net/wireless/realtek/rtw88/usb.c:591:
> +                       if (!skb) {
> +                               rtw_dbg(rtwdev, RTW_DBG_USB,
> 
> 
> 

checkpatch is wrong about this one. alloc_skb doesn't say
anything when it fails.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb
  2024-12-19 11:15   ` Bitterblue Smith
@ 2024-12-20  0:13     ` Ping-Ke Shih
  0 siblings, 0 replies; 9+ messages in thread
From: Ping-Ke Shih @ 2024-12-20  0:13 UTC (permalink / raw)
  To: Bitterblue Smith, linux-wireless@vger.kernel.org; +Cc: Sascha Hauer

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> 
> On 19/12/2024 08:17, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> >
> >> +                       skb = alloc_skb(skb_len, GFP_KERNEL);
> >> +                       if (!skb) {
> >> +                               rtw_dbg(rtwdev, RTW_DBG_USB,
> >> +                                       "failed to allocate RX skb of size %u\n",
> >> +                                       skb_len);
> >
> > checkpatch warns this is unnecessary.
> >
> > WARNING: Possible unnecessary 'out of memory' message
> > #94: FILE: drivers/net/wireless/realtek/rtw88/usb.c:591:
> > +                       if (!skb) {
> > +                               rtw_dbg(rtwdev, RTW_DBG_USB,
> >
> >
> >
> 
> checkpatch is wrong about this one. alloc_skb doesn't say
> anything when it fails.

Got it. Then

Acked-by: Ping-Ke Shih <pkshih@realtek.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb
  2024-12-18 22:33 [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb Bitterblue Smith
                   ` (2 preceding siblings ...)
  2024-12-19  6:17 ` [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb Ping-Ke Shih
@ 2024-12-23  8:07 ` Ping-Ke Shih
  3 siblings, 0 replies; 9+ messages in thread
From: Ping-Ke Shih @ 2024-12-23  8:07 UTC (permalink / raw)
  To: Bitterblue Smith, linux-wireless@vger.kernel.org
  Cc: Ping-Ke Shih, Sascha Hauer

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:

> "iperf3 -c 192.168.0.1 -R --udp -b 0" shows about 40% of datagrams
> are lost. Many torrents don't download faster than 3 MiB/s, probably
> because the Bittorrent protocol uses UDP. This is somehow related to
> the use of skb_clone() in the RX path.
> 
> Don't use skb_clone(). Instead allocate a new skb for each 802.11 frame
> received and copy the data from the big (32768 byte) skb.
> 
> With this patch, "iperf3 -c 192.168.0.1 -R --udp -b 0" shows only 1-2%
> of datagrams are lost, and torrents can reach download speeds of 36
> MiB/s.
> 
> Tested with RTL8812AU and RTL8822CU.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Acked-by: Ping-Ke Shih <pkshih@realtek.com>

3 patch(es) applied to rtw-next branch of rtw.git, thanks.

e9048e2935f7 wifi: rtw88: usb: Copy instead of cloning the RX skb
13221be72034 wifi: rtw88: Handle C2H_ADAPTIVITY in rtw_fw_c2h_cmd_handle()
3e3aa566dd18 wifi: rtw88: usb: Preallocate and reuse the RX skbs

---
https://github.com/pkshih/rtw.git


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-12-23  8:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-18 22:33 [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb Bitterblue Smith
2024-12-18 22:34 ` [PATCH v2 2/3] wifi: rtw88: Handle C2H_ADAPTIVITY in rtw_fw_c2h_cmd_handle() Bitterblue Smith
2024-12-19  6:19   ` Ping-Ke Shih
2024-12-18 22:35 ` [PATCH v2 3/3] wifi: rtw88: usb: Preallocate and reuse the RX skbs Bitterblue Smith
2024-12-19  7:18   ` Ping-Ke Shih
2024-12-19  6:17 ` [PATCH v2 1/3] wifi: rtw88: usb: Copy instead of cloning the RX skb Ping-Ke Shih
2024-12-19 11:15   ` Bitterblue Smith
2024-12-20  0:13     ` Ping-Ke Shih
2024-12-23  8:07 ` Ping-Ke Shih

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).