| Message ID | 20250517083819.6127-1-00107082@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] USB: core: add a memory pool to urb caching host-controller private data | expand |
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
At 2025-05-21 18:32:09, "Greg KH" <gregkh@linuxfoundation.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@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. > 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@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? 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. Using URB as a mempool slot holder would be a very simple approach. The URB objects are already well managed: based on my memory profiling, the alive urb objects and the rate of creating new urb objects are both at small scale. Reusing urb lifecycle management would save lots of troubles, I image.... Also, I would image other hcds could use similar simple changes to cache its private data when they get hold on a URB object. Thanks David > >thanks, > >greg k-h
On 17.5.2025 11.38, 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. > --- > 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. Won't this still allocate a lot of unnecessary memory for the roothub urbs? i.e. the ones queued with rh_urb_enqueue(hcd, urb). The host drivers don't use the urb->hcpriv of those URBs. This would be the roothub status URB, and every hub request sent during device enumeration for devices connected to the roothub. Is this whole URB hcpriv mempool addition an actual issue that needs fixing? If yes then I still think it's better to do it in the host driver like I described earlier. I don't think it will be too complex Thanks -Mathias
On Wed, May 21, 2025 at 07:25:12PM +0800, David Wang wrote: > At 2025-05-21 18:32:09, "Greg KH" <gregkh@linuxfoundation.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@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. > > > > > 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@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? > > 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. > Using URB as a mempool slot holder would be a very simple approach. The URB objects are already well managed: > based on my memory profiling, the alive urb objects and the rate of creating new urb objects are both at small scale. > Reusing urb lifecycle management would save lots of troubles, I image.... > > Also, I would image other hcds could use similar simple changes to cache its private data when they get hold on a URB object. There is already a hcd-specific pointer in the urb, why can't they just use that? Also, while I know you saw less allocation/freeing happening, was that actually measurable in a real way? Without that, the added complexity feels wrong (i.e. you are optimizing for something that is not really needed.) thanks, greg k-h
On Wed, May 21, 2025 at 07:25:12PM +0800, David Wang wrote: > At 2025-05-21 18:32:09, "Greg KH" <gregkh@linuxfoundation.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@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. > > > > > 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@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? > > 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. thanks, greg k-h
At 2025-05-21 20:28:17, "Mathias Nyman" <mathias.nyman@intel.com> wrote: >On 17.5.2025 11.38, 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. >> --- >> 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. > >Won't this still allocate a lot of unnecessary memory for the roothub urbs? >i.e. the ones queued with rh_urb_enqueue(hcd, urb). >The host drivers don't use the urb->hcpriv of those URBs. > The mempool slot is alloced on demand when hcd request private data with its urb. If a urb is ever used by hcd and the hcd requests private data with it, a memory would be alloced and this memory will not be released until the urb is destroyed. If those URB keeps being reused, than the mempool slot within it can be reused by hcds which get hold on it. And yes, it a URB is used only once or used very unfrequently, and hcd needs private data with it, the mempool slot would be alloced with it and would be a very useless mempool slot, wasting memory. >This would be the roothub status URB, and every hub request sent >during device enumeration for devices connected to the roothub. > >Is this whole URB hcpriv mempool addition an actual issue that needs fixing? NO, not an issue, just meant to avoid memory allocations, lots of allocation, to theoretically improve things.. > >If yes then I still think it's better to do it in the host driver like I >described earlier. I don't think it will be too complex > >Thanks >-Mathias > >
On 21.5.2025 16.23, David Wang wrote: >> Won't this still allocate a lot of unnecessary memory for the roothub urbs? >> i.e. the ones queued with rh_urb_enqueue(hcd, urb). >> The host drivers don't use the urb->hcpriv of those URBs. >> > The mempool slot is alloced on demand when hcd request private data with its urb. > If a urb is ever used by hcd and the hcd requests private data with it, a memory would be alloced > and this memory will not be released until the urb is destroyed. Ok, thanks for the clarification. Roothubs URBs should be fine then. Thanks Mathias
At 2025-05-21 20:58:18, "Greg KH" <gregkh@linuxfoundation.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@linuxfoundation.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@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. >> >> > >> >> 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@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? >> >> 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. >> Using URB as a mempool slot holder would be a very simple approach. The URB objects are already well managed: >> based on my memory profiling, the alive urb objects and the rate of creating new urb objects are both at small scale. >> Reusing urb lifecycle management would save lots of troubles, I image.... >> >> Also, I would image other hcds could use similar simple changes to cache its private data when they get hold on a URB object. > >There is already a hcd-specific pointer in the urb, why can't they just >use that? All hcds would need changes, and I have only xhci to verify. Meant to make a small step first, without breaking existing hcds. > >Also, while I know you saw less allocation/freeing happening, was that >actually measurable in a real way? Without that, the added complexity By "measurable in a real way", hope you are not meaning measurable from end user's point of view I have not find a solid proof, yet. (When system is under memory pressure, everything is slow. I feel strongly there would be a point in middle where extra allocation would cost, but failed to pinpoint it yet.) I am using memory profiling[1] to watch this, with a accumulative counter patch[2], whenever a memory is alloced, a counter would be incremented by 1, by calculating delta(counter)/delta(time), I can measure the allocation rate for most call site. [1] https://docs.kernel.org/mm/allocation-profiling.html [2] https://lore.kernel.org/lkml/20240617153250.9079-1-00107082@163.com/ >feels wrong (i.e. you are optimizing for something that is not really >needed.) > >thanks, > >greg k-h
At 2025-05-21 20:59:02, "Greg KH" <gregkh@linuxfoundation.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@linuxfoundation.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@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. >> >> > >> >> 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@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? >> >> 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
At 2025-05-21 22:00:14, "David Wang" <00107082@163.com> wrote: > >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 hcds found using kmem_cache for hc private data: ./drivers/usb/dwc2/hcd.c: hsotg->desc_gen_cache = kmem_cache_create("dwc2-gen-desc", ./drivers/usb/dwc2/hcd.c: hsotg->desc_hsisoc_cache = kmem_cache_create("dwc2-hsisoc-desc", ./drivers/usb/dwc2/hcd.c: hsotg->unaligned_cache = kmem_cache_create("dwc2-unaligned-dma", ./drivers/usb/isp1760/isp1760-hcd.c: urb_listitem_cachep = kmem_cache_create("isp1760_urb_listitem", ./drivers/usb/isp1760/isp1760-hcd.c: qtd_cachep = kmem_cache_create("isp1760_qtd", ./drivers/usb/isp1760/isp1760-hcd.c: qh_cachep = kmem_cache_create("isp1760_qh", sizeof(struct isp1760_qh), ./drivers/usb/host/uhci-hcd.c: uhci_up_cachep = kmem_cache_create("uhci_urb_priv", ./drivers/usb/host/xen-hcd.c: xenhcd_urbp_cachep = kmem_cache_create("xenhcd_urb_priv", I think those hcds have already considered the high frequency of memory allocation for private data as an issue and address it via a const sized memory pool.. xhci could not follow this pattern since it needs a variable length memory. But is this a good pattern to follow? A single slot of memory in URB can handle all those request. Thanks David
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; + } +} +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
--- 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. --- 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(+)