Message-ID: <5d02ca20.a42f.196f3251196.Coremail.00107082@163.com>
Date: Wed, 21 May 2025 22:00:14 +0800 (CST)
From: "David Wang" <00107082@....com>
To: "Greg KH" <gregkh@...uxfoundation.org>
Cc: mathias.nyman@...el.com, oneukum@...e.com, stern@...land.harvard.edu,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] USB: core: add a memory pool to urb caching
 host-controller private data


At 2025-05-21 20:59:02, "Greg KH" <gregkh@...uxfoundation.org> wrote:
>On Wed, May 21, 2025 at 07:25:12PM +0800, David Wang wrote:
>> At 2025-05-21 18:32:09, "Greg KH" <gregkh@...uxfoundation.org> wrote:
>> >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@...e.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.
>> 
>> >
>> 
>> Roger that~
>> 
>> 
>> >> ---
>> >> 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@....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?
>> 
>> I needs to distinguish two situations;
>> 1.  the memory pool is used, then the urb_free_hcpriv should do nothing
>> 2.  the memory was alloced by hcd,  then the memory should be kfreed
>> 
>> Using hcpriv_mempool_activated does look confusing...
>> what about following changes:
>> 
>> +	if (urb->hcpriv != urb->hcpriv_mempool) {
>> +		kfree(urb->hcpriv);
>> +		urb->hcpriv = NULL;
>> +	}
>> 
>> >
>> >And are you sure that the hcd can actually use a kmalloced "mempool"?  I
>> 
>> The patch for xhci is here:  https://lore.kernel.org/lkml/20250517083750.6097-1-00107082@163.com/
>> xhci was kzallocing memory for its private data, and when using USB webcam/mic, I can observe 1k+/s kzallocs
>> And with this patch, during my obs session(with USB webcam/mic), no memory allocation
>> observed for usb sub system;
>> 
>> >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.
>> 
>> Yes, it is possible to make a mempool in hcds. But the lifecycle management would not be an easy one,
>> basically a "mempool" would need to be build up from zero-ground, lots of details need to be addressed,
>> e.g. when should resize the mempool when mempool is too big.
>
>That's up to the HCD to manage, IF they need this.  Otherwise this is a
>burden on all other systems with the increased memory size for no needed
>reason.

Agree that other hcds may not need private data allocation at all, this changes would waste them three more fields they would never
use in URB; but if a hcd needs private data, they can use those field without manage it by their own. 
(I think most hcds need private data)

When looking in codes, I notice xen-hcd have already implemented a mempool, for example:
drivers/usb/host/xen-hcd.c
1323 static int xenhcd_urb_enqueue(struct usb_hcd *hcd, struct urb *urb
...
1334         urbp = kmem_cache_zalloc(xenhcd_urbp_cachep, mem_flags);
1335         if (!urbp)
1336                 return -ENOMEM;
1337 
1338         spin_lock_irqsave(&info->lock, flags);
1339 
1340         urbp->urb = urb;
1341         urb->hcpriv = urbp;
1342         urbp->req_id = ~0;

But most others still use kzalloc.   
Between kmem_cache and a mem slot in URB, I want say the mem slot in URB is managed more efficiently/balanced,
and would it be  healthy to add kmem_cache in each hcds as they see the needs, we would have separated kmem_cache
everywhere.


Thanks
David

Powered by blists - more mailing lists