On Mon, Mar 17 2008 at 10:53 +0200, Andi Kleen <andi@xxxxxxxxxxxxxx> wrote: > On Mon, Mar 17, 2008 at 09:38:32AM +0100, Jens Axboe wrote: >> 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 > > While the implementation in the first patch does not require tracking > length, the mask allocator version that is only introduced about 15 > patches later or so does. That is why i added the length parameter > in the first patch already, although it is not used yet. > > Sorry for not making that more clear in the changelog > > I actually considered tracking length in the struct page in the > mask allocator for this, but when I looked at the callers I noticed 90+% > of them only used a constant length which does not really > need to get tracked. It is always the same. Then there are two > or so with a variable length, but it is all local to a single > function so it's also not particularly burdensome to track either. > Please take a look at the callers and you'll see the same. > >> doesn't add anything useful, > > Well for once it allows easy conversion to the mask alloactor linux > world. That is the reason'd'etre of the whole patchkit as far as I'm > concerned anyways. > >> it's not a good abstraction/api imho. > > I'm still trying to figure out what exactly you object to. > > Is the only problem the length? Would blk_kmalloc/kfree() be ok > for you if kfree didn't require a length parameter? It would > not be that hard to add one, although frankly I personally > don't see the need. > > Or alternatively I will just remove it if James can get convinced > to back down on his earlier requirement for bounce less scanning. > >>>>> 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. > > There is no gfp anymore for this in the mask allocator world!!!! Only > masks and a slight abstraction for some cases request queues with > bounce pfns. > > -Andi Andi, Jens I would like if we could do a regular allocation and let the block layer bounce it if needed. Please bear in mind that a lot of this code wants to change to use proper BIO and blk_ API as was proposed by Mike Christie. So there is only that one user at scsi_scan where the performance gains are so marginal, comparing the sizes used, to the latency and slowness of the actual IO. On the other hand it is not only the ISA cases right? Even in an x86_64 system with a PCI-32 host, we are going to bounce. Perhaps if there is a convenient API for allocating un-bounced memory for a block device there will be more users for it, like file-systems. The API, I would like that if failing to allocate un--bounced memory would then allocate any memory and bounce later. Boaz -- 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
