Search Linux Wireless

Re: [PATCH 2/2] wifi: rtw88: usb: Preallocate and reuse the RX skbs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 26/11/2024 03:26, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
>> On 19/11/2024 02:50, Ping-Ke Shih wrote:
>>> Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> 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.
>>>>
>>>> Move C2H_ADAPTIVITY to the c2h workqueue instead of handling it directly
>>>> from rtw_fw_c2h_cmd_rx_irqsafe(), which runs in the RX workqueue. It
>>>> reads hardware registers, which is not "irqsafe" with USB.
>>>
>>> This part should be in a separated patch.
>>>
>>
>> Do you mean just C2H_ADAPTIVITY or WQ_BH as well?
> 
> Just C2H_ADAPTIVITY.
> 
> With patch subject, people can't understand this change. 
> 
>>
>>>>
>>>> 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@xxxxxxxxx/
>>>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
>>>> ---
>>>>  drivers/net/wireless/realtek/rtw88/fw.c  |  7 +--
>>>>  drivers/net/wireless/realtek/rtw88/usb.c | 73 ++++++++++++++++++++----
>>>>  drivers/net/wireless/realtek/rtw88/usb.h |  3 +
>>>>  3 files changed, 67 insertions(+), 16 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;
>>>> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
>>>> index 727569d4ef4b..5c2dfa2ced92 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,7 +612,11 @@ 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 - RTW_USB_RXCB_NUM)
>>>> +                       dev_kfree_skb_any(rx_skb);
>>>> +               else
>>>> +                       skb_queue_tail(&rtwusb->rx_free_queue, rx_skb);
>>>
>>> Why not just queue and reuse rx_skb for all? That would be simpler.
>>>
>>
>> I didn't want to let it allocate too many skbs. I didn't find any kind
>> of limit in the official drivers, so maybe it would be fine.
> 
> The case ' skb_queue_len(&rtwusb->rx_free_queue) >= 8 - 4' is rare? 
> If so, I think this is fine. If not, to repeatedly allocate and free 
> would cause memory fragment, and higher probability to failed to allocate
> memory with GFP_ATOMIC. Also seemingly additional 4 persistent rx_skb
> is not a big cost. 
> 
> 

I feel like it's rare for me, but I never counted how often it happens.

Someone else reported that in a period of 14.5 hours additional skbs
were allocated and freed 295 times. This is their log:
https://github.com/user-attachments/files/17792914/dmesg_grep_rtw_8821au.txt
This computer has i7-6700 CPU with 32 GB of RAM.

Another user reported that additional skbs were allocated and freed
3824 times in a period of 1.25 hours. This is their log:
https://github.com/user-attachments/files/18008735/log.txt
This computer is a Raspberry Pi 3B with 1 GB RAM.

I agree that another 4 skbs would be fine. I don't know what the upper
limit should be. The user with the Raspberry Pi 3B also tested with no
limit and reported that the driver allocated 17 extra skbs. That seems
like too many to me.




[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux