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. > In my original patchkit I didn't have blk_kmalloc etc. but just relied > on the block layer bouncing everything for ISA as needed, but James > thought it was important that SCSI LUN scan etc do not bounce for ISA. > That is when I came up with these helpers. > > You think it should go back to always bouncing? That would be fine > for me too. With that they wouldn't be needed. I don't see a reason for not bouncing for scanning - James? > I think the only issue where it might be a problem would be with > sg.c where it might be a performance issue. > > > Since callers need to remember size for free anyway (I've always hated > > such interfaces), they may as well do their own allocations. I don't > > think the block abstraction buys us anything. > > > > > If anything, add a generic allocator helper that you can pass a mask to > > instead. > > 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 wants to allocate memory. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
