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

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

[David Wang]

When using USB devices, there are lots of memory allocations for
host controller's private data. For example, on a system with xhci,
using a webcam and a mic would needs ~250 and ~1k kzallocs per
second, respectively.

Some HCDs use kmem_cache to handle the high frequency of memory
allocation during device usages. There are several drawbacks:
1. The lifespan of kmem_cache entry has weak correlation with
device usages: when devices are in idle, those memory is still
kept in slab until system decide to reclaim it, if reclaimable.
2. kmem_cache is only used for fix-sized memory allocation, if a
HCD needs private data of m different sizes, m different kmem_cache
would be needed, and the overhead of kmem_cache is multiplied by m.
When m is large, as some HCD needs variable length private data,
kmem_cache approach is inflexible to use.
3. A system may have multiple HCDs in use, then the overhead of
kmem_cache would be multiplied again by the number of HCDs.

URB objects have long lifespans, an urb can be reused between
submit loops while the devices are being used. And with following
premise:
1. Each URB cannot be used by more than one HCDs at the same time.
2. The lifespan of HC private data is contained in its URB object's
lifespan.

high frequent allocations for HCD private data can be avoided if
urb take over the ownership of memory, the memory then shares
the longer lifespan with urb objects.

The memory pool in URB works this way:
1. A URB object holds only one slot of memory pool.
2. When created, memory pool is NULL. If no private data requested,
no memory pool alloced for the URB.
3. The memory pool slot will grow when larger size is requested.
4. Memory pool slot will be released only when URB is destroyed.

Normally the hcpriv pointer in URB structure is the same as memory
pool slot, but considering some drivers may still have its own
management of urb->hcpriv, a hcpriv_mempool pointer is added, to make
sure no impact on existing drivers.

A hcpriv_mempool_size field is needed for making sure that the mempool
slot is big enough, if not, more memory would be requested from system.

Existing drivers may have it own allocation of URB object, not via
usb_alloc_urb(), this may cause memory leak if URB has a active mempool
slot but it is not released via usb_free_urb(); A hcpriv_mempool_activated
flag is added to address this issue, it is set only in usb_alloc_urb().
When requesting a hcpriv memory to a URB which has hcpriv_mempool_activated
not set, the memory will be requested from system directly.

One of the drawbacks is URB object now have 3 more fields even if the
object would never be requested for HCD private data.

From an end-user's perspective, the performance difference with this change
is insignificant when system is under no memory pressure, and when under
heavy memory pressure. When system is under heavy memory pressure,
everything is slow.  There could be a point in-between no memory pressure
and heavy memory pressure where these 1k+/s memory allocations would
dominate the performance, 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..2cf218d4fb73 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 != urb->hcpriv_mempool) {
+		kfree(urb->hcpriv);
+		urb->hcpriv = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(urb_free_hcpriv);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index b46738701f8d..27bc394b8141 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1602,6 +1602,9 @@ struct urb {
 	struct kref kref;		/* reference count of the URB */
 	int unlinked;			/* unlink error code */
 	void *hcpriv;			/* private data for host controller */
+	void *hcpriv_mempool;           /* a single slot of cache for HCD's private data */
+	size_t hcpriv_mempool_size;     /* current size of the memory pool */
+	bool hcpriv_mempool_activated;  /* flag the mempool usage */
 	atomic_t use_count;		/* concurrent submissions counter */
 	atomic_t reject;		/* submissions will fail */
 
@@ -1790,6 +1793,8 @@ extern int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor,
 extern struct urb *usb_get_from_anchor(struct usb_anchor *anchor);
 extern void usb_scuttle_anchored_urbs(struct usb_anchor *anchor);
 extern int usb_anchor_empty(struct usb_anchor *anchor);
+extern void *urb_hcpriv_mempool_zalloc(struct urb *urb, size_t size, gfp_t mem_flags);
+extern void urb_free_hcpriv(struct urb *urb);
 
 #define usb_unblock_urb	usb_unpoison_urb
 
-- 
2.39.2

[PATCH v4 2/2] USB: xhci: use urb hcpriv mempool for private data

[David Wang]

xhci keeps alloc/free private data for each enqueue/dequeue cycles,
when using a USB webcam, allocation rate is ~250/s;
when using a USB mic, allocation rate reaches ~1k/s;
The more usb device in use, the higher allocation rate.

URB objects have longer lifespan than private data, hand over ownership
of private data to urb can save lots of memory allocations over time.
With this change, no extra memory allocation is needed during usages of
USB webcam/mic.

Signed-off-by: David Wang <00107082@163.com>
---
 drivers/usb/host/xhci-mem.c  | 1 +
 drivers/usb/host/xhci-ring.c | 3 +--
 drivers/usb/host/xhci.c      | 8 +++-----
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index d698095fc88d..b19e41cf1c4c 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1745,6 +1745,7 @@ struct xhci_command *xhci_alloc_command_with_ctx(struct xhci_hcd *xhci,
 
 void xhci_urb_free_priv(struct urb_priv *urb_priv)
 {
+	WARN_ONCE(1, "xhci private data should be managed by urb");
 	kfree(urb_priv);
 }
 
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 423bf3649570..8fa3f71fdb29 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -821,7 +821,6 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
 				     struct xhci_td *cur_td, int status)
 {
 	struct urb	*urb		= cur_td->urb;
-	struct urb_priv	*urb_priv	= urb->hcpriv;
 	struct usb_hcd	*hcd		= bus_to_hcd(urb->dev->bus);
 
 	if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
@@ -831,7 +830,7 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
 				usb_amd_quirk_pll_enable();
 		}
 	}
-	xhci_urb_free_priv(urb_priv);
+	urb_free_hcpriv(urb);
 	usb_hcd_unlink_urb_from_ep(hcd, urb);
 	trace_xhci_urb_giveback(urb);
 	usb_hcd_giveback_urb(hcd, urb, status);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 90eb491267b5..071a7680b36e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1552,7 +1552,7 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	else
 		num_tds = 1;
 
-	urb_priv = kzalloc(struct_size(urb_priv, td, num_tds), mem_flags);
+	urb_priv = urb_hcpriv_mempool_zalloc(urb, struct_size(urb_priv, td, num_tds), mem_flags);
 	if (!urb_priv)
 		return -ENOMEM;
 
@@ -1626,8 +1626,7 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 
 	if (ret) {
 free_priv:
-		xhci_urb_free_priv(urb_priv);
-		urb->hcpriv = NULL;
+		urb_free_hcpriv(urb);
 	}
 	spin_unlock_irqrestore(&xhci->lock, flags);
 	return ret;
@@ -1789,8 +1788,7 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	return ret;
 
 err_giveback:
-	if (urb_priv)
-		xhci_urb_free_priv(urb_priv);
+	urb_free_hcpriv(urb);
 	usb_hcd_unlink_urb_from_ep(hcd, urb);
 	spin_unlock_irqrestore(&xhci->lock, flags);
 	usb_hcd_giveback_urb(hcd, urb, -ESHUTDOWN);
-- 
2.39.2

Re: [PATCH v4 2/2] USB: xhci: use urb hcpriv mempool for private data

[Greg KH]

On Thu, May 22, 2025 at 03:10:10PM +0800, David Wang wrote:
> xhci keeps alloc/free private data for each enqueue/dequeue cycles,
> when using a USB webcam, allocation rate is ~250/s;
> when using a USB mic, allocation rate reaches ~1k/s;
> The more usb device in use, the higher allocation rate.
> 
> URB objects have longer lifespan than private data, hand over ownership
> of private data to urb can save lots of memory allocations over time.
> With this change, no extra memory allocation is needed during usages of
> USB webcam/mic.
> 
> Signed-off-by: David Wang <00107082@163.com>
> ---
>  drivers/usb/host/xhci-mem.c  | 1 +
>  drivers/usb/host/xhci-ring.c | 3 +--
>  drivers/usb/host/xhci.c      | 8 +++-----
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index d698095fc88d..b19e41cf1c4c 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1745,6 +1745,7 @@ struct xhci_command *xhci_alloc_command_with_ctx(struct xhci_hcd *xhci,
>  
>  void xhci_urb_free_priv(struct urb_priv *urb_priv)
>  {
> +	WARN_ONCE(1, "xhci private data should be managed by urb");

You just crashed the kernel if this ever gets hit.  As you are saying
this should never be called, why is this function even present anymore?

This makes no sense :(

Again, NEVER add a WARN*() call to the kernel for something that it
should be handling properly on its own.  Otherwise you just lost all the
user's data when the box got rebooted (and if userspace can trigger
this, you just created a new CVE...)

thanks,

greg k-h

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

[Greg KH]

On Thu, May 22, 2025 at 03:09:43PM +0800, David Wang wrote:
> From an end-user's perspective, the performance difference with this change
> is insignificant when system is under no memory pressure, and when under
> heavy memory pressure. When system is under heavy memory pressure,
> everything is slow.  There could be a point in-between no memory pressure
> and heavy memory pressure where these 1k+/s memory allocations would
> dominate the performance, but very hard to pinpoint it.

For this reason alone I can't take this change, sorry.

Also, as others have stated, this could be done in the hcd drivers
themselves if they want to, no need to push this into the usb core.

thanks,

greg k-h

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

[David Wang]

At 2025-05-22 16:33:42, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Thu, May 22, 2025 at 03:09:43PM +0800, David Wang wrote:
>> From an end-user's perspective, the performance difference with this change
>> is insignificant when system is under no memory pressure, and when under
>> heavy memory pressure. When system is under heavy memory pressure,
>> everything is slow.  There could be a point in-between no memory pressure
>> and heavy memory pressure where these 1k+/s memory allocations would
>> dominate the performance, but very hard to pinpoint it.
>
>For this reason alone I can't take this change, sorry.

Still, reasonable to me....
>
>Also, as others have stated, this could be done in the hcd drivers
>themselves if they want to, no need to push this into the usb core.


>
>thanks,
>
>greg k-h


Thanks all you guys for taking time reviewing/discussing this.

David

Re: [PATCH v4 2/2] USB: xhci: use urb hcpriv mempool for private data

[David Wang]

At 2025-05-22 16:32:40, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Thu, May 22, 2025 at 03:10:10PM +0800, David Wang wrote:
>> xhci keeps alloc/free private data for each enqueue/dequeue cycles,
>> when using a USB webcam, allocation rate is ~250/s;
>> when using a USB mic, allocation rate reaches ~1k/s;
>> The more usb device in use, the higher allocation rate.
>> 
>> URB objects have longer lifespan than private data, hand over ownership
>> of private data to urb can save lots of memory allocations over time.
>> With this change, no extra memory allocation is needed during usages of
>> USB webcam/mic.
>> 
>> Signed-off-by: David Wang <00107082@163.com>
>> ---
>>  drivers/usb/host/xhci-mem.c  | 1 +
>>  drivers/usb/host/xhci-ring.c | 3 +--
>>  drivers/usb/host/xhci.c      | 8 +++-----
>>  3 files changed, 5 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index d698095fc88d..b19e41cf1c4c 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -1745,6 +1745,7 @@ struct xhci_command *xhci_alloc_command_with_ctx(struct xhci_hcd *xhci,
>>  
>>  void xhci_urb_free_priv(struct urb_priv *urb_priv)
>>  {
>> +	WARN_ONCE(1, "xhci private data should be managed by urb");
>
>You just crashed the kernel if this ever gets hit.  As you are saying
>this should never be called, why is this function even present anymore?
>
>This makes no sense :(

I meant to warn further changes to xhci:  better not manage  private data .
I don't think it would crash,   xhci_urb_free_priv should not  be paired with 
urb_hcpriv_mempool_zalloc.  (But nothing prevent it though, same as nothing 
prevents urb_hcpriv_mempool_zalloc being paired with kfree....)
It would be better to remove the whole function.


>
>Again, NEVER add a WARN*() call to the kernel for something that it
>should be handling properly on its own.  Otherwise you just lost all the
>user's data when the box got rebooted (and if userspace can trigger
>this, you just created a new CVE...)

Copy that~!

Thanks~
David
>
>thanks,
>
>greg k-h