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

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, May 14, 2025 at 02:44:55PM +0800, David Wang wrote:
> Hi, 
> 
> Update memory footprints after hours of USB devices usage
> on my system:
> (I have webcam/mic/keyboard/mouse/harddisk connected via USB,
> a full picture of memory footprints is attached below)
> +----------------------+----------------+-------------------------------------------+-----------------------+
> | active memory(bytes) | active objects |               alloc location              | total objects created |
> +----------------------+----------------+-------------------------------------------+-----------------------+
> |        22912         |       24       | core/urb.c:1054:urb_hcpriv_mempool_zalloc |         10523         |
> |        11776         |       31       |        core/urb.c:76:usb_alloc_urb        |         11027         |
> +----------------------+----------------+-------------------------------------------+-----------------------+
> 
> The count for active URB objects remain at low level,
> its peak is about 12KB when I copied 10G file to my harddisk.
> The memory pool in this patch takes about 22KB, its peak is 23KB.
> The patch meant to reuse memory via a mempool, the memory kept in pool is indeed
> the "tradeoff" when the system is idle. (Well, we are talking about mempool anyway.)
> How balance the tradeoff is depends on how well the mempool is managed.
> This patch takes a easy approach: put faith in URB objects management and put
> a single slot of mempool in URB on demands. And the changes, by counting lines
> in this patch, are very simple.
> Base on the profiling, the number of active URB objects are kept at a very low scale,
> only several could have a very long lifecycle.
> I think URB is a good candidate for caching those memory needed for private data.
> But I could be very wrong, due simply to the lack of knowledge.
> 
> And before, without the patch, a 10 minutes webcam usage and copying 10G file to harddisk
> would yield high rate of memory allocation for priviate data in xhci_urb_enqueue:
> +----------------------+----------------+-----------------------------------+-----------------------+
> | active memory(bytes) | active objects |           alloc location          | total objects created |
> +----------------------+----------------+-----------------------------------+-----------------------+
> |        22784         |       23       | host/xhci.c:1555:xhci_urb_enqueue |         894281 << grow|ing very quick
> |        10880         |       31       |    core/urb.c:75:usb_alloc_urb    |          4028         |
> +----------------------+----------------+-----------------------------------+-----------------------+
> I observe a highest allocation rate of 1.5K/s in xhci_urb_enqueue
> when I was copying 10G file, and had my webcam opened at the same time.
> 
> And again, to be honest, I did not observe any observable performance improvement from
> an enduser's point of view with this patch. The only significant improvement is memory footprint
> _numbers_.
> I guess memory allocation is indeed "_really damn fast_", but I still have the mindset of
> "the less allocation the better".

No, this isn't necessarily true at all.  Allocations are fast, and if we
free/allocate things quickly, it's even faster.  USB is limited by the
hardware throughput, which is _very_ slow compared to memory accesses of
the allocator.

So unless you can show that we are using less CPU time, or something
else "real" that is measurable in a real way in userspace, that would
justify the extra complexity, it's going to be hard to get me to agree
that this is something that needs to be addressed at all.

Also, I'm totally confused as to what the "latest" version of this
patchset is...

thanks,

greg k-h