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

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

*1:
[    8.321343] ------------[ cut here ]------------
[    8.325971] kernel BUG at /home/hdoyu/mydroid-k340-cardhu/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: Refactor out to introduce __alloc_fill_pages
  ARM: dma-mapping: IOMMU allocates pages from pool with GFP_ATOMIC
  ARM: dma-mapping: Return cpu addr when dma_alloc(GFP_ATOMIC)
  ARM: dma-mapping: dma_{alloc,free}_coherent with empty attrs

 arch/arm/include/asm/dma-mapping.h |   21 ++++++++++--
 arch/arm/mm/dma-mapping.c          |   59 ++++++++++++++++++++++++++++--------
 2 files changed, 63 insertions(+), 17 deletions(-)

-- 
1.7.5.4

[RFC 1/4] ARM: dma-mapping: Refactor out to introduce __alloc_fill_pages

[Hiroshi Doyu]

__alloc_fill_pages() allocates power of 2 page number allocation at
most repeatedly.

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

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 7dc61ed..aec0c06 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -988,19 +988,10 @@ static inline void __free_iova(struct dma_iommu_mapping *mapping,
 	spin_unlock_irqrestore(&mapping->lock, flags);
 }
 
-static struct page **__iommu_alloc_buffer(struct device *dev, size_t size, gfp_t gfp)
+static int __alloc_fill_pages(struct page ***ppages, int count, gfp_t gfp)
 {
-	struct page **pages;
-	int count = size >> PAGE_SHIFT;
-	int array_size = count * sizeof(struct page *);
 	int i = 0;
-
-	if (array_size <= PAGE_SIZE)
-		pages = kzalloc(array_size, gfp);
-	else
-		pages = vzalloc(array_size);
-	if (!pages)
-		return NULL;
+	struct page **pages = *ppages;
 
 	while (count) {
 		int j, order = __fls(count);
@@ -1022,11 +1013,36 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size, gfp_t
 		count -= 1 << order;
 	}
 
-	return pages;
+	return 0;
+
 error:
 	while (i--)
 		if (pages[i])
 			__free_pages(pages[i], 0);
+	return -ENOMEM;
+}
+
+static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
+					  gfp_t gfp)
+{
+	struct page **pages;
+	int count = size >> PAGE_SHIFT;
+	int array_size = count * sizeof(struct page *);
+	int err;
+
+	if (array_size <= PAGE_SIZE)
+		pages = kzalloc(array_size, gfp);
+	else
+		pages = vzalloc(array_size);
+	if (!pages)
+		return NULL;
+
+	err = __alloc_fill_pages(&pages, count, gfp);
+	if (err)
+		goto error
+
+	return pages;
+error:
 	if (array_size <= PAGE_SIZE)
 		kfree(pages);
 	else
-- 
1.7.5.4

[RFC 2/4] ARM: dma-mapping: IOMMU allocates pages from pool with GFP_ATOMIC

[Hiroshi Doyu]

Makes use of the same atomic pool from DMA, and skips kernel page
mapping which can involves sleep'able operation at allocating a kernel
page table.

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

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index aec0c06..9260107 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1028,7 +1028,6 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 	struct page **pages;
 	int count = size >> PAGE_SHIFT;
 	int array_size = count * sizeof(struct page *);
-	int err;
 
 	if (array_size <= PAGE_SIZE)
 		pages = kzalloc(array_size, gfp);
@@ -1037,9 +1036,20 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 	if (!pages)
 		return NULL;
 
-	err = __alloc_fill_pages(&pages, count, gfp);
-	if (err)
-		goto error
+	if (gfp & GFP_ATOMIC) {
+		struct page *page;
+		int i;
+		void *addr = __alloc_from_pool(size, &page);
+		if (!addr)
+			goto err_out;
+
+		for (i = 0; i < count; i++)
+			pages[i] = page + i;
+	} else {
+		int err = __alloc_fill_pages(&pages, count, gfp);
+		if (err)
+			goto error;
+	}
 
 	return pages;
 error:
@@ -1055,6 +1065,10 @@ static int __iommu_free_buffer(struct device *dev, struct page **pages, size_t s
 	int count = size >> PAGE_SHIFT;
 	int array_size = count * sizeof(struct page *);
 	int i;
+
+	if (__free_from_pool(page_address(pages[0]), size))
+		return 0;
+
 	for (i = 0; i < count; i++)
 		if (pages[i])
 			__free_pages(pages[i], 0);
-- 
1.7.5.4

[RFC 3/4] ARM: dma-mapping: Return cpu addr when dma_alloc(GFP_ATOMIC)

[Hiroshi Doyu]

Make dma_alloc{_attrs}() returns cpu address when
DMA_ATTR_NO_KERNEL_MAPPING && GFP_ATOMIC.

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

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 9260107..a0cd476 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1198,8 +1198,11 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
 	if (*handle == DMA_ERROR_CODE)
 		goto err_buffer;
 
-	if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs))
+	if (dma_get_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs)) {
+		if (gfp & GFP_ATOMIC)
+			return page_address(pages[0]);
 		return pages;
+	}
 
 	addr = __iommu_alloc_remap(pages, size, gfp, prot,
 				   __builtin_return_address(0));
-- 
1.7.5.4

[RFC 4/4] ARM: dma-mapping: dma_{alloc,free}_coherent with empty attrs

[Hiroshi Doyu]

There's possibility that this attrs be used later to set.

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

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index c27d855..96b67c7 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -125,8 +125,6 @@ extern int dma_supported(struct device *dev, u64 mask);
 extern void *arm_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
 			   gfp_t gfp, struct dma_attrs *attrs);
 
-#define dma_alloc_coherent(d, s, h, f) dma_alloc_attrs(d, s, h, f, NULL)
-
 static inline void *dma_alloc_attrs(struct device *dev, size_t size,
 				       dma_addr_t *dma_handle, gfp_t flag,
 				       struct dma_attrs *attrs)
@@ -135,11 +133,21 @@ static inline void *dma_alloc_attrs(struct device *dev, size_t size,
 	void *cpu_addr;
 	BUG_ON(!ops);
 
+	if (flag & GFP_ATOMIC)
+		dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, attrs);
+
 	cpu_addr = ops->alloc(dev, size, dma_handle, flag, attrs);
 	debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr);
 	return cpu_addr;
 }
 
+static inline void *dma_alloc_coherent(struct device *dev, size_t size,
+				       dma_addr_t *dma_handle, gfp_t flag)
+{
+	DEFINE_DMA_ATTRS(attrs);
+	return dma_alloc_attrs(dev, size, dma_handle, flag, &attrs);
+}
+
 /**
  * arm_dma_free - free memory allocated by arm_dma_alloc
  * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
@@ -157,8 +165,6 @@ static inline void *dma_alloc_attrs(struct device *dev, size_t size,
 extern void arm_dma_free(struct device *dev, size_t size, void *cpu_addr,
 			 dma_addr_t handle, struct dma_attrs *attrs);
 
-#define dma_free_coherent(d, s, c, h) dma_free_attrs(d, s, c, h, NULL)
-
 static inline void dma_free_attrs(struct device *dev, size_t size,
 				     void *cpu_addr, dma_addr_t dma_handle,
 				     struct dma_attrs *attrs)
@@ -170,6 +176,13 @@ static inline void dma_free_attrs(struct device *dev, size_t size,
 	ops->free(dev, size, cpu_addr, dma_handle, attrs);
 }
 
+static inline void dma_free_coherent(struct device *dev, size_t size,
+				     void *cpu_addr, dma_addr_t dma_handle)
+{
+	DEFINE_DMA_ATTRS(attrs);
+	return dma_free_attrs(dev, size, cpu_addr, dma_handle, &attrs);
+}
+
 /**
  * arm_dma_mmap - map a coherent DMA allocation into user space
  * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
-- 
1.7.5.4

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

[Marek Szyprowski]

Hi Hiroshi,

On Wednesday, August 22, 2012 12:20 PM Hiroshi Doyu wrote:

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

I was working on the similar patches, but You were faster. ;-)

Basically the patch no 1 and 2 are fine, but I don't like the changes proposed in 
patch 3 and 4. You should not alter the attributes provided by the user nor make any
assumptions that such attributes has been provided - drivers are allowed to call 
dma_alloc_attrs() directly. Please rework your patches to avoid such approach.

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

RE: [RFC 2/4] ARM: dma-mapping: IOMMU allocates pages from pool with GFP_ATOMIC

[Marek Szyprowski]

Hello,

On Wednesday, August 22, 2012 12:20 PM Hiroshi Doyu wrote:

> Makes use of the same atomic pool from DMA, and skips kernel page
> mapping which can involves sleep'able operation at allocating a kernel
> page table.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
>  arch/arm/mm/dma-mapping.c |   22 ++++++++++++++++++----
>  1 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index aec0c06..9260107 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1028,7 +1028,6 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t
> size,
>  	struct page **pages;
>  	int count = size >> PAGE_SHIFT;
>  	int array_size = count * sizeof(struct page *);
> -	int err;
> 
>  	if (array_size <= PAGE_SIZE)
>  		pages = kzalloc(array_size, gfp);
> @@ -1037,9 +1036,20 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t
> size,
>  	if (!pages)
>  		return NULL;
> 
> -	err = __alloc_fill_pages(&pages, count, gfp);
> -	if (err)
> -		goto error
> +	if (gfp & GFP_ATOMIC) {
> +		struct page *page;
> +		int i;
> +		void *addr = __alloc_from_pool(size, &page);
> +		if (!addr)
> +			goto err_out;
> +
> +		for (i = 0; i < count; i++)
> +			pages[i] = page + i;
> +	} else {
> +		int err = __alloc_fill_pages(&pages, count, gfp);
> +		if (err)
> +			goto error;
> +	}
> 
>  	return pages;
>  error:
> @@ -1055,6 +1065,10 @@ static int __iommu_free_buffer(struct device *dev, struct page **pages,
> size_t s
>  	int count = size >> PAGE_SHIFT;
>  	int array_size = count * sizeof(struct page *);
>  	int i;
> +
> +	if (__free_from_pool(page_address(pages[0]), size))
> +		return 0;

You leak memory here. pages array should be also freed.

> +
>  	for (i = 0; i < count; i++)
>  		if (pages[i])
>  			__free_pages(pages[i], 0);
> --
> 1.7.5.4


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

Re: [RFC 2/4] ARM: dma-mapping: IOMMU allocates pages from pool with GFP_ATOMIC

[Hiroshi Doyu]

Marek Szyprowski <m.szyprowski@samsung.com> wrote @ Wed, 22 Aug 2012 14:29:47 +0200:

> Hello,
> 
> On Wednesday, August 22, 2012 12:20 PM Hiroshi Doyu wrote:
> 
> > Makes use of the same atomic pool from DMA, and skips kernel page
> > mapping which can involves sleep'able operation at allocating a kernel
> > page table.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> >  arch/arm/mm/dma-mapping.c |   22 ++++++++++++++++++----
> >  1 files changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index aec0c06..9260107 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -1028,7 +1028,6 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t
> > size,
> >  	struct page **pages;
> >  	int count = size >> PAGE_SHIFT;
> >  	int array_size = count * sizeof(struct page *);
> > -	int err;
> > 
> >  	if (array_size <= PAGE_SIZE)
> >  		pages = kzalloc(array_size, gfp);
> > @@ -1037,9 +1036,20 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t
> > size,
> >  	if (!pages)
> >  		return NULL;
> > 
> > -	err = __alloc_fill_pages(&pages, count, gfp);
> > -	if (err)
> > -		goto error
> > +	if (gfp & GFP_ATOMIC) {
> > +		struct page *page;
> > +		int i;
> > +		void *addr = __alloc_from_pool(size, &page);
> > +		if (!addr)
> > +			goto err_out;
> > +
> > +		for (i = 0; i < count; i++)
> > +			pages[i] = page + i;
> > +	} else {
> > +		int err = __alloc_fill_pages(&pages, count, gfp);
> > +		if (err)
> > +			goto error;
> > +	}
> > 
> >  	return pages;
> >  error:
> > @@ -1055,6 +1065,10 @@ static int __iommu_free_buffer(struct device *dev, struct page **pages,
> > size_t s
> >  	int count = size >> PAGE_SHIFT;
> >  	int array_size = count * sizeof(struct page *);
> >  	int i;
> > +
> > +	if (__free_from_pool(page_address(pages[0]), size))
> > +		return 0;
> 
> You leak memory here. pages array should be also freed.

Right, I'll fix as below:

	Modified arch/arm/mm/dma-mapping.c
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 47c4978..4656c0f 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1121,11 +1121,12 @@ static int __iommu_free_buffer(struct device *dev, struct page **pages, size_t s
 	int i;
 
 	if (__free_from_pool(page_address(pages[0]), size))
-		return 0;
+		goto out;
 
 	for (i = 0; i < count; i++)
 		if (pages[i])
 			__free_pages(pages[i], 0);
+out:
 	if (array_size <= PAGE_SIZE)
 		kfree(pages);
 	else

Re: [RFC 2/4] ARM: dma-mapping: IOMMU allocates pages from pool with GFP_ATOMIC

[Hiroshi Doyu]

Hi,

KyongHo Cho <pullip.cho@samsung.com> wrote @ Wed, 22 Aug 2012 14:47:00 +0200:

> vzalloc() call in __iommu_alloc_buffer() also causes BUG() in atomic context.

Right.

I've been thinking that kzalloc() may be enough here, since
vzalloc() was introduced to avoid allocation failure for big chunk of
memory, but I think that it's unlikely that the number of page array
can be so big. So I propose to drop vzalloc() here, and just simply to
use kzalloc only as below(*1).

For example, 

1920(H) x 1080(W) x 4(bytes) ~= 8MiB

For 8 MiB buffer,
  8(MiB) * 1024 = 8192(KiB)
  8192(KiB) / 4(KiB/page) = 2048 pages
  sizeof(struct page *) = 4 bytes
  2048(pages) * 4(bytes/page) = 8192(bytes) = 8(KiB)
  8(KiB) / 4(KiB/page) = 2 pages

If the above estimation is right(I hope;)), the necessary pages are
_at most_ 2 pages. If the system gets into the situation to fail to
allocate 2 contiguous pages, that's real the problem. I guess that
that kind of fragmentation problem would be solved with page migration
or something, especially nowadays devices are getting larger memories.

*1:
>From a613c40d1b3d4fb1577cdb0807a74e8dbd08a3e6 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu@nvidia.com>
Date: Wed, 22 Aug 2012 16:25:54 +0300
Subject: [PATCH 1/1] ARM: dma-mapping: Use only kzalloc without vzalloc

Use only kzalloc for atomic allocation.

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

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 4656c0f..d4f1cf2 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1083,10 +1083,7 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 	int count = size >> PAGE_SHIFT;
 	int array_size = count * sizeof(struct page *);
 
-	if (array_size <= PAGE_SIZE)
-		pages = kzalloc(array_size, gfp);
-	else
-		pages = vzalloc(array_size);
+	pages = kzalloc(array_size, gfp);
 	if (!pages)
 		return NULL;
 
@@ -1107,10 +1104,7 @@ static struct page **__iommu_alloc_buffer(struct device *dev, size_t size,
 
 	return pages;
 error:
-	if (array_size <= PAGE_SIZE)
-		kfree(pages);
-	else
-		vfree(pages);
+	kfree(pages);
 	return NULL;
 }
 
-- 
1.7.5.4

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

[Hiroshi Doyu]

Hi Marek,

Marek Szyprowski <m.szyprowski@samsung.com> wrote @ Wed, 22 Aug 2012 14:04:26 +0200:

> Hi Hiroshi,
> 
> On Wednesday, August 22, 2012 12:20 PM Hiroshi Doyu wrote:
> 
> > 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.
> 
> I was working on the similar patches, but You were faster. ;-)

Thank you for reviewing my patches.

> Basically the patch no 1 and 2 are fine, but I don't like the changes proposed in 
> patch 3 and 4. You should not alter the attributes provided by the user nor make any
> assumptions that such attributes has been provided - drivers are allowed to call 
> dma_alloc_attrs() directly. Please rework your patches to avoid such
> approach.

Sure. I'll send the series again later.

Instead of making use of DMA_ATTR_NO_KERNEL_MAPPING, I use the
following "__in_atomic_pool()" to see if buffer comes from atomic or
not at freeing.

>From cdf8621fd0876f3e56a55885c27e363893df2c98 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu@nvidia.com>
Date: Wed, 22 Aug 2012 17:26:29 +0300
Subject: [PATCH 1/1] ARM: dma-mapping: Refactor out to introduce
 __in_atomic_pool

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 |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 30bef80..26080ef 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -450,20 +450,30 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
 	return ptr;
 }
 
-static int __free_from_pool(void *start, size_t size)
+static bool __in_atomic_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;
+		return false;
 
 	if (start + size > pool->vaddr + pool->size) {
 		WARN(1, "freeing wrong coherent size from pool\n");
-		return 0;
+		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 (!__in_atomic_pool(start, size))
+		return 0;
+
 	pageno = (start - pool->vaddr) >> PAGE_SHIFT;
 	count = size >> PAGE_SHIFT;
 
-- 
1.7.5.4

Re: [RFC 2/4] ARM: dma-mapping: IOMMU allocates pages from pool with GFP_ATOMIC

[Minchan Kim]

On Wed, Aug 22, 2012 at 03:36:48PM +0200, Hiroshi Doyu wrote:
> Hi,
> 
> KyongHo Cho <pullip.cho@samsung.com> wrote @ Wed, 22 Aug 2012 14:47:00 +0200:
> 
> > vzalloc() call in __iommu_alloc_buffer() also causes BUG() in atomic context.
> 
> Right.
> 
> I've been thinking that kzalloc() may be enough here, since
> vzalloc() was introduced to avoid allocation failure for big chunk of
> memory, but I think that it's unlikely that the number of page array
> can be so big. So I propose to drop vzalloc() here, and just simply to
> use kzalloc only as below(*1).
> 
> For example, 
> 
> 1920(H) x 1080(W) x 4(bytes) ~= 8MiB
> 
> For 8 MiB buffer,
>   8(MiB) * 1024 = 8192(KiB)
>   8192(KiB) / 4(KiB/page) = 2048 pages
>   sizeof(struct page *) = 4 bytes
>   2048(pages) * 4(bytes/page) = 8192(bytes) = 8(KiB)
>   8(KiB) / 4(KiB/page) = 2 pages
> 
> If the above estimation is right(I hope;)), the necessary pages are
> _at most_ 2 pages. If the system gets into the situation to fail to
> allocate 2 contiguous pages, that's real the problem. I guess that
> that kind of fragmentation problem would be solved with page migration
> or something, especially nowadays devices are getting larger memories.

In atomic context, VM have no choice except relying on kswapd so
high order allocation can fail easily when memory fragementation
is high.

-- 
Kind regards,
Minchan Kim

RE: [RFC 2/4] ARM: dma-mapping: IOMMU allocates pages from pool with GFP_ATOMIC

[Marek Szyprowski]

Hello,

On Wednesday, August 22, 2012 3:37 PM Hiroshi Doyu wrote:

> KyongHo Cho <pullip.cho@samsung.com> wrote @ Wed, 22 Aug 2012 14:47:00 +0200:
> 
> > vzalloc() call in __iommu_alloc_buffer() also causes BUG() in atomic context.
> 
> Right.
> 
> I've been thinking that kzalloc() may be enough here, since
> vzalloc() was introduced to avoid allocation failure for big chunk of
> memory, but I think that it's unlikely that the number of page array
> can be so big. So I propose to drop vzalloc() here, and just simply to
> use kzalloc only as below(*1).

We already had a discussion about this, so I don't think it makes much sense to
change it back to kzalloc. This vmalloc() call won't hurt anyone. It should not
be considered a problem for atomic allocations, because no sane driver will try
to allocate buffers larger than a dozen KiB with GFP_ATOMIC flag. I would call
such try a serious bug, which we should not care here.

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

Re: [RFC 2/4] ARM: dma-mapping: IOMMU allocates pages from pool with GFP_ATOMIC

[Hiroshi Doyu]

Hi,

On Thu, 23 Aug 2012 07:58:34 +0200
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> Hello,
> 
> On Wednesday, August 22, 2012 3:37 PM Hiroshi Doyu wrote:
> 
> > KyongHo Cho <pullip.cho@samsung.com> wrote @ Wed, 22 Aug 2012 14:47:00 +0200:
> > 
> > > vzalloc() call in __iommu_alloc_buffer() also causes BUG() in atomic context.
> > 
> > Right.
> > 
> > I've been thinking that kzalloc() may be enough here, since
> > vzalloc() was introduced to avoid allocation failure for big chunk of
> > memory, but I think that it's unlikely that the number of page array
> > can be so big. So I propose to drop vzalloc() here, and just simply to
> > use kzalloc only as below(*1).
> 
> We already had a discussion about this, so I don't think it makes much sense to
> change it back to kzalloc. This vmalloc() call won't hurt anyone. It should not
> be considered a problem for atomic allocations, because no sane driver will try
> to allocate buffers larger than a dozen KiB with GFP_ATOMIC flag. I would call
> such try a serious bug, which we should not care here.

Ok, I've already sent v2 just now, where, instead of changing it back,
just with GFP_ATOMIC, kzalloc() would be selected, just in case. I guess
that this would be ok(a bit safer?)

RE: [RFC 2/4] ARM: dma-mapping: IOMMU allocates pages from pool with GFP_ATOMIC

[Marek Szyprowski]

Hi Hiroshi,

On Thursday, August 23, 2012 8:15 AM Hiroshi Doyu wrote:

> On Thu, 23 Aug 2012 07:58:34 +0200
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> 
> > Hello,
> >
> > On Wednesday, August 22, 2012 3:37 PM Hiroshi Doyu wrote:
> >
> > > KyongHo Cho <pullip.cho@samsung.com> wrote @ Wed, 22 Aug 2012 14:47:00 +0200:
> > >
> > > > vzalloc() call in __iommu_alloc_buffer() also causes BUG() in atomic context.
> > >
> > > Right.
> > >
> > > I've been thinking that kzalloc() may be enough here, since
> > > vzalloc() was introduced to avoid allocation failure for big chunk of
> > > memory, but I think that it's unlikely that the number of page array
> > > can be so big. So I propose to drop vzalloc() here, and just simply to
> > > use kzalloc only as below(*1).
> >
> > We already had a discussion about this, so I don't think it makes much sense to
> > change it back to kzalloc. This vmalloc() call won't hurt anyone. It should not
> > be considered a problem for atomic allocations, because no sane driver will try
> > to allocate buffers larger than a dozen KiB with GFP_ATOMIC flag. I would call
> > such try a serious bug, which we should not care here.
> 
> Ok, I've already sent v2 just now, where, instead of changing it back,
> just with GFP_ATOMIC, kzalloc() would be selected, just in case. I guess
> that this would be ok(a bit safer?)

I've posted some comments to v2. If you agree with my suggestion, no changes around
those vmalloc() calls will be needed.

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