Re: [PATCH v3 1/2] USB: core: add a memory pool to urb caching host-controller private data

[Greg KH <gregkh@linuxfoundation.org>]

On Sat, May 17, 2025 at 04:38:19PM +0800, David Wang wrote:
> ---
> Changes since v2:
> 1. activat the pool only when the urb object is created via
> usb_alloc_urb()
> Thanks to Oliver Neukum <oneukum@suse.com>'s review.

Changes go below the bottom --- line, not at the top.  Please read the
documentation for how to do this.

Also, these are not "threaded" together, making them hard to pick out.
Please when you resend, make them be together using git send-email or
some such tool.

> ---
> URB objects have long lifecycle, an urb can be reused between
> submit 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, in
> xhci_urb_enqueue:
> Using a USB webcam would have ~250/s memory allocation;
> Using a USB mic would have ~1K/s memory allocation;
> 
> High frequent allocations for host-controller private data can be
> avoided if urb take over the ownership of memory, the memory then shares
> the longer lifecycle with urb objects.
> 
> Add a mempool to urb for hcpriv usage, the mempool only holds one block
> of memory and grows when larger size is requested.
> 
> The mempool is activated only when the URB object is created via
> usb_alloc_urb() in case some drivers create a URB object by other
> means and manage it lifecycle without corresponding usb_free_urb.
> 
> The performance difference with this change is insignificant when
> system is under no memory pressure or under heavy memory pressure.
> There could be a point inbetween where extra 1k/s memory alloction
> would dominate the preformance, but very hard to pinpoint it.
> 
> Signed-off-by: David Wang <00107082@163.com>
> ---
>  drivers/usb/core/urb.c | 45 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h    |  5 +++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 5e52a35486af..53117743150f 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -23,6 +23,8 @@ static void urb_destroy(struct kref *kref)
>  
>  	if (urb->transfer_flags & URB_FREE_BUFFER)
>  		kfree(urb->transfer_buffer);
> +	if (urb->hcpriv_mempool_activated)
> +		kfree(urb->hcpriv_mempool);
>  
>  	kfree(urb);
>  }
> @@ -77,6 +79,8 @@ struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags)
>  	if (!urb)
>  		return NULL;
>  	usb_init_urb(urb);
> +	/* activate hcpriv mempool when urb is created via usb_alloc_urb */
> +	urb->hcpriv_mempool_activated = true;
>  	return urb;
>  }
>  EXPORT_SYMBOL_GPL(usb_alloc_urb);
> @@ -1037,3 +1041,44 @@ int usb_anchor_empty(struct usb_anchor *anchor)
>  
>  EXPORT_SYMBOL_GPL(usb_anchor_empty);
>  
> +/**
> + * urb_hcpriv_mempool_zalloc - alloc memory from mempool for hcpriv
> + * @urb: pointer to URB being used
> + * @size: memory size requested by current host controller
> + * @mem_flags: the type of memory to allocate
> + *
> + * Return: NULL if out of memory, otherwise memory are zeroed
> + */
> +void *urb_hcpriv_mempool_zalloc(struct urb *urb, size_t size, gfp_t mem_flags)
> +{
> +	if (!urb->hcpriv_mempool_activated)
> +		return kzalloc(size, mem_flags);
> +
> +	if (urb->hcpriv_mempool_size < size) {
> +		kfree(urb->hcpriv_mempool);
> +		urb->hcpriv_mempool_size = size;
> +		urb->hcpriv_mempool = kmalloc(size, mem_flags);
> +	}
> +	if (urb->hcpriv_mempool)
> +		memset(urb->hcpriv_mempool, 0, size);
> +	else
> +		urb->hcpriv_mempool_size = 0;
> +	return urb->hcpriv_mempool;
> +}
> +EXPORT_SYMBOL_GPL(urb_hcpriv_mempool_zalloc);
> +
> +/**
> + * urb_free_hcpriv - free hcpriv data if necessary
> + * @urb: pointer to URB being used
> + *
> + * If mempool is activated, private data's lifecycle
> + * is managed by urb object.
> + */
> +void urb_free_hcpriv(struct urb *urb)
> +{
> +	if (!urb->hcpriv_mempool_activated) {
> +		kfree(urb->hcpriv);
> +		urb->hcpriv = NULL;

You seem to set this to NULL for no reason, AND check for
hcpriv_mempool_activated.  Only one is going to be needed, you don't
need to have both, right?  Why not just rely on hcdpriv being set?

And are you sure that the hcd can actually use a kmalloced "mempool"?  I
don't understand why xhci can't just do this in its driver instead of
this being required in the usb core and adding extra logic and size to
every urb in the system.

thanks,

greg k-h