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
There's possibility that this attrs be used later to set.
Signed-off-by: Hiroshi Doyu <[email protected]>
---
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
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 <[email protected]>
---
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
Make dma_alloc{_attrs}() returns cpu address when
DMA_ATTR_NO_KERNEL_MAPPING && GFP_ATOMIC.
Signed-off-by: Hiroshi Doyu <[email protected]>
---
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
__alloc_fill_pages() allocates power of 2 page number allocation at
most repeatedly.
Signed-off-by: Hiroshi Doyu <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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
Marek Szyprowski <[email protected]> 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 <[email protected]>
> > ---
> > 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
Hi,
KyongHo Cho <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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
Hi Marek,
Marek Szyprowski <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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
On Wed, Aug 22, 2012 at 03:36:48PM +0200, Hiroshi Doyu wrote:
> Hi,
>
> KyongHo Cho <[email protected]> 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
Hello,
On Wednesday, August 22, 2012 3:37 PM Hiroshi Doyu wrote:
> KyongHo Cho <[email protected]> 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
Hi,
On Thu, 23 Aug 2012 07:58:34 +0200
Marek Szyprowski <[email protected]> wrote:
> Hello,
>
> On Wednesday, August 22, 2012 3:37 PM Hiroshi Doyu wrote:
>
> > KyongHo Cho <[email protected]> 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?)
Hi Hiroshi,
On Thursday, August 23, 2012 8:15 AM Hiroshi Doyu wrote:
> On Thu, 23 Aug 2012 07:58:34 +0200
> Marek Szyprowski <[email protected]> wrote:
>
> > Hello,
> >
> > On Wednesday, August 22, 2012 3:37 PM Hiroshi Doyu wrote:
> >
> > > KyongHo Cho <[email protected]> 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