Re: [PATCH] [9/20] Add blk_kmalloc/blk_alloc_pages

[Andi Kleen <andi@firstfloor.org>]

On Mon, Mar 17, 2008 at 09:27:11AM +0100, Jens Axboe wrote:
> On Fri, Mar 14 2008, Andi Kleen wrote:
> > On Fri, Mar 14, 2008 at 02:48:14PM +0100, Jens Axboe wrote:
> > > On Thu, Mar 13 2008, James Bottomley wrote:
> > > > \On Fri, 2008-03-07 at 18:54 +0100, Andi Kleen wrote:
> > > > > When a user is doing IO in the kernel and wants to avoid bouncing
> > > > > it is best to just ask the block layer to allocate the memory for it.
> > > > > This patch adds two simple wrappers: blk_kmalloc and blk_alloc_pages
> > > > > and respective free functions to do this.
> > > > > 
> > > > > blk_alloc_pages is a little unusual in that it takes size
> > > > > instead of order arguments -- i did this because I have later
> > > > > patches to convert it over to a new allocator which does not 
> > > > > require power of two for pages.
> > > > 
> > > > I really don't like this ... it's wedging something in the block layer
> > > > that shouldn't be there just to avoid doing it properly in terms of
> > > > allocations on the device dma_mask.
> > > > 
> > > > I also think the kfree takes a length part is asking for trouble because
> > > > it's pretty fragile.
> > > > 
> > > > However, if Jens will ack it, I'll (reluctantly) add it.
> > > 
> > > I agree with you, I don't like it at all (for a variety of reasons).
> > 
> > For what reasons exactly? 
> 
> Mainly the one I list below. Using the interface for allocation is
> actually more involved that just doing it yourself, which is not a sign
> of a good abstraction.

hmm, missed the point sorry. How is using the interface more
involved? It has to get a mask or a pfn from somewhere and 
without the interface that's actually more code to write out.

> > The reason I added the size is that the mask allocator (not in the SCSI
> > specific patchkit, but I posted that to l-k later) needs a size to free
> > because it is based on a similar design as free_pages which always needed
> > size. Also at least for the few users I converted (there are not really
> > that many) the size is either constant anyways or very easy to remember
> > (please take a look at the concrete patches before judging it)  
> > 
> > The other problem is that there is no mask, except for the bounce mask. These
> > old cruft ISA drivers don't have a device (and before anybody asks
> > again -- no i don't plan to rewrite them all to be device based);
> > but they do have correct bounce_pfn which has all the needed information.
> > I don't think it would make sense to add another field for this.
> > 
> > That is why I ended up with the helpers
> > 
> > Of course if you don't want the helpers it could be something like
> > 
> > 	get_pages_mask(GFP_KERNEL, size, blk_q_mask(request_Queue))
> > 
> > written out but why not have simple helpers that allows this a little nicer?  
> 
> I'd greatly prefer that, a helper that converts the dma mask to a
> suitable GFP for the architecture. The benefit is that all allocators
> takes this mask, and it still leaves the driver free to do whatever it

These are not really used by drivers (except libata) 

> wants to allocate memory.

Well it already exists, it is just called blk_q_mask() and 
is added later with the mask allocator block layer code and was used
there internally. I can move that out into SCSI if it's really needed.

Short term it would be a little problematic because it would turn
the patch dependencies around. Currently the early SCSI patchkit
is nicely independent from the mask allocator and it would be 
somewhat messy to add it later. If you really it is important 
I can do it, but if it's only a slight preference or something
it would avoid me quite some work to not do that.

Really to be honest I cannot see all that much difference between 
blk_kmalloc(q, ...) and get_pages_mask(..., blk_q_mask(q)) 

-Andi