Re: [RFC] USB: core/xhci: add a buffer in urb for host controller private data

["David Wang" <00107082@163.com>]

At 2025-05-13 17:27:34, "Mathias Nyman" <mathias.nyman@intel.com> wrote:
>Hi David
>
>On 12.5.2025 18.07, David Wang wrote:
>> ---
>> I was checking memory allocation behaviors (via memory profiling[1]),
>> when I notice a high frequent memory allocation in xhci_urb_enqueue, about
>> 250/s when using a USB webcam. If those alloced buffer could be kept and
>> reused, lots of memory allocations could be avoid over time.
>> 
>> This patch is just a POC, about 0/s memory allocation in xhci with this
>> patch, when I use my USB devices, webcam/keyboard/mouse.
>
>Thanks for looking at this, this is something that obviously needs more
>attention
>
>> 
>> A dynamic cached memory would be better: URB keep host controller's
>> private data, if larger size buffer needed for private data, old buffer
>> released and a larger buffer alloced.
>> 
>> I did not observe any nagative impact with xhci's 250/s allocations
>> when using my system, hence no measurement of how useful this changes
>> can make to user. Just want to collect feedbacks before putting more
>> effort.
>> 
>
>I think we can make a xhci internal solution that doesn't affect other hosts
>or usb core.

Yes,  my latest patches separates xhci changes from  usbcore .
>
>How about adding a list of struct urb_priv nodes for every usb device, or maybe
>even per endpoint? i.e. to struct xhci_virt_device or  struct xhci_virt_ep.
>
>Add size and list_head entries to struct urb_priv.
>
device level mempool could be very complicated, I think a single memory pool hold by URB
would be much simpler. (Could you help  take a look at this patch:
https://lore.kernel.org/lkml/20250513055447.5696-1-00107082@163.com/) 

>In xhci_urb_enqueue() pick the first urb_priv node from list if it exists and is
>large enough, otherwise just allocate a new one and set the size.
>
>When giving back the URB zero everything in the struct urb_priv except the size,
>and add it to the list.
>
>When the device is freed there shouldn't be any nodes left in the list, but if
>there are then warn and free them.
>
>Isoc transfers are the ones with odd urb_priv sizes. If we have a per device or
>per endpoint urb_priv list then sizes will match better.

If the mempool is in URB, then only largest size matters, since no two HC can own the URB
at the same time.

>
>Thanks
>Mathias
>

Thanks for the comments and suggestions.
David