Message-Id: <201106101821.50437.arnd@arndb.de>
Date:	Fri, 10 Jun 2011 18:21:50 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Marek Szyprowski <m.szyprowski@...sung.com>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-media@...r.kernel.org, linux-mm@...ck.org,
	linaro-mm-sig@...ts.linaro.org,
	Michal Nazarewicz <mina86@...a86.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Ankita Garg <ankita@...ibm.com>,
	Daniel Walker <dwalker@...eaurora.org>,
	Johan MOSSBERG <johan.xx.mossberg@...ricsson.com>,
	Mel Gorman <mel@....ul.ie>,
	Jesse Barker <jesse.barker@...aro.org>
Subject: Re: [PATCH 08/10] mm: cma: Contiguous Memory Allocator added

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.

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

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

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

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

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.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists