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 02:21:15, "Alan Stern" <stern@rowland.harvard.edu> wrote:
>On Wed, May 14, 2025 at 12:35:29AM +0800, David Wang wrote:
>> 
>> At 2025-05-13 23:37:20, "Alan Stern" <stern@rowland.harvard.edu> wrote:
>> >> I have to disagree,  your suggestion has no much difference from requesting memory from
>> >> system, locks and memory pool managements, all the same are needed, why bother?
>> >
>> >There are two differences.  First, xhci-hcd already has its own lock 
>> >that it acquires when enqueuing or dequeuing URBs, so no additional 
>> >locks would be needed.  Second, complicated memory pool management isn't 
>> >necessary; the management can be extremely simple.  (For example, 
>> >Mathias suggested just reusing the most recently released memory area 
>> >unless it is too small.)
>> 
>> My patch also just reuse memory,  in a simpler way I would argue....
>
>I didn't say your approach wasn't simple.  I said that my approach has 
>very low overhead, a lot lower than the existing code, whereas you said 
>my approach "has no much difference" from the existing code.
>
>> >> >This idea can become more elaborate if you maintain separate free lists 
>> >> >for different devices, or even for different endpoints, or sort the free 
>> >> >list by the size of the memory areas.  But the basic idea is always the 
>> >> >same: Don't change usbcore (including struct urb), and make getting and 
>> >> >releasing the extra memory areas have extremely low overhead.
>> >> 
>> >> Why implements a device level memory pool would have extremely low overhead?
>> >
>> >Because then getting or releasing memory areas from the pool could be 
>> >carried out just by manipulating a couple of pointers.
>> 
>> A couple of pointers manipulation? and it would be simpler than a reusable buffer in URB?
>>  I doubt that.
>
>I didn't say pointer manipulation was simpler than a reusable buffer.  I 
>said that it was very low overhead, in order to answer your question: 
>"Why implements a device level memory pool would have extremely low 
>overhead?"

Now I feels it  becomes a wording game .....
>
>> There would be lots of details needs to consider,  detail is devil and that why we prefer simpler solution,
>> I just don't understand, you seems imply that my patch is not simple, could you elaborate more on it,
>> or it is just that in my mind, make changes to "usb core" is a big no-no!
>
>Neither one.
>  
>However, you have forgotten about one thing: With your patch, each URB 
>maintains ownership of a memory area even when the URB is not in use.  
>With the existing code, that memory is freed when the URB is not in use, 
>and with my approach the memory is shared among URBs.
>
>In this way, your patch will use more memory than the existing code or 
>my approach.  The question to answer is which is better: Using more 
>memory (your patch) or using more time (the allocations and 
>deallocations in the existing code or my approach)?
>
>> >If I were redesigning Linux's entire USB stack from the beginning, I 
>> >would change it so that URBs would be dedicated to particular host 
>> >controllers and endpoint types -- maybe even to particular endpoints.  
>> >They would contain all the additional memory required for the HCD to use 
>> >them, all pre-allocated, so that dynamic allocation would not be needed 
>> >during normal use.  (The gadget subsystem behaves this way already.)
>> >
>> >Such a drastic change isn't feasible at this point, although what you 
>> >are suggesting is a step in that direction.  In the end it comes down 
>> >to a time/space tradeoff, and it's very difficult to know what the best 
>> >balance is.
>> Drastic change? Do you mean make change to USB core?
>
>No, I meant redesigning the entire USB stack.  It would require changing 
>all the USB drivers to allocate URBs differently from the way they do 
>now.  And changing every HC driver to preallocate the memory it needs to 
>perform transfers.  I think you can agree that would be a pretty drastic 
>change.
>
>>  Because counting by
>> lines of changes in this patch, I feel my patch is quite simple and efficient.
>> I also don't get your time/space tradeoff here,  are you talking about my patch?
>> or you were just talking a solution in your mind....
>
>Both.  See my comment above.
>
>> This patch only needs a pointer and a size in URB, and URB object allocated in a slow pace...
>
>It also needs an extra memory area that is allocated the first time the 
>URB is used, and is not deallocated until the URB is destroyed.  You 
>seem to be ignoring this fact.
>
>Alan Stern

It is not an "extra" memory area,  the memory is needed by HC anyway, the memory pool just cache it.
And about not freeing memory until URB released,  you seems forgot that we are talking 
about "memory pool" .  A URB only used once could be considered a memory pool never used.

If your memory pool approach would not  "waste" memory, I would  rather happy to learn.

I want to mention the purpose of this patch again:  
A lot of "private data" allocation could be avoided if  we use a "mempool" to cache and reuse those memory.
And use URB as the holder is a very simple way to implement this,. 

And to add , base on my memory profiling, URB usage is very efficient. I think it is a very good candidate to hold
private data cache for HCs.


David