Hello,
On Friday, June 10, 2011 6:22 PM Arnd Bergmann wrote:
On Friday 10 June 2011, Marek Szyprowski wrote:
The Contiguous Memory Allocator is a set of functions that lets one initialise a region of memory which then can be used to perform allocations of contiguous memory chunks from.
CMA allows for creation of separate contexts. Kernel is allowed to allocate movable pages within CMA's managed memory so that it can be used for page cache when CMA devices do not use it. On cm_alloc() request such pages are migrated out of CMA area to free required contiguous block.
Hi Marek,
I'm generally happy with the patches 1 through 7, i.e the heavy lifting to make contiguous allocations work. Thank you very much for keeping up the work and submitting these in a good shape.
I do think that we need to discuss the driver-visible API a bit more. My feeling is that this is rather un-Linux-like and it needs to be simplified some more. Of course, I don't mind being overruled by the memory management experts here, or if you can argue that it's really the right way to do it.
Thanks for your comments!
- Driver usage
- CMA should not be used directly by the device drivers. It should
- be considered as helper framework for dma-mapping subsystm and
- respective (platform)bus drivers.
- The CMA client needs to have a pointer to a CMA context
- represented by a struct cma (which is an opaque data type).
- Once such pointer is obtained, a caller may allocate contiguous
- memory chunk using the following function:
cm_alloc()
- This function returns a pointer to the first struct page which
- represent a contiguous memory chunk. This pointer
- may be used with the following function:
cm_free() -- frees allocated contiguous memoryPlease explain why you want a new top-level API here. I think it would be much nicer if a device driver could simply call alloc_pages(..., GFP_CMA) or similar, where all the complexity in here gets hidden behind a conditional deep inside of the page allocator.
Evidently, the two functions you describe here have an extra argument for the context. Can you elaborate why that is really needed? What is the specific requirement to have multiple such contexts in one system and what is the worst-case effect that you would get when the interface is simplified to have only one for all devices?
Alternatively, would it be possible to provide the cm_alloc/cm_free functions only as backing to the dma mapping API and not export them as a generic device driver interface?
cm_alloc/free are definitely not meant to be called from device drivers. They should be only considered as a backend for dma-mapping.
'Raw' contiguous memory block doesn't really make sense for the device drivers. What the drivers require is a contiguous memory block that is somehow mapped into respective bus address space, so dma-mapping framework is the right interface.
alloc_pages(..., GFP_CMA) looks nice but in fact it is really impractical. The driver will need to map such buffer to dma context anyway, so imho dma_alloc_attributed() will give the drivers much more flexibility. In terms of dma-mapping the context argument isn't anything odd.
If possible I would like to make cma something similar to declare_dma_coherent()&friends, so the board/platform/bus startup code will just call declare_dma_contiguous() to enable support for cma for particular devices.
- Platform/machine integration
- CMA context must be created on platform or machine initialisation
- and passed to respective subsystem that will be a client for CMA.
- The latter may be done by a global variable or some filed in
- struct device. For the former CMA provides the following
functions:
cma_init_migratetype()
cma_reserve()
cma_create()
- The first one initialises a portion of reserved memory so that it
- can be used with CMA. The second first tries to reserve memory
- (using memblock) and then initialise it.
- The cma_reserve() function must be called when memblock is still
- operational and reserving memory with it is still possible. On
- ARM platform the "reserve" machine callback is a perfect place to
- call it.
- The last function creates a CMA context on a range of previously
- initialised memory addresses. Because it uses kmalloc() it needs
- to be called after SLAB is initialised.
- */
This interface looks flawed to me for multiple reasons:
- It requires you to call three distinct functions in order to do one thing, and they all take the same arguments (more or less). Why not have one function call at the latest possible point where you can still change the memory attributes, and have everything else happen automatically?
Initialization part will be definitely simplified, I must confess that I was in hurry to post the patches before the weekend and just forgot to cleanup this part...
- It requires you to pass the exact location of the area. I can see why you want that on certain machines that require DMA areas to be spread across multiple memory buses, but IMHO it's not appropriate for a generic API.
IMHO we can also use some NULL context to indicate some global, system wide CMA area and again -> in terms of dma-mapping api having a context isn't anything uncommon.
- It requires you to hardcode the size in a machine specific source file. This probably seems to be a natural thing to do when you have worked a lot on the ARM architecture, but it's really not. We really want to get rid of board files and replace them with generic probing based on the device tree, and the size of the area is more policy than a property of the hardware that can be accurately described in the device tree or a board file.
The problem is the fact that right now, we still have board files and we have to live with them for a while (with all advantages and disadvantages). I hope that you won't require me to rewrite the whole support for all ARM platforms to get rid of board files to get CMA merged ;)
I see no problem defining CMA areas in device tree, as this is something really specific to particular board configuration.
I'm sure that we can find a solution for all of these problems, it just takes a few good ideas. Especially for the last point, I don't have a better suggestion yet, but hopefully someone else can come up with one.
I hope we will manage to get agreement :)
Best regards