Hello,

On Friday, June 10, 2011 6:22 PM Arnd Bergmann wrote:

On Friday 10 June 2011, Marek Szyprowski wrote:

quoted

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!
 

quoted

+ * 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 memory

Please 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.

quoted

+ * 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:

quoted

+ *
+ *     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
-- 
Marek Szyprowski
Samsung Poland R&D Center