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

---
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. 

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.


[1] https://lore.kernel.org/all/20240221194052.927623-1-surenb@google.com/
---
xhci keeps allocing new memory when enque a urb for private data,
and enque frequency could be high, about 250/s when using a usb
webcam, about 30/s for high pace USB keyboard/mouse usage.
Using a cache/buffer for those private data could avoid
lots memory allocations.

Signed-off-by: David Wang <00107082@163.com>
---
 drivers/usb/host/xhci-ring.c |  3 ++-
 drivers/usb/host/xhci.c      | 14 +++++++++++---
 include/linux/usb.h          |  1 +
 3 files changed, 14 insertions(+), 4 deletions(-)

Hi,

On 13.05.25 07:54, David Wang wrote:
> URB objects have long lifecycle, an urb can be reused between
> enqueue-dequeue loops; The private data needed by some host controller
> has very short lifecycle, the memory is alloced when enqueue, and
> released when dequeue. For example, on a system with xhci, several
> minutes of usage of webcam/keyboard/mouse have memory alloc counts:
>    drivers/usb/core/urb.c:75 [usbcore] func:usb_alloc_urb 661
>    drivers/usb/host/xhci.c:1555 [xhci_hcd] func:xhci_urb_enqueue 424863
> Memory allocation frequency for host-controller private data can reach
> ~1k/s.

First of all, thank you for trying to tackle this long running issue.

> @@ -77,6 +78,7 @@ struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags)
>   	if (!urb)
>   		return NULL;
>   	usb_init_urb(urb);
> +	urb->hcpriv_mempool_flags = mem_flags;

No. You cannot do this. The flags you pass to usb_alloc_urb()
depend on the context you call it in. For example, if you are
allocating it while holding a spinlock, ou need to use GFP_ATOMIC

But that may or may not be the same context you submit the URB in.
Recording mem_flags here makes no sense.

	Regards
		Oliver

At 2025-05-13 16:11:20, "Oliver Neukum" <oneukum@suse.com> wrote:
>Hi,
>
>On 13.05.25 07:54, David Wang wrote:
>> URB objects have long lifecycle, an urb can be reused between
>> enqueue-dequeue loops; The private data needed by some host controller
>> has very short lifecycle, the memory is alloced when enqueue, and
>> released when dequeue. For example, on a system with xhci, several
>> minutes of usage of webcam/keyboard/mouse have memory alloc counts:
>>    drivers/usb/core/urb.c:75 [usbcore] func:usb_alloc_urb 661
>>    drivers/usb/host/xhci.c:1555 [xhci_hcd] func:xhci_urb_enqueue 424863
>> Memory allocation frequency for host-controller private data can reach
>> ~1k/s.
>
>First of all, thank you for trying to tackle this long running issue.
>
>> @@ -77,6 +78,7 @@ struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags)
>>   	if (!urb)
>>   		return NULL;
>>   	usb_init_urb(urb);
>> +	urb->hcpriv_mempool_flags = mem_flags;
>
>No. You cannot do this. The flags you pass to usb_alloc_urb()
>depend on the context you call it in. For example, if you are
>allocating it while holding a spinlock, ou need to use GFP_ATOMIC
>
>But that may or may not be the same context you submit the URB in.
>Recording mem_flags here makes no sense.
>
>	Regards
>		Oliver

Thanks for reviewing this.  The memory flag thing do raise concern. 
I think I can make adjustment:  realloc the memory if flag changed.


Thanks
David

On 13.05.25 10:23, David Wang wrote:

Hi,
  > Thanks for reviewing this.  The memory flag thing do raise concern.
> I think I can make adjustment:  realloc the memory if flag changed.

I am sorry. I have been unclear. Here comes a detailed explanation:

What we call "gfp_t" is a combination of flags. They describe

A - the type of memory (always valid)
B - the way the memory can be allocated (valid only at a specific time)

The URB is a generic data structure to be processed by the CPU, _not_
the HC. It is always generic kernel memory. Flags of type A make no sense
to pass.
In fact you may not know for which device an URB will be used when you
allocate it. The only valid mem_flags you can pass to usb_alloc_urb()
are GFP_KERNEL, GFP_NOIO or GFP_ATOMIC.

If you need to reallocate memory for private data you _must_ use
the flags passed with usb_submit_urb(). A HCD can modify them by adding
flags of type A, but you cannot change flags of type B.
For example, if usb_alloc_urb() used GFP_KERNEL to allocate the URB,
but uses GFP_ATOMIC in usb_submit_urb(), you will deadlock if you save
and reuse the GFP_KERNEL.

	HTH
		Oliver

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.

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.

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.

Thanks
Mathias

At 2025-05-13 16:46:39, "Oliver Neukum" <oneukum@suse.com> wrote:
>On 13.05.25 10:23, David Wang wrote:
>
>Hi,
>  > Thanks for reviewing this.  The memory flag thing do raise concern.
>> I think I can make adjustment:  realloc the memory if flag changed.
>
>I am sorry. I have been unclear. Here comes a detailed explanation:
>
>What we call "gfp_t" is a combination of flags. They describe
>
>A - the type of memory (always valid)
>B - the way the memory can be allocated (valid only at a specific time)
>
>The URB is a generic data structure to be processed by the CPU, _not_
>the HC. It is always generic kernel memory. Flags of type A make no sense
>to pass.
>In fact you may not know for which device an URB will be used when you
>allocate it. The only valid mem_flags you can pass to usb_alloc_urb()
>are GFP_KERNEL, GFP_NOIO or GFP_ATOMIC.
>
>If you need to reallocate memory for private data you _must_ use
>the flags passed with usb_submit_urb(). A HCD can modify them by adding
>flags of type A, but you cannot change flags of type B.
>For example, if usb_alloc_urb() used GFP_KERNEL to allocate the URB,
>but uses GFP_ATOMIC in usb_submit_urb(), you will deadlock if you save
>and reuse the GFP_KERNEL.
>
>	HTH
>		Oliver
>  

Hi, I have one question about mem flags.
If usb_submit_urb wants a memory in context of flags A, say GFP_ATOMIC, but I already have a memory alloc with  flags B  and its size
is big enough,  is it safe to return this memory  to usb_submit_urb  which is in the context of flags A? 


Thanks
David

On 13.05.25 11:49, David Wang wrote:

> Hi, I have one question about mem flags.
> If usb_submit_urb wants a memory in context of flags A, say GFP_ATOMIC, but I already have a memory alloc with  flags B  and its size
> is big enough,  is it safe to return this memory  to usb_submit_urb  which is in the context of flags A?

Yes, that is perfectly safe.

	HTH
		Oliver