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

["David Wang" <00107082@163.com>]

At 2025-05-14 15:29:42, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>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.

Thanks for feedbacks~! 
That's very reasonable to me,  and I have been pondering on how
to profile a USB performance, but still no clue.

 I will keep thinking about it, hopefully this 1k+/s allocation would show up somewhere, or 
conclude that it really has no significant impact at all.


Thanks
David

>
>Also, I'm totally confused as to what the "latest" version of this
>patchset is...
>
sorry, I think I mess up the mails when I add "reply-to" header to newer patches

>thanks,
>
>greg k-h