Message-ID: <ed0c2f75-f97b-4cad-ad35-78361edf142b@rowland.harvard.edu>
Date: Tue, 13 May 2025 14:21:15 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: David Wang <00107082@....com>
Cc: mathias.nyman@...el.com, gregkh@...uxfoundation.org, oneukum@...e.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] USB: core: add a memory pool to urb for
 host-controller private data

On Wed, May 14, 2025 at 12:35:29AM +0800, David Wang wrote:
> 
> At 2025-05-13 23:37:20, "Alan Stern" <stern@...land.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?"

> 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

Powered by blists - more mailing lists