[v3 0/4] ARM: dma-mapping: IOMMU atomic allocation

[v3 0/4] ARM: dma-mapping: IOMMU atomic allocation

[Hiroshi Doyu]

Hi,

The commit e9da6e9 "ARM: dma-mapping: remove custom consistent dma
region" breaks the compatibility with existing drivers. This causes
the following kernel oops(*1). That driver has called dma_pool_alloc()
to allocate memory from the interrupt context, and it hits
BUG_ON(in_interrpt()) in "get_vm_area_caller()". This patch seris
fixes this problem with making use of the pre-allocate atomic memory
pool which DMA is using in the same way as DMA does now.

Any comment would be really appreciated.

v3:
Provide a different path for IOMMU for more clean code. (Marek)
atomic_pool is backed with struct page *pages[].
v2:
Don't modify attrs(DMA_ATTR_NO_KERNEL_MAPPING) for atomic allocation. (Marek)
Modify vzalloc (KyongHo, Minchan)
v1:
http://lists.linaro.org/pipermail/linaro-mm-sig/2012-August/002398.html

*1:
[    8.321343] ------------[ cut here ]------------
[    8.325971] kernel BUG at kernel/mm/vmalloc.c:1322!
[    8.333615] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
[    8.339436] Modules linked in:
[    8.342496] CPU: 0    Tainted: G        W     (3.4.6-00067-g5d485f7 #67)
[    8.349192] PC is at __get_vm_area_node.isra.29+0x164/0x16c
[    8.354758] LR is at get_vm_area_caller+0x4c/0x54
[    8.359454] pc : [<c011297c>]    lr : [<c011318c>]    psr: 20000193
[    8.359458] sp : c09edca0  ip : c09ec000  fp : ae278000
[    8.370922] r10: f0000000  r9 : c011aa54  r8 : c0a26cb8
[    8.376136] r7 : 00000001  r6 : 000000d0  r5 : 20000008  r4 : c09edca0
[    8.382651] r3 : 00010000  r2 : 20000008  r1 : 00000001  r0 : 00001000
[    8.389166] Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[    8.396549] Control: 10c5387d  Table: ad98c04a  DAC: 00000015
....
[    9.169162] dfa0: 412fc099 c09ec000 00000000 c000fdd8 c06df1e4 c0a1b080 00000000 00000000
[    9.177329] dfc0: c0a235cc 8000406a 00000000 c0986818 ffffffff ffffffff c0986404 00000000
[    9.185497] dfe0: 00000000 c09bb070 10c5387d c0a19c58 c09bb064 80008044 00000000 00000000
[    9.193673] [<c011297c>] (__get_vm_area_node.isra.29+0x164/0x16c) from [<c011318c>] (get_vm_area_caller+0x4c/0x54)
[    9.204022] [<c011318c>] (get_vm_area_caller+0x4c/0x54) from [<c001aed8>] (__iommu_alloc_remap.isra.14+0x2c/0xfc)
[    9.214276] [<c001aed8>] (__iommu_alloc_remap.isra.14+0x2c/0xfc) from [<c001b06c>] (arm_iommu_alloc_attrs+0xc4/0xf8)
[    9.224795] [<c001b06c>] (arm_iommu_alloc_attrs+0xc4/0xf8) from [<c011aa54>] (pool_alloc_page.constprop.5+0x6c/0xf8)
[    9.235309] [<c011aa54>] (pool_alloc_page.constprop.5+0x6c/0xf8) from [<c011ab60>] (dma_pool_alloc+0x80/0x170)
[    9.245304] [<c011ab60>] (dma_pool_alloc+0x80/0x170) from [<c03cbbcc>] (tegra_build_dtd+0x48/0x14c)
[    9.254344] [<c03cbbcc>] (tegra_build_dtd+0x48/0x14c) from [<c03cbd4c>] (tegra_req_to_dtd+0x7c/0xa8)
[    9.263467] [<c03cbd4c>] (tegra_req_to_dtd+0x7c/0xa8) from [<c03cc140>] (tegra_ep_queue+0x154/0x33c)
[    9.272592] [<c03cc140>] (tegra_ep_queue+0x154/0x33c) from [<c03dd5b4>] (composite_setup+0x364/0x6d4)
[    9.281804] [<c03dd5b4>] (composite_setup+0x364/0x6d4) from [<c03dd9dc>] (android_setup+0xb8/0x14c)
[    9.290843] [<c03dd9dc>] (android_setup+0xb8/0x14c) from [<c03cd144>] (setup_received_irq+0xbc/0x270)
[    9.300053] [<c03cd144>] (setup_received_irq+0xbc/0x270) from [<c03cda64>] (tegra_udc_irq+0x2ac/0x2c4)
[    9.309353] [<c03cda64>] (tegra_udc_irq+0x2ac/0x2c4) from [<c00b5708>] (handle_irq_event_percpu+0x78/0x2e0)
[    9.319087] [<c00b5708>] (handle_irq_event_percpu+0x78/0x2e0) from [<c00b59b4>] (handle_irq_event+0x44/0x64)
[    9.328907] [<c00b59b4>] (handle_irq_event+0x44/0x64) from [<c00b8688>] (handle_fasteoi_irq+0xc4/0x16c)
[    9.338294] [<c00b8688>] (handle_fasteoi_irq+0xc4/0x16c) from [<c00b4f14>] (generic_handle_irq+0x34/0x48)
[    9.347858] [<c00b4f14>] (generic_handle_irq+0x34/0x48) from [<c000f6f4>] (handle_IRQ+0x54/0xb4)
[    9.356637] [<c000f6f4>] (handle_IRQ+0x54/0xb4) from [<c00084b0>] (gic_handle_irq+0x2c/0x60)
[    9.365068] [<c00084b0>] (gic_handle_irq+0x2c/0x60) from [<c000e900>] (__irq_svc+0x40/0x70)
[    9.373405] Exception stack(0xc09edf10 to 0xc09edf58)
[    9.378447] df00:                                     00000000 000f4240 00000003 00000000
[    9.386615] df20: 00000000 e55bbc00 ef66f3ca 00000001 00000000 412fc099 c0abb9c8 00000000
[    9.394781] df40: 3b9ac9ff c09edf58 c027a9bc c0042880 20000113 ffffffff
[    9.401396] [<c000e900>] (__irq_svc+0x40/0x70) from [<c0042880>] (tegra_idle_enter_lp3+0x68/0x78)
[    9.410272] [<c0042880>] (tegra_idle_enter_lp3+0x68/0x78) from [<c04701d4>] (cpuidle_idle_call+0xdc/0x3a4)
[    9.419922] [<c04701d4>] (cpuidle_idle_call+0xdc/0x3a4) from [<c000fdd8>] (cpu_idle+0xd8/0x134)
[    9.428612] [<c000fdd8>] (cpu_idle+0xd8/0x134) from [<c0986818>] (start_kernel+0x27c/0x2cc)
[    9.436952] Code: e1a00004 e3a04000 eb002265 eaffffe0 (e7f001f2)
[    9.443038] ---[ end trace 1b75b31a2719ed24 ]---
[    9.447645] Kernel panic - not syncing: Fatal exception in interrupt


Hiroshi Doyu (4):
  ARM: dma-mapping: atomic_pool with struct page **pages
  ARM: dma-mapping: Refactor out to introduce __in_atomic_pool
  ARM: dma-mapping: Introduce __atomic_get_pages() for
    __iommu_get_pages()
  ARM: dma-mapping: IOMMU allocates pages from atomic_pool with
    GFP_ATOMIC

 arch/arm/mm/dma-mapping.c |   90 ++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 80 insertions(+), 10 deletions(-)

-- 
1.7.5.4

[v3 1/4] ARM: dma-mapping: atomic_pool with struct page **pages

[Hiroshi Doyu]

struct page **pages is necessary to align with non atomic path in
__iommu_get_pages(). atomic_pool() has the intialized **pages instead
of just *page.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 arch/arm/mm/dma-mapping.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 601da7a..b14ee64 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -296,7 +296,7 @@ struct dma_pool {
 	unsigned long *bitmap;
 	unsigned long nr_pages;
 	void *vaddr;
-	struct page *page;
+	struct page **pages;
 };
 
 static struct dma_pool atomic_pool = {
@@ -335,12 +335,16 @@ static int __init atomic_pool_init(void)
 	unsigned long nr_pages = pool->size >> PAGE_SHIFT;
 	unsigned long *bitmap;
 	struct page *page;
+	struct page **pages;
 	void *ptr;
 	int bitmap_size = BITS_TO_LONGS(nr_pages) * sizeof(long);
+	size_t size = nr_pages * sizeof(struct page *);
 
-	bitmap = kzalloc(bitmap_size, GFP_KERNEL);
+	size += bitmap_size;
+	bitmap = kzalloc(size, GFP_KERNEL);
 	if (!bitmap)
 		goto no_bitmap;
+	pages = (void *)bitmap + bitmap_size;
 
 	if (IS_ENABLED(CONFIG_CMA))
 		ptr = __alloc_from_contiguous(NULL, pool->size, prot, &page);
@@ -348,9 +352,14 @@ static int __init atomic_pool_init(void)
 		ptr = __alloc_remap_buffer(NULL, pool->size, GFP_KERNEL, prot,
 					   &page, NULL);
 	if (ptr) {
+		int i;
+
+		for (i = 0; i < nr_pages; i++)
+			pages[i] = page + i;
+
 		spin_lock_init(&pool->lock);
 		pool->vaddr = ptr;
-		pool->page = page;
+		pool->pages = pages;
 		pool->bitmap = bitmap;
 		pool->nr_pages = nr_pages;
 		pr_info("DMA: preallocated %u KiB pool for atomic coherent allocations\n",
@@ -481,7 +490,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
 	if (pageno < pool->nr_pages) {
 		bitmap_set(pool->bitmap, pageno, count);
 		ptr = pool->vaddr + PAGE_SIZE * pageno;
-		*ret_page = pool->page + pageno;
+		*ret_page = pool->pages[pageno];
 	} else {
 		pr_err_once("ERROR: %u KiB atomic DMA coherent pool is too small!\n"
 			    "Please increase it with coherent_pool= kernel parameter!\n",
-- 
1.7.5.4

[v3 2/4] ARM: dma-mapping: Refactor out to introduce __in_atomic_pool

[Hiroshi Doyu]

Check the given range("start", "size") is included in "atomic_pool" or not.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 arch/arm/mm/dma-mapping.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index b14ee64..508fde1 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -501,19 +501,32 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
 	return ptr;
 }
 
+static bool __in_atomic_pool(void *start, size_t size)
+{
+	struct dma_pool *pool = &atomic_pool;
+	void *end = start + size;
+	void *pool_start = pool->vaddr;
+	void *pool_end = pool->vaddr + pool->size;
+
+	if (start < pool_start || start > pool_end)
+		return false;
+
+	if (end > pool_end) {
+		WARN(1, "freeing wrong coherent size from pool\n");
+		return false;
+	}
+
+	return true;
+}
+
 static int __free_from_pool(void *start, size_t size)
 {
 	struct dma_pool *pool = &atomic_pool;
 	unsigned long pageno, count;
 	unsigned long flags;
 
-	if (start < pool->vaddr || start > pool->vaddr + pool->size)
-		return 0;
-
-	if (start + size > pool->vaddr + pool->size) {
-		WARN(1, "freeing wrong coherent size from pool\n");
+	if (!__in_atomic_pool(start, size))
 		return 0;
-	}
 
 	pageno = (start - pool->vaddr) >> PAGE_SHIFT;
 	count = size >> PAGE_SHIFT;
-- 
1.7.5.4

[v3 3/4] ARM: dma-mapping: Introduce __atomic_get_pages() for __iommu_get_pages()

[Hiroshi Doyu]

Support atomic allocation in __iommu_get_pages().

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 arch/arm/mm/dma-mapping.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 508fde1..58a852b 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -501,6 +501,15 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
 	return ptr;
 }
 
+static struct page **__atomic_get_pages(void *addr)
+{
+	struct dma_pool *pool = &atomic_pool;
+	struct page **pages = pool->pages;
+	int offs = (addr - pool->vaddr) >> PAGE_SHIFT;
+
+	return pages + offs;
+}
+
 static bool __in_atomic_pool(void *start, size_t size)
 {
 	struct dma_pool *pool = &atomic_pool;
@@ -1184,6 +1193,9 @@ static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
 {
 	struct vm_struct *area;
 
+	if (__in_atomic_pool(cpu_addr, PAGE_SIZE))
+		return __atomic_get_pages(cpu_addr);
+
 	if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
 		return cpu_addr;
 
-- 
1.7.5.4

[v3 4/4] ARM: dma-mapping: IOMMU allocates pages from atomic_pool with GFP_ATOMIC

[Hiroshi Doyu]

Make use of the same atomic pool as DMA does, and skip a kernel page
mapping which can involve sleep'able operations at allocating a kernel
page table.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 arch/arm/mm/dma-mapping.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 58a852b..3ce152a 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1205,6 +1205,34 @@ static struct page **__iommu_get_pages(void *cpu_addr, struct dma_attrs *attrs)
 	return NULL;
 }
 
+static void *__iommu_alloc_atomic(struct device *dev, size_t size,
+				  dma_addr_t *handle)
+{
+	struct page *page;
+	void *addr;
+
+	addr = __alloc_from_pool(size, &page);
+	if (!addr)
+		return NULL;
+
+	*handle = __iommu_create_mapping(dev, &page, size);
+	if (*handle == DMA_ERROR_CODE)
+		goto err_mapping;
+
+	return addr;
+
+err_mapping:
+	__free_from_pool(addr, size);
+	return NULL;
+}
+
+static void __iommu_free_atomic(struct device *dev, struct page **pages,
+				dma_addr_t handle, size_t size)
+{
+	__iommu_remove_mapping(dev, handle, size);
+	__free_from_pool(page_address(pages[0]), size);
+}
+
 static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
 	    dma_addr_t *handle, gfp_t gfp, struct dma_attrs *attrs)
 {
@@ -1215,6 +1243,9 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
 	*handle = DMA_ERROR_CODE;
 	size = PAGE_ALIGN(size);
 
+	if (gfp & GFP_ATOMIC)
+		return __iommu_alloc_atomic(dev, size, handle);
+
 	pages = __iommu_alloc_buffer(dev, size, gfp);
 	if (!pages)
 		return NULL;
@@ -1281,6 +1312,11 @@ void arm_iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr,
 		return;
 	}
 
+	if (__in_atomic_pool(cpu_addr, size)) {
+		__iommu_free_atomic(dev, pages, handle, size);
+		return;
+	}
+
 	if (!dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs)) {
 		unmap_kernel_range((unsigned long)cpu_addr, size);
 		vunmap(cpu_addr);
-- 
1.7.5.4

Re: [v3 1/4] ARM: dma-mapping: atomic_pool with struct page **pages

[Konrad Rzeszutek Wilk]

On Fri, Aug 24, 2012 at 11:29:02AM +0300, Hiroshi Doyu wrote:
> struct page **pages is necessary to align with non atomic path in
> __iommu_get_pages(). atomic_pool() has the intialized **pages instead
> of just *page.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
>  arch/arm/mm/dma-mapping.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 601da7a..b14ee64 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -296,7 +296,7 @@ struct dma_pool {
>  	unsigned long *bitmap;
>  	unsigned long nr_pages;
>  	void *vaddr;
> -	struct page *page;
> +	struct page **pages;
>  };
>  
>  static struct dma_pool atomic_pool = {
> @@ -335,12 +335,16 @@ static int __init atomic_pool_init(void)
>  	unsigned long nr_pages = pool->size >> PAGE_SHIFT;
>  	unsigned long *bitmap;
>  	struct page *page;
> +	struct page **pages;
>  	void *ptr;
>  	int bitmap_size = BITS_TO_LONGS(nr_pages) * sizeof(long);
> +	size_t size = nr_pages * sizeof(struct page *);
>  
> -	bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> +	size += bitmap_size;
> +	bitmap = kzalloc(size, GFP_KERNEL);
>  	if (!bitmap)
>  		goto no_bitmap;
> +	pages = (void *)bitmap + bitmap_size;

So you stuck a bitmap field in front of the array then?
Why not just define a structure where this is clearly defined
instead of doing the casting.

>  
>  	if (IS_ENABLED(CONFIG_CMA))
>  		ptr = __alloc_from_contiguous(NULL, pool->size, prot, &page);
> @@ -348,9 +352,14 @@ static int __init atomic_pool_init(void)
>  		ptr = __alloc_remap_buffer(NULL, pool->size, GFP_KERNEL, prot,
>  					   &page, NULL);
>  	if (ptr) {
> +		int i;
> +
> +		for (i = 0; i < nr_pages; i++)
> +			pages[i] = page + i;
> +
>  		spin_lock_init(&pool->lock);
>  		pool->vaddr = ptr;
> -		pool->page = page;
> +		pool->pages = pages;
>  		pool->bitmap = bitmap;
>  		pool->nr_pages = nr_pages;
>  		pr_info("DMA: preallocated %u KiB pool for atomic coherent allocations\n",
> @@ -481,7 +490,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
>  	if (pageno < pool->nr_pages) {
>  		bitmap_set(pool->bitmap, pageno, count);
>  		ptr = pool->vaddr + PAGE_SIZE * pageno;
> -		*ret_page = pool->page + pageno;
> +		*ret_page = pool->pages[pageno];
>  	} else {
>  		pr_err_once("ERROR: %u KiB atomic DMA coherent pool is too small!\n"
>  			    "Please increase it with coherent_pool= kernel parameter!\n",
> -- 
> 1.7.5.4
> 

Re: [v3 2/4] ARM: dma-mapping: Refactor out to introduce __in_atomic_pool

[Konrad Rzeszutek Wilk]

On Fri, Aug 24, 2012 at 11:29:03AM +0300, Hiroshi Doyu wrote:
> Check the given range("start", "size") is included in "atomic_pool" or not.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
>  arch/arm/mm/dma-mapping.c |   25 +++++++++++++++++++------
>  1 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index b14ee64..508fde1 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -501,19 +501,32 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
>  	return ptr;
>  }
>  
> +static bool __in_atomic_pool(void *start, size_t size)
> +{
> +	struct dma_pool *pool = &atomic_pool;
> +	void *end = start + size;
> +	void *pool_start = pool->vaddr;
> +	void *pool_end = pool->vaddr + pool->size;
> +
> +	if (start < pool_start || start > pool_end)
> +		return false;
> +
> +	if (end > pool_end) {
> +		WARN(1, "freeing wrong coherent size from pool\n");

That does not tell what size or from what pool. Perhaps you should
include some details, such as the 'size' value, the pool used, the
range of the pool, etc. Something that will help _you_in the field
be able to narrow down what might be wrong.

> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  static int __free_from_pool(void *start, size_t size)
>  {
>  	struct dma_pool *pool = &atomic_pool;
>  	unsigned long pageno, count;
>  	unsigned long flags;
>  
> -	if (start < pool->vaddr || start > pool->vaddr + pool->size)
> -		return 0;
> -
> -	if (start + size > pool->vaddr + pool->size) {
> -		WARN(1, "freeing wrong coherent size from pool\n");
> +	if (!__in_atomic_pool(start, size))
>  		return 0;
> -	}
>  
>  	pageno = (start - pool->vaddr) >> PAGE_SHIFT;
>  	count = size >> PAGE_SHIFT;
> -- 
> 1.7.5.4
> 

Re: [v3 2/4] ARM: dma-mapping: Refactor out to introduce __in_atomic_pool

[Hiroshi Doyu]

Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote @ Fri, 24 Aug 2012 13:14:55 +0200:

> On Fri, Aug 24, 2012 at 11:29:03AM +0300, Hiroshi Doyu wrote:
> > Check the given range("start", "size") is included in "atomic_pool" or not.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> >  arch/arm/mm/dma-mapping.c |   25 +++++++++++++++++++------
> >  1 files changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index b14ee64..508fde1 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -501,19 +501,32 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
> >  	return ptr;
> >  }
> >  
> > +static bool __in_atomic_pool(void *start, size_t size)
> > +{
> > +	struct dma_pool *pool = &atomic_pool;
> > +	void *end = start + size;
> > +	void *pool_start = pool->vaddr;
> > +	void *pool_end = pool->vaddr + pool->size;
> > +
> > +	if (start < pool_start || start > pool_end)
> > +		return false;
> > +
> > +	if (end > pool_end) {
> > +		WARN(1, "freeing wrong coherent size from pool\n");
> 
> That does not tell what size or from what pool. Perhaps you should
> include some details, such as the 'size' value, the pool used, the
> range of the pool, etc. Something that will help _you_in the field
> be able to narrow down what might be wrong.

True. I'll.

Re: [v3 1/4] ARM: dma-mapping: atomic_pool with struct page **pages

[Hiroshi Doyu]

Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote @ Fri, 24 Aug 2012 13:13:23 +0200:

> On Fri, Aug 24, 2012 at 11:29:02AM +0300, Hiroshi Doyu wrote:
> > struct page **pages is necessary to align with non atomic path in
> > __iommu_get_pages(). atomic_pool() has the intialized **pages instead
> > of just *page.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> >  arch/arm/mm/dma-mapping.c |   17 +++++++++++++----
> >  1 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index 601da7a..b14ee64 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -296,7 +296,7 @@ struct dma_pool {
> >  	unsigned long *bitmap;
> >  	unsigned long nr_pages;
> >  	void *vaddr;
> > -	struct page *page;
> > +	struct page **pages;
> >  };
> >  
> >  static struct dma_pool atomic_pool = {
> > @@ -335,12 +335,16 @@ static int __init atomic_pool_init(void)
> >  	unsigned long nr_pages = pool->size >> PAGE_SHIFT;
> >  	unsigned long *bitmap;
> >  	struct page *page;
> > +	struct page **pages;
> >  	void *ptr;
> >  	int bitmap_size = BITS_TO_LONGS(nr_pages) * sizeof(long);
> > +	size_t size = nr_pages * sizeof(struct page *);
> >  
> > -	bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > +	size += bitmap_size;
> > +	bitmap = kzalloc(size, GFP_KERNEL);
> >  	if (!bitmap)
> >  		goto no_bitmap;
> > +	pages = (void *)bitmap + bitmap_size;
> 
> So you stuck a bitmap field in front of the array then?
> Why not just define a structure where this is clearly defined
> instead of doing the casting.

I just wanted to allocate only once for the members "pool->bitmap" and
"pool->pages" at once. Since the size of a whole bitmap isn't known in
advance, I couldn't find any fixed type for this bitmap, which pointer
can be shifted without casting. IOW, they are variable length.

RE: [v3 1/4] ARM: dma-mapping: atomic_pool with struct page **pages

[Marek Szyprowski]

Hello,

On Friday, August 24, 2012 1:52 PM Hiroshi Doyu wrote:

> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote @ Fri, 24 Aug 2012 13:13:23 +0200:
> 
> > On Fri, Aug 24, 2012 at 11:29:02AM +0300, Hiroshi Doyu wrote:
> > > struct page **pages is necessary to align with non atomic path in
> > > __iommu_get_pages(). atomic_pool() has the intialized **pages instead
> > > of just *page.
> > >
> > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > ---
> > >  arch/arm/mm/dma-mapping.c |   17 +++++++++++++----
> > >  1 files changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > > index 601da7a..b14ee64 100644
> > > --- a/arch/arm/mm/dma-mapping.c
> > > +++ b/arch/arm/mm/dma-mapping.c
> > > @@ -296,7 +296,7 @@ struct dma_pool {
> > >  	unsigned long *bitmap;
> > >  	unsigned long nr_pages;
> > >  	void *vaddr;
> > > -	struct page *page;
> > > +	struct page **pages;
> > >  };
> > >
> > >  static struct dma_pool atomic_pool = {
> > > @@ -335,12 +335,16 @@ static int __init atomic_pool_init(void)
> > >  	unsigned long nr_pages = pool->size >> PAGE_SHIFT;
> > >  	unsigned long *bitmap;
> > >  	struct page *page;
> > > +	struct page **pages;
> > >  	void *ptr;
> > >  	int bitmap_size = BITS_TO_LONGS(nr_pages) * sizeof(long);
> > > +	size_t size = nr_pages * sizeof(struct page *);
> > >
> > > -	bitmap = kzalloc(bitmap_size, GFP_KERNEL);
> > > +	size += bitmap_size;
> > > +	bitmap = kzalloc(size, GFP_KERNEL);
> > >  	if (!bitmap)
> > >  		goto no_bitmap;
> > > +	pages = (void *)bitmap + bitmap_size;
> >
> > So you stuck a bitmap field in front of the array then?
> > Why not just define a structure where this is clearly defined
> > instead of doing the casting.
> 
> I just wanted to allocate only once for the members "pool->bitmap" and
> "pool->pages" at once. Since the size of a whole bitmap isn't known in
> advance, I couldn't find any fixed type for this bitmap, which pointer
> can be shifted without casting. IOW, they are variable length.

IMHO it is better to avoid any non-trivial things in generic arch code. Merging
those 2 allocations doesn't save any significant bit of memory and might confuse
someone. Better just allocate them separately.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center