[lkml]   [2011]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRE: [PATCH 08/10] mm: cma: Contiguous Memory Allocator added

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

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

 \ /
  Last update: 2011-06-13 11:09    [W:0.133 / U:6.012 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site