RE: [PATCH v2 3/3] wifi: rtw88: usb: Preallocate and reuse the RX skbs

[Ping-Ke Shih <pkshih@realtek.com>]

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);
> +}