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? 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 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? The nice property about the helper is that it also avoids a circular depency cycle in patchkits. -Andi -- 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
