On Mon, Mar 17 2008, Andi Kleen wrote: > 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. Because it requires tracking length, for instance. Your interface just doesn't add anything useful, it's not a good abstraction/api imho. > > > 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. Put it in block/ or blkdev.h and call it queue_mask_to_gfp() or something, that would be fine. > 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)) A bit of patch churn is better than getting stuck with blk_free(ptr, size), so I think it's the way to do. I already voiced my opinion on why I don't like the allocator abstraction, so I wont repeat that. -- 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
