Skip to contentSkip to conversation

mm: dmapool: use provided gfp flags for all dma_alloc_coherent() calls

PATCH Superseded Marek Szyprowski 9 participants 64 messages Nov 8, 2012 – May 13, 2013

Subsystems (8)
Memory management Arm (32-bit) Media (V4L/DVB) Arm sub-architectures +4 more subsystems
lore
Share & tools
lore

Marek Szyprowski

author
#0 lore raw

PATCH 1/1 mm: dmapool: use provided gfp flags for all dma_alloc_coherent() calls

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. This patch changes the code to correctly use gfp flags provided by the dmapool caller. This should solve the dmapool usage on ARM architecture, where GFP_ATOMIC DMA allocations can be served only from the special, very limited memory pool.

Reported-by Soren Moch <[email protected]>
Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
---
 mm/dmapool.c |   27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)
mm/dmapool.c +7 -20
index c5ab33b..86de9b2 100644
@@ -62,8 +62,6 @@ struct dma_page {		/* cacheable header for 'allocation' bytes */
 	unsigned int offset;
 };
 
-#define	POOL_TIMEOUT_JIFFIES	((100 /* msec */ * HZ) / 1000)
-
 static DEFINE_MUTEX(pools_lock);
 
 static ssize_t

@@ -227,7 +225,6 @@ static struct dma_page *pool_alloc_page(struct dma_pool *pool, gfp_t mem_flags)
 		memset(page->vaddr, POOL_POISON_FREED, pool->allocation);
 #endif
 		pool_initialise_page(pool, page);
-		list_add(&page->page_list, &pool->page_list);
 		page->in_use = 0;
 		page->offset = 0;
 	} else {

@@ -315,30 +312,21 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 	might_sleep_if(mem_flags & __GFP_WAIT);
 
 	spin_lock_irqsave(&pool->lock, flags);
- restart:
 	list_for_each_entry(page, &pool->page_list, page_list) {
 		if (page->offset < pool->allocation)
 			goto ready;
 	}
-	page = pool_alloc_page(pool, GFP_ATOMIC);
-	if (!page) {
-		if (mem_flags & __GFP_WAIT) {
-			DECLARE_WAITQUEUE(wait, current);
 
-			__set_current_state(TASK_UNINTERRUPTIBLE);
-			__add_wait_queue(&pool->waitq, &wait);
-			spin_unlock_irqrestore(&pool->lock, flags);
+	/* pool_alloc_page() might sleep, so temporarily drop &pool->lock */
+	spin_unlock_irqrestore(&pool->lock, flags);
 
-			schedule_timeout(POOL_TIMEOUT_JIFFIES);
+	page = pool_alloc_page(pool, mem_flags);
+	if (!page)
+		return NULL;
 
-			spin_lock_irqsave(&pool->lock, flags);
-			__remove_wait_queue(&pool->waitq, &wait);
-			goto restart;
-		}
-		retval = NULL;
-		goto done;
-	}
+	spin_lock_irqsave(&pool->lock, flags);
 
+	list_add(&page->page_list, &pool->page_list);
  ready:
 	page->in_use++;
 	offset = page->offset;

@@ -348,7 +336,6 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 #ifdef	DMAPOOL_DEBUG
 	memset(retval, POOL_POISON_ALLOCATED, pool->size);
 #endif
- done:
 	spin_unlock_irqrestore(&pool->lock, flags);
 	return retval;
 }
Signature · 1 line
1.7.9.5
maintainer Tested-by on v1 1/1
#1 lore raw
Quoted 10 lines · Marek Szyprowski

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. This patch changes the code to correctly use gfp flags provided by the dmapool caller. This should solve the dmapool usage on ARM architecture, where GFP_ATOMIC DMA allocations can be served only from the special, very limited memory pool.

Reported-by Soren Moch <[email protected]>
Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Tested-by Andrew Lunn <[email protected]>

I tested this on a Kirkwood QNAP after removing the call to init_dma_coherent_pool_size().

Andrew

Quoted 74 lines
---
 mm/dmapool.c |   27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)
mm/dmapool.c +7 -20
index c5ab33b..86de9b2 100644
@@ -62,8 +62,6 @@ struct dma_page {		/* cacheable header for 'allocation' bytes */
 	unsigned int offset;
 };
 
-#define	POOL_TIMEOUT_JIFFIES	((100 /* msec */ * HZ) / 1000)
-
 static DEFINE_MUTEX(pools_lock);
 
 static ssize_t

@@ -227,7 +225,6 @@ static struct dma_page *pool_alloc_page(struct dma_pool *pool, gfp_t mem_flags)
 		memset(page->vaddr, POOL_POISON_FREED, pool->allocation);
 #endif
 		pool_initialise_page(pool, page);
-		list_add(&page->page_list, &pool->page_list);
 		page->in_use = 0;
 		page->offset = 0;
 	} else {

@@ -315,30 +312,21 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 	might_sleep_if(mem_flags & __GFP_WAIT);
 
 	spin_lock_irqsave(&pool->lock, flags);
- restart:
 	list_for_each_entry(page, &pool->page_list, page_list) {
 		if (page->offset < pool->allocation)
 			goto ready;
 	}
-	page = pool_alloc_page(pool, GFP_ATOMIC);
-	if (!page) {
-		if (mem_flags & __GFP_WAIT) {
-			DECLARE_WAITQUEUE(wait, current);
 
-			__set_current_state(TASK_UNINTERRUPTIBLE);
-			__add_wait_queue(&pool->waitq, &wait);
-			spin_unlock_irqrestore(&pool->lock, flags);
+	/* pool_alloc_page() might sleep, so temporarily drop &pool->lock */
+	spin_unlock_irqrestore(&pool->lock, flags);
 
-			schedule_timeout(POOL_TIMEOUT_JIFFIES);
+	page = pool_alloc_page(pool, mem_flags);
+	if (!page)
+		return NULL;
 
-			spin_lock_irqsave(&pool->lock, flags);
-			__remove_wait_queue(&pool->waitq, &wait);
-			goto restart;
-		}
-		retval = NULL;
-		goto done;
-	}
+	spin_lock_irqsave(&pool->lock, flags);
 
+	list_add(&page->page_list, &pool->page_list);
  ready:
 	page->in_use++;
 	offset = page->offset;

@@ -348,7 +336,6 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 #ifdef	DMAPOOL_DEBUG
 	memset(retval, POOL_POISON_ALLOCATED, pool->size);
 #endif
- done:
 	spin_unlock_irqrestore(&pool->lock, flags);
 	return retval;
 }
Signature · 1 line
1.7.9.5
Tested-by
#2 lore raw
Quoted 2 lines · Andrew Lunn
Quoted 1 line · Marek Szyprowski

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,

regardless

Quoted 2 lines
Quoted 2 lines

the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. This patch changes

the code

Quoted 3 lines
Quoted 3 lines

to correctly use gfp flags provided by the dmapool caller. This should solve the dmapool usage on ARM architecture, where GFP_ATOMIC DMA allocations can be served only from the special, very limited memory

pool.

Quoted 2 lines
Quoted 2 lines
Reported-by Soren Moch <[email protected]>

Please use

Reported-by Soeren Moch <[email protected]>
Quoted 9 lines
Quoted 2 lines
Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Tested-by Andrew Lunn <[email protected]>

I tested this on a Kirkwood QNAP after removing the call to init_dma_coherent_pool_size().

Andrew

Tested-by Soeren Moch <[email protected]>

Now I had a chance to test this patch on my Kirkwood guruplug system with linux-3.6.6 . It is running much better now, but with the original 256K coherent pool size I still see errors after several hours of runtime:

Nov 12 09:42:32 guru kernel: ERROR: 256 KiB atomic DMA coherent pool is 
too small!
Nov 12 09:42:32 guru kernel: Please increase it with coherent_pool=

kernel parameter!

Soeren

Quoted 9 lines

mm/dmapool.c struct dma_page { /* cacheable header for

Quoted 9 lines
---
 mm/dmapool.c |   27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)
mm/dmapool.c +0 -0
index c5ab33b..86de9b2 100644
@@ -62,8 +62,6 @@ struct dma_page {        /* cacheable header for

'allocation' bytes */

Quoted 9 lines

mm/dmapool.c static struct dma_page *pool_alloc_page(struct

Quoted 9 lines
mm/dmapool.c +0 -2
     unsigned int offset;
 };
 
-#define    POOL_TIMEOUT_JIFFIES    ((100 /* msec */ * HZ) / 1000)
-
 static DEFINE_MUTEX(pools_lock);
 
 static ssize_t

@@ -227,7 +225,6 @@ static struct dma_page *pool_alloc_page(struct

dma_pool *pool, gfp_t mem_flags)

Quoted 8 lines

mm/dmapool.c void *dma_pool_alloc(struct dma_pool *pool,

Quoted 8 lines
mm/dmapool.c +0 -1
         memset(page->vaddr, POOL_POISON_FREED, pool->allocation);
 #endif
         pool_initialise_page(pool, page);
-        list_add(&page->page_list, &pool->page_list);
         page->in_use = 0;
         page->offset = 0;
     } else {

@@ -315,30 +312,21 @@ void *dma_pool_alloc(struct dma_pool *pool,

gfp_t mem_flags,

Quoted 13 lines
Quoted 13 lines
mm/dmapool.c +0 -5
     might_sleep_if(mem_flags & __GFP_WAIT);
 
     spin_lock_irqsave(&pool->lock, flags);
- restart:
     list_for_each_entry(page, &pool->page_list, page_list) {
         if (page->offset < pool->allocation)
             goto ready;
     }
-    page = pool_alloc_page(pool, GFP_ATOMIC);
-    if (!page) {
-        if (mem_flags & __GFP_WAIT) {
-            DECLARE_WAITQUEUE(wait, current);
Quoted 4 lines

mm/dmapool.c void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,

Quoted 4 lines
mm/dmapool.c +1 -3
-            __set_current_state(TASK_UNINTERRUPTIBLE);
-            __add_wait_queue(&pool->waitq, &wait);
-            spin_unlock_irqrestore(&pool->lock, flags);
+    /* pool_alloc_page() might sleep, so temporarily drop

&pool->lock */

Quoted 16 lines
Quoted 16 lines
mm/dmapool.c +5 -8
+    spin_unlock_irqrestore(&pool->lock, flags);
 
-            schedule_timeout(POOL_TIMEOUT_JIFFIES);
+    page = pool_alloc_page(pool, mem_flags);
+    if (!page)
+        return NULL;
 
-            spin_lock_irqsave(&pool->lock, flags);
-            __remove_wait_queue(&pool->waitq, &wait);
-            goto restart;
-        }
-        retval = NULL;
-        goto done;
-    }
+    spin_lock_irqsave(&pool->lock, flags);
Quoted 5 lines

mm/dmapool.c void *dma_pool_alloc(struct dma_pool *pool,

Quoted 5 lines
mm/dmapool.c +1 -0
+    list_add(&page->page_list, &pool->page_list);
  ready:
     page->in_use++;
     offset = page->offset;

@@ -348,7 +336,6 @@ void *dma_pool_alloc(struct dma_pool *pool,

gfp_t mem_flags,

Quoted 10 lines
Quoted 10 lines
mm/dmapool.c +0 -2
 #ifdef    DMAPOOL_DEBUG
     memset(retval, POOL_POISON_ALLOCATED, pool->size);
 #endif
- done:
     spin_unlock_irqrestore(&pool->lock, flags);
     return retval;
 }
--

1.7.9.5

maintainer
#3 lore raw
Quoted 33 lines · Soeren Moch
Quoted 2 lines · Andrew Lunn
Quoted 1 line · Marek Szyprowski

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,

regardless

Quoted 2 lines
Quoted 2 lines

the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. This patch

changes the code

Quoted 3 lines
Quoted 3 lines

to correctly use gfp flags provided by the dmapool caller. This should solve the dmapool usage on ARM architecture, where GFP_ATOMIC DMA allocations can be served only from the special, very limited

memory pool.

Quoted 2 lines
Quoted 2 lines
Reported-by Soren Moch <[email protected]>

Please use

Reported-by Soeren Moch <[email protected]>
Quoted 9 lines
Quoted 2 lines
Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Tested-by Andrew Lunn <[email protected]>

I tested this on a Kirkwood QNAP after removing the call to init_dma_coherent_pool_size().

Andrew

Tested-by Soeren Moch <[email protected]>

Now I had a chance to test this patch on my Kirkwood guruplug system with linux-3.6.6 . It is running much better now, but with the original 256K coherent pool size I still see errors after several hours of runtime:

Quoted 4 lines
Nov 12 09:42:32 guru kernel: ERROR: 256 KiB atomic DMA coherent pool
is too small!
Nov 12 09:42:32 guru kernel: Please increase it with coherent_pool=

kernel parameter!

Hi Soeren

Could you tell us what DVB devices you are using.

Thanks
Andrew

#4 lore raw
Quoted 35 lines · Andrew Lunn
Quoted 30 lines · Soeren Moch
Quoted 2 lines · Andrew Lunn
Quoted 1 line · Marek Szyprowski

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,

regardless

Quoted 2 lines
Quoted 2 lines

the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. This patch

changes the code

Quoted 3 lines
Quoted 3 lines

to correctly use gfp flags provided by the dmapool caller. This should solve the dmapool usage on ARM architecture, where GFP_ATOMIC DMA allocations can be served only from the special, very limited

memory pool.

Quoted 1 line
Quoted 1 line
Reported-by Soren Moch <[email protected]>

Please use

Reported-by Soeren Moch <[email protected]>
Quoted 8 lines
Quoted 2 lines
Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Tested-by Andrew Lunn <[email protected]>

I tested this on a Kirkwood QNAP after removing the call to init_dma_coherent_pool_size().

Andrew

Tested-by Soeren Moch <[email protected]>

Now I had a chance to test this patch on my Kirkwood guruplug system with linux-3.6.6 . It is running much better now, but with the original 256K coherent pool size I still see errors after several hours of runtime:

Quoted 4 lines
Nov 12 09:42:32 guru kernel: ERROR: 256 KiB atomic DMA coherent pool
is too small!
Nov 12 09:42:32 guru kernel: Please increase it with coherent_pool=

kernel parameter!

Quoted 6 lines

Hi Soeren

Could you tell us what DVB devices you are using.

Thanks
Andrew

from lsusb:
Bus 001 Device 005: ID 0ccd:00b2 TerraTec Electronic GmbH Bus 001 Device 006: ID 2040:5200 Hauppauge
Bus 001 Device 009: ID 2304:0242 Pinnacle Systems, Inc.

If you want to check the drivers, I recommend to start with "em28xx".

Regards,
Soeren

maintainer
#5 lore raw

PATCH 1/1 ARM: Orion: Remove redundent init_dma_coherent_pool_size()

In reply to Marek Szyprowski

The patch:

mm: dmapool: use provided gfp flags for all dma_alloc_coherent() calls

makes these calls on Kirkword and Orion5x redundent. The drivers are not making atomic requests for coherent memory and hence the default pool size is now sufficient.

Signed-off-by Andrew Lunn <[email protected]>
---
 arch/arm/mach-kirkwood/common.c |    7 -------
 arch/arm/mach-orion5x/common.c  |    7 -------
 2 files changed, 14 deletions(-)
arch/arm/mach-kirkwood/common.c +0 -7
index 2c6c218..8080ceb 100644
@@ -505,13 +505,6 @@ void __init kirkwood_wdt_init(void)
 void __init kirkwood_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Kirkwood devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int kirkwood_tclk;
arch/arm/mach-orion5x/common.c +0 -7
index b3eb3da..3e07f52 100644
@@ -194,13 +194,6 @@ void __init orion5x_wdt_init(void)
 void __init orion5x_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Orion5x devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int orion5x_tclk;
Signature · 1 line
1.7.10.4
#6 lore raw

PATCH 1/1 ARM: Orion: Remove redundent init_dma_coherent_pool_size()

Quoted 8 lines · Andrew Lunn

The patch:

mm: dmapool: use provided gfp flags for all dma_alloc_coherent() calls

makes these calls on Kirkword and Orion5x redundent. The drivers are not making atomic requests for coherent memory and hence the default pool size is now sufficient.

Quoted 5 lines
Signed-off-by Andrew Lunn <[email protected]>
---
 arch/arm/mach-kirkwood/common.c |    7 -------
 arch/arm/mach-orion5x/common.c  |    7 -------
 2 files changed, 14 deletions(-)

Andrew,

I'm going to hold onto this until we hear from the mm maintainers on the parent patch. If it doesn't make v3.8, I know we'll have to extend this patch to remove the similar code from mach-mvebu...

thx,

Jason.

Quoted 45 lines
arch/arm/mach-kirkwood/common.c +0 -7
index 2c6c218..8080ceb 100644
@@ -505,13 +505,6 @@ void __init kirkwood_wdt_init(void)
 void __init kirkwood_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Kirkwood devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int kirkwood_tclk;
arch/arm/mach-orion5x/common.c +0 -7
index b3eb3da..3e07f52 100644
@@ -194,13 +194,6 @@ void __init orion5x_wdt_init(void)
 void __init orion5x_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Orion5x devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int orion5x_tclk;
Signature · 7 lines
1.7.10.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#7 lore raw

PATCH 1/1 ARM: Orion: Remove redundent init_dma_coherent_pool_size()

In reply to Andrew Lunn

Andrew,

Now that we finally have Soren Moch's (sp? sorry) cache exhaustion solved, can we respin this for v3.11? I'd like to include mvebu as well if possible.

thx,

Jason.

Quoted 58 lines · Andrew Lunn

The patch:

mm: dmapool: use provided gfp flags for all dma_alloc_coherent() calls

makes these calls on Kirkword and Orion5x redundent. The drivers are not making atomic requests for coherent memory and hence the default pool size is now sufficient.

Signed-off-by Andrew Lunn <[email protected]>
---
 arch/arm/mach-kirkwood/common.c |    7 -------
 arch/arm/mach-orion5x/common.c  |    7 -------
 2 files changed, 14 deletions(-)
arch/arm/mach-kirkwood/common.c +0 -7
index 2c6c218..8080ceb 100644
@@ -505,13 +505,6 @@ void __init kirkwood_wdt_init(void)
 void __init kirkwood_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Kirkwood devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int kirkwood_tclk;
arch/arm/mach-orion5x/common.c +0 -7
index b3eb3da..3e07f52 100644
@@ -194,13 +194,6 @@ void __init orion5x_wdt_init(void)
 void __init orion5x_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Orion5x devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int orion5x_tclk;
Signature · 7 lines
1.7.10.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#8 lore raw

PATCH 1/1 ARM: Orion: Remove redundent init_dma_coherent_pool_size()

Quoted 5 lines · Jason Cooper

Andrew,

Now that we finally have Soren Moch's (sp? sorry) cache exhaustion solved, can we respin this for v3.11? I'd like to include mvebu as well if possible.

For mvebu, I can do a follow-up patch, or Andrew can directly add it to his patch.

Regards,

Quoted 64 lines

thx,

Jason.

Quoted 58 lines · Andrew Lunn

The patch:

mm: dmapool: use provided gfp flags for all dma_alloc_coherent() calls

makes these calls on Kirkword and Orion5x redundent. The drivers are not making atomic requests for coherent memory and hence the default pool size is now sufficient.

Signed-off-by Andrew Lunn <[email protected]>
---
 arch/arm/mach-kirkwood/common.c |    7 -------
 arch/arm/mach-orion5x/common.c  |    7 -------
 2 files changed, 14 deletions(-)
arch/arm/mach-kirkwood/common.c +0 -7
index 2c6c218..8080ceb 100644
@@ -505,13 +505,6 @@ void __init kirkwood_wdt_init(void)
 void __init kirkwood_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Kirkwood devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int kirkwood_tclk;
arch/arm/mach-orion5x/common.c +0 -7
index b3eb3da..3e07f52 100644
@@ -194,13 +194,6 @@ void __init orion5x_wdt_init(void)
 void __init orion5x_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Orion5x devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int orion5x_tclk;
Signature · 7 lines
1.7.10.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Applied
#9 lore raw

PATCH 1/1 ARM: Orion: Remove redundent init_dma_coherent_pool_size()

Quoted 9 lines · Gregory CLEMENT
Quoted 5 lines · Jason Cooper

Andrew,

Now that we finally have Soren Moch's (sp? sorry) cache exhaustion solved, can we respin this for v3.11? I'd like to include mvebu as well if possible.

For mvebu, I can do a follow-up patch, or Andrew can directly add it to his patch.

Applied to mvebu/cleanup with a few minor typo corrections and I added the hunk for mach-mvebu/.

thx,

Jason.

Quoted 71 lines
Quoted 59 lines · Andrew Lunn
Quoted 58 lines

The patch:

mm: dmapool: use provided gfp flags for all dma_alloc_coherent() calls

makes these calls on Kirkword and Orion5x redundent. The drivers are not making atomic requests for coherent memory and hence the default pool size is now sufficient.

Signed-off-by Andrew Lunn <[email protected]>
---
 arch/arm/mach-kirkwood/common.c |    7 -------
 arch/arm/mach-orion5x/common.c  |    7 -------
 2 files changed, 14 deletions(-)
arch/arm/mach-kirkwood/common.c +0 -7
index 2c6c218..8080ceb 100644
@@ -505,13 +505,6 @@ void __init kirkwood_wdt_init(void)
 void __init kirkwood_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Kirkwood devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int kirkwood_tclk;
arch/arm/mach-orion5x/common.c +0 -7
index b3eb3da..3e07f52 100644
@@ -194,13 +194,6 @@ void __init orion5x_wdt_init(void)
 void __init orion5x_init_early(void)
 {
 	orion_time_set_base(TIMER_VIRT_BASE);
-
-	/*
-	 * Some Orion5x devices allocate their coherent buffers from atomic
-	 * context. Increase size of atomic coherent pool to make sure such
-	 * the allocations won't fail.
-	 */
-	init_dma_coherent_pool_size(SZ_1M);
 }
 
 int orion5x_tclk;
Signature · 7 lines
1.7.10.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 9 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#10 lore raw
In reply to Marek Szyprowski

Marek,

I've added the maintainers for mm/*. Hopefully they can let us know if this is good for v3.8...

thx,

Jason.

Quoted 89 lines · Marek Szyprowski

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. This patch changes the code to correctly use gfp flags provided by the dmapool caller. This should solve the dmapool usage on ARM architecture, where GFP_ATOMIC DMA allocations can be served only from the special, very limited memory pool.

Reported-by Soren Moch <[email protected]>
Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
---
 mm/dmapool.c |   27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)
mm/dmapool.c +7 -20
index c5ab33b..86de9b2 100644
@@ -62,8 +62,6 @@ struct dma_page {		/* cacheable header for 'allocation' bytes */
 	unsigned int offset;
 };
 
-#define	POOL_TIMEOUT_JIFFIES	((100 /* msec */ * HZ) / 1000)
-
 static DEFINE_MUTEX(pools_lock);
 
 static ssize_t

@@ -227,7 +225,6 @@ static struct dma_page *pool_alloc_page(struct dma_pool *pool, gfp_t mem_flags)
 		memset(page->vaddr, POOL_POISON_FREED, pool->allocation);
 #endif
 		pool_initialise_page(pool, page);
-		list_add(&page->page_list, &pool->page_list);
 		page->in_use = 0;
 		page->offset = 0;
 	} else {

@@ -315,30 +312,21 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 	might_sleep_if(mem_flags & __GFP_WAIT);
 
 	spin_lock_irqsave(&pool->lock, flags);
- restart:
 	list_for_each_entry(page, &pool->page_list, page_list) {
 		if (page->offset < pool->allocation)
 			goto ready;
 	}
-	page = pool_alloc_page(pool, GFP_ATOMIC);
-	if (!page) {
-		if (mem_flags & __GFP_WAIT) {
-			DECLARE_WAITQUEUE(wait, current);
 
-			__set_current_state(TASK_UNINTERRUPTIBLE);
-			__add_wait_queue(&pool->waitq, &wait);
-			spin_unlock_irqrestore(&pool->lock, flags);
+	/* pool_alloc_page() might sleep, so temporarily drop &pool->lock */
+	spin_unlock_irqrestore(&pool->lock, flags);
 
-			schedule_timeout(POOL_TIMEOUT_JIFFIES);
+	page = pool_alloc_page(pool, mem_flags);
+	if (!page)
+		return NULL;
 
-			spin_lock_irqsave(&pool->lock, flags);
-			__remove_wait_queue(&pool->waitq, &wait);
-			goto restart;
-		}
-		retval = NULL;
-		goto done;
-	}
+	spin_lock_irqsave(&pool->lock, flags);
 
+	list_add(&page->page_list, &pool->page_list);
  ready:
 	page->in_use++;
 	offset = page->offset;

@@ -348,7 +336,6 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 #ifdef	DMAPOOL_DEBUG
 	memset(retval, POOL_POISON_ALLOCATED, pool->size);
 #endif
- done:
 	spin_unlock_irqrestore(&pool->lock, flags);
 	return retval;
 }
Signature · 7 lines
1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
maintainer
#11 lore raw
Quoted 2 lines · On Sun, 18 Nov 2012 19:18:46 -0500 Jason Cooper <[email protected]> wrote:

I've added the maintainers for mm/*. Hopefully they can let us know if this is good for v3.8...

As Marek has inexplicably put this patch into linux-next via his tree, we don't appear to be getting a say in the matter!

The patch looks good to me. That open-coded wait loop predates the creation of bitkeeper tree(!) but doesn't appear to be needed. There will perhaps be some behavioural changes observable for GFP_KERNEL callers as dma_pool_alloc() will no longer dip into page reserves but I see nothing special about dma_pool_alloc() which justifies doing that anyway.

The patch makes pool->waitq and its manipulation obsolete, but it failed to remove all that stuff.

The changelog failed to describe the problem which Soren reported. That should be included, and as the problem sounds fairly serious we might decide to backport the fix into -stable kernels.

dma_pool_alloc()'s use of a local "struct dma_page *page" is distressing - MM developers very much expect a local called "page" to have type "struct page *". But that's a separate issue.

As this patch is already in -next and is stuck there for two more weeks I can't (or at least won't) merge this patch, so I can't help with any of the above.

author
#12 lore raw

Hello,

Quoted 8 lines · Andrew Morton
Quoted 2 lines · On Sun, 18 Nov 2012 19:18:46 -0500 Jason Cooper <[email protected]> wrote:

I've added the maintainers for mm/*. Hopefully they can let us know if this is good for v3.8...

As Marek has inexplicably put this patch into linux-next via his tree, we don't appear to be getting a say in the matter!

I've just put this patch to linux-next via my dma-mapping tree to give it some testing asap to check if other changes to arm dma-mapping are required or not.

Quoted 9 lines

The patch looks good to me. That open-coded wait loop predates the creation of bitkeeper tree(!) but doesn't appear to be needed. There will perhaps be some behavioural changes observable for GFP_KERNEL callers as dma_pool_alloc() will no longer dip into page reserves but I see nothing special about dma_pool_alloc() which justifies doing that anyway.

The patch makes pool->waitq and its manipulation obsolete, but it failed to remove all that stuff.

Right, I missed that part, I will update it asap.

Quoted 3 lines

The changelog failed to describe the problem which Soren reported. That should be included, and as the problem sounds fairly serious we might decide to backport the fix into -stable kernels.

Ok, I will extend the changelog.

Quoted 3 lines

dma_pool_alloc()'s use of a local "struct dma_page *page" is distressing - MM developers very much expect a local called "page" to have type "struct page *". But that's a separate issue.

I will prepare a separate patch cleaning it. I was also a bit surprised by such naming scheme, but it is probably related to the fact that this come has not been touched much since a very ancient times.

Quoted 3 lines

As this patch is already in -next and is stuck there for two more weeks I can't (or at least won't) merge this patch, so I can't help with any of the above.

I will fix both issues in the next version of the patch. Would like to merge it to your tree or should I keep it in my dma-mapping tree?

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
maintainer
#13 lore raw
Quoted 6 lines · On Tue, 20 Nov 2012 11:48:44 +0100 Marek Szyprowski <[email protected]>…
Quoted 3 lines

As this patch is already in -next and is stuck there for two more weeks I can't (or at least won't) merge this patch, so I can't help with any of the above.

I will fix both issues in the next version of the patch. Would like to merge it to your tree or should I keep it in my dma-mapping tree?

The patch looks OK to me now (still wondering about the -stable question though).

It would be a bit of a pain for me to carry a patch which conflicts with one in linux-next, and this patch doesn't appear to conflict with the other pending dmapool.c patches in -mm so you may as well keep it in the dma-mapping tree now.

author
#14 lore raw

PATCH v2 1/1 mm: dmapool: use provided gfp flags for all dma_alloc_coherent() calls

In reply to Andrew Morton

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

This patch changes the dmapool code to correctly use gfp flags provided by the dmapool caller.

Reported-by Soeren Moch <[email protected]>
Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Tested-by Andrew Lunn <[email protected]>
Tested-by Soeren Moch <[email protected]>

---

changelog
v2:
- removed all waitq related stuff
- extended commit message

 mm/dmapool.c |   31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)
mm/dmapool.c +7 -24
index c5ab33b..da1b0f0 100644
@@ -50,7 +50,6 @@ struct dma_pool {		/* the pool */
 	size_t allocation;
 	size_t boundary;
 	char name[32];
-	wait_queue_head_t waitq;
 	struct list_head pools;
 };
 

@@ -62,8 +61,6 @@ struct dma_page {		/* cacheable header for 'allocation' bytes */
 	unsigned int offset;
 };
 
-#define	POOL_TIMEOUT_JIFFIES	((100 /* msec */ * HZ) / 1000)
-
 static DEFINE_MUTEX(pools_lock);
 
 static ssize_t

@@ -172,7 +169,6 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	retval->size = size;
 	retval->boundary = boundary;
 	retval->allocation = allocation;
-	init_waitqueue_head(&retval->waitq);
 
 	if (dev) {
 		int ret;

@@ -227,7 +223,6 @@ static struct dma_page *pool_alloc_page(struct dma_pool *pool, gfp_t mem_flags)
 		memset(page->vaddr, POOL_POISON_FREED, pool->allocation);
 #endif
 		pool_initialise_page(pool, page);
-		list_add(&page->page_list, &pool->page_list);
 		page->in_use = 0;
 		page->offset = 0;
 	} else {

@@ -315,30 +310,21 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 	might_sleep_if(mem_flags & __GFP_WAIT);
 
 	spin_lock_irqsave(&pool->lock, flags);
- restart:
 	list_for_each_entry(page, &pool->page_list, page_list) {
 		if (page->offset < pool->allocation)
 			goto ready;
 	}
-	page = pool_alloc_page(pool, GFP_ATOMIC);
-	if (!page) {
-		if (mem_flags & __GFP_WAIT) {
-			DECLARE_WAITQUEUE(wait, current);
 
-			__set_current_state(TASK_UNINTERRUPTIBLE);
-			__add_wait_queue(&pool->waitq, &wait);
-			spin_unlock_irqrestore(&pool->lock, flags);
+	/* pool_alloc_page() might sleep, so temporarily drop &pool->lock */
+	spin_unlock_irqrestore(&pool->lock, flags);
 
-			schedule_timeout(POOL_TIMEOUT_JIFFIES);
+	page = pool_alloc_page(pool, mem_flags);
+	if (!page)
+		return NULL;
 
-			spin_lock_irqsave(&pool->lock, flags);
-			__remove_wait_queue(&pool->waitq, &wait);
-			goto restart;
-		}
-		retval = NULL;
-		goto done;
-	}
+	spin_lock_irqsave(&pool->lock, flags);
 
+	list_add(&page->page_list, &pool->page_list);
  ready:
 	page->in_use++;
 	offset = page->offset;

@@ -348,7 +334,6 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 #ifdef	DMAPOOL_DEBUG
 	memset(retval, POOL_POISON_ALLOCATED, pool->size);
 #endif
- done:
 	spin_unlock_irqrestore(&pool->lock, flags);
 	return retval;
 }

@@ -435,8 +420,6 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
 	page->in_use--;
 	*(int *)vaddr = page->offset;
 	page->offset = offset;
-	if (waitqueue_active(&pool->waitq))
-		wake_up_locked(&pool->waitq);
 	/*
 	 * Resist a temptation to do
 	 *    if (!is_page_busy(page)) pool_free_page(pool, page);
Signature · 1 line
1.7.9.5
maintainer
#15 lore raw
Quoted 11 lines · On Tue, 20 Nov 2012 15:31:45 +0100 Marek Szyprowski <[email protected]>…

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Is this problem serious enough to justify merging the patch into 3.7? And into -stable kernels?

#16 lore raw
Quoted 15 lines · Andrew Morton
Quoted 11 lines · On Tue, 20 Nov 2012 15:31:45 +0100 Marek Szyprowski <[email protected]>…

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Quoted 2 lines

Is this problem serious enough to justify merging the patch into 3.7? And into -stable kernels?

kirkwood and orion5x currently have the following code in their early init:

/*
* Some Kirkwood devices allocate their coherent buffers from atomic
* context. Increase size of atomic coherent pool to make sure such the
* allocations won't fail.
*/
init_dma_coherent_pool_size(SZ_1M);

We have a pending patch to do the same for mvebu (new armv7 Marvell SoCs). There is at least one reported real world case where even the above isn't sufficient [1].

thx,

Jason.

[1] http://www.spinics.net/lists/arm-kernel/msg205495.html

author
#17 lore raw
In reply to Andrew Morton

Hello,

Quoted 15 lines · Andrew Morton
Quoted 11 lines · On Tue, 20 Nov 2012 15:31:45 +0100 Marek Szyprowski <[email protected]>…

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Quoted 2 lines

Is this problem serious enough to justify merging the patch into 3.7? And into -stable kernels?

I wonder if it is a good idea to merge such change at the end of current -rc period. It changes the behavior of dma pool allocations and I bet there might be some drivers which don't care much about passed gfp flags, as for ages it simply worked for them, even if the allocations were done from atomic context. What do You think? Technically it is also not a pure bugfix, so imho it shouldn't be considered for -stable.

On the other hand at least for ARM users of sata_mv driver (which is just an innocent client of dma pool, correctly passing GFP_KERNEL flag) it would solve the issues related to shortage of atomic pool for dma allocations what might justify pushing it to 3.7.

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
maintainer
#18 lore raw
Quoted 18 lines · On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <[email protected]>…

Hello,

Quoted 15 lines · Andrew Morton
Quoted 11 lines · On Tue, 20 Nov 2012 15:31:45 +0100 Marek Szyprowski <[email protected]>…

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Quoted 5 lines
Quoted 2 lines

Is this problem serious enough to justify merging the patch into 3.7? And into -stable kernels?

I wonder if it is a good idea to merge such change at the end of current -rc period.

I'm not sure what you mean by this.

But what we do sometimes if we think a patch needs a bit more real-world testing before backporting is to merge it into -rc1 in the normal merge window, and tag it for -stable backporting. That way it gets a few weeks(?) testing in mainline before getting backported.

author
#19 lore raw

Hello,

Quoted 26 lines · Andrew Morton
Quoted 18 lines · On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <[email protected]>…

Hello,

Quoted 15 lines · Andrew Morton
Quoted 11 lines · On Tue, 20 Nov 2012 15:31:45 +0100 Marek Szyprowski <[email protected]>…

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Quoted 5 lines
Quoted 2 lines

Is this problem serious enough to justify merging the patch into 3.7? And into -stable kernels?

I wonder if it is a good idea to merge such change at the end of current -rc period.

Quoted 6 lines

I'm not sure what you mean by this.

But what we do sometimes if we think a patch needs a bit more real-world testing before backporting is to merge it into -rc1 in the normal merge window, and tag it for -stable backporting. That way it gets a few weeks(?) testing in mainline before getting backported.

I just wondered that if it gets merged to v3.7-rc7 there won't be much time for real-world testing before final v3.7 release. This patch is in linux-next for over a week and I'm not aware of any issues, but -rc releases gets much more attention and testing than linux-next tree.

If You think it's fine to put such change to v3.7-rc7 I will send a pull request and tag it for stable asap.

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
maintainer
#20 lore raw
Quoted 36 lines · On Wed, 21 Nov 2012 10:20:07 +0100 Marek Szyprowski <[email protected]>…

Hello,

Quoted 26 lines · Andrew Morton
Quoted 18 lines · On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <[email protected]>…

Hello,

Quoted 15 lines · Andrew Morton
Quoted 11 lines · On Tue, 20 Nov 2012 15:31:45 +0100 Marek Szyprowski <[email protected]>…

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Quoted 5 lines
Quoted 2 lines

Is this problem serious enough to justify merging the patch into 3.7? And into -stable kernels?

I wonder if it is a good idea to merge such change at the end of current -rc period.

Quoted 6 lines

I'm not sure what you mean by this.

But what we do sometimes if we think a patch needs a bit more real-world testing before backporting is to merge it into -rc1 in the normal merge window, and tag it for -stable backporting. That way it gets a few weeks(?) testing in mainline before getting backported.

Quoted 8 lines

I just wondered that if it gets merged to v3.7-rc7 there won't be much time for real-world testing before final v3.7 release. This patch is in linux-next for over a week and I'm not aware of any issues, but -rc releases gets much more attention and testing than linux-next tree.

If You think it's fine to put such change to v3.7-rc7 I will send a pull request and tag it for stable asap.

What I'm suggesting is that it be merged for 3.8-rc1 with a -stable tag, then it will be backported into 3.7.x later on.

author
#21 lore raw
Quoted 38 lines · Andrew Morton
Quoted 36 lines · On Wed, 21 Nov 2012 10:20:07 +0100 Marek Szyprowski <[email protected]>…
Quoted 23 lines · Andrew Morton
Quoted 16 lines · On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <[email protected]>…
Quoted 15 lines · Andrew Morton
Quoted 11 lines · On Tue, 20 Nov 2012 15:31:45 +0100 Marek Szyprowski <[email protected]>…

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Quoted 5 lines
Quoted 2 lines

Is this problem serious enough to justify merging the patch into 3.7? And into -stable kernels?

I wonder if it is a good idea to merge such change at the end of current -rc period.

Quoted 6 lines

I'm not sure what you mean by this.

But what we do sometimes if we think a patch needs a bit more real-world testing before backporting is to merge it into -rc1 in the normal merge window, and tag it for -stable backporting. That way it gets a few weeks(?) testing in mainline before getting backported.

I just wondered that if it gets merged to v3.7-rc7 there won't be much time for real-world testing before final v3.7 release. This patch is in linux-next for over a week and I'm not aware of any issues, but -rc releases gets much more attention and testing than linux-next tree.

Quoted 5 lines
Quoted 2 lines

If You think it's fine to put such change to v3.7-rc7 I will send a pull request and tag it for stable asap.

What I'm suggesting is that it be merged for 3.8-rc1 with a -stable tag, then it will be backported into 3.7.x later on.

OK, I will push it to v3.8-rc1 then and tag for stable.

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#22 lore raw
In reply to Marek Szyprowski
Quoted 11 lines · Marek Szyprowski

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Quoted 8 lines

This patch changes the dmapool code to correctly use gfp flags provided by the dmapool caller.

Reported-by Soeren Moch <[email protected]>
Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Tested-by Andrew Lunn <[email protected]>
Tested-by Soeren Moch <[email protected]>

Now I tested linux-3.7.1 (this patch is included there) on my Marvell Kirkwood system. I still see

ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!

after several hours of runtime under heavy load with SATA and DVB-Sticks (em28xx / drxk and dib0700).

As already reported earlier this patch improved the behavior compared to linux-3.6.x and 3.7.0 (error after several ten minutes runtime), but I still see a regression compared to linux-3.5.x. With this kernel the same system with same workload runs flawlessly.

Regards,
Soeren

#23 lore raw

Greg,

I've added you to the this thread hoping for a little insight into USB drivers and their use of coherent and GFP_ATOMIC. Am I barking up the wrong tree by looking a the drivers?

Quoted 21 lines · Soeren Moch
Quoted 11 lines · Marek Szyprowski

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Quoted 8 lines

This patch changes the dmapool code to correctly use gfp flags provided by the dmapool caller.

Reported-by Soeren Moch <[email protected]>
Reported-by Thomas Petazzoni <[email protected]>
Signed-off-by Marek Szyprowski <[email protected]>
Tested-by Andrew Lunn <[email protected]>
Tested-by Soeren Moch <[email protected]>
Quoted 8 lines

Now I tested linux-3.7.1 (this patch is included there) on my Marvell Kirkwood system. I still see

ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!

after several hours of runtime under heavy load with SATA and DVB-Sticks (em28xx / drxk and dib0700).

Could you try running the system w/o the em28xx stick and see how it goes with v3.7.1?

thx,

Jason.

Quoted 14 lines

As already reported earlier this patch improved the behavior compared to linux-3.6.x and 3.7.0 (error after several ten minutes runtime), but
I still see a regression compared to linux-3.5.x. With this kernel the same system with same workload runs flawlessly.

Regards,
Soeren

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#24 lore raw
Quoted 5 lines · Jason Cooper

Greg,

I've added you to the this thread hoping for a little insight into USB drivers and their use of coherent and GFP_ATOMIC. Am I barking up the wrong tree by looking a the drivers?

I don't understand, which drivers are you referring to? USB host controller drivers, or the "normal" drivers? Most USB drivers use GFP_ATOMIC if they are creating memory during their URB callback path, as that is interrupt context. But it shouldn't be all that bad, and the USB core hasn't changed in a while, so something else must be causing this.

greg k-h

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#25 lore raw
Quoted 9 lines · Greg KH
Quoted 5 lines · Jason Cooper

Greg,

I've added you to the this thread hoping for a little insight into USB drivers and their use of coherent and GFP_ATOMIC. Am I barking up the wrong tree by looking a the drivers?

I don't understand, which drivers are you referring to? USB host controller drivers, or the "normal" drivers?

Sorry I wasn't clear, I was referring specifically to the usb dvb drivers em28xx, drxk and dib0700. These are the drivers reported to be in heavy use when the error occurs.

sata_mv is also in use, however no other users of sata_mv have reported problems. Including myself. ;-)

Quoted 4 lines

Most USB drivers use GFP_ATOMIC if they are creating memory during their URB callback path, as that is interrupt context. But it shouldn't be all that bad, and the USB core hasn't changed in a while, so something else must be causing this.

Agreed, so I went and did more reading. The key piece of the puzzle that I was missing was in arch/arm/mm/dma-mapping.c 660-684.

/*
* Allocate DMA-coherent memory space and return both the kernel
* remapped
* virtual and bus address for that space.
*/
void *arm_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
gfp_t gfp, struct dma_attrs *attrs)

{
        pgprot_t prot = __get_dma_pgprot(attrs, pgprot_kernel);
        void *memory;

        if (dma_alloc_from_coherent(dev, size, handle, &memory))
                return memory;

        return __dma_alloc(dev, size, handle, gfp, prot, false,
                           __builtin_return_address(0));
}

static void *arm_coherent_dma_alloc(struct device *dev, size_t size,
dma_addr_t *handle, gfp_t gfp, struct dma_attrs *attrs)

{
        pgprot_t prot = __get_dma_pgprot(attrs, pgprot_kernel);
        void *memory;

        if (dma_alloc_from_coherent(dev, size, handle, &memory))
                return memory;

        return __dma_alloc(dev, size, handle, gfp, prot, true,
                           __builtin_return_address(0));
}

My understanding of this code is that when a driver requests dma memory, we will first try to alloc from the per-driver pool. If that fails, we will then attempt to allocate from the atomic_pool.

Once the atomic_pool is exhausted, we get the error:

ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!

If my understanding is correct, one of the drivers (most likely one) either asks for too small of a dma buffer, or is not properly deallocating blocks from the per-device pool. Either case leads to exhaustion, and falling back to the atomic pool. Which subsequently gets wiped out as well.

Am I on the right track?

thx,

Jason.

#26 lore raw

Soeren,

Quoted 5 lines · Jason Cooper

If my understanding is correct, one of the drivers (most likely one) either asks for too small of a dma buffer, or is not properly deallocating blocks from the per-device pool. Either case leads to exhaustion, and falling back to the atomic pool. Which subsequently gets wiped out as well.

If my hunch is right, could you please try each of the three dvb drivers in turn and see which one (or more than one) causes the error?

thx,

Jason.

#27 lore raw
Quoted 12 lines · Jason Cooper

Soeren,

Quoted 5 lines · Jason Cooper

If my understanding is correct, one of the drivers (most likely one) either asks for too small of a dma buffer, or is not properly deallocating blocks from the per-device pool. Either case leads to exhaustion, and falling back to the atomic pool. Which subsequently gets wiped out as well.

If my hunch is right, could you please try each of the three dvb drivers in turn and see which one (or more than one) causes the error?

In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk demodulator, and dib0700 usb bridge plus dib7000p demod.

I would bet for em28xx causing the error, but this is not thoroughly tested. Unfortunately testing with removed sticks is not easy, because this is a production system and disabling some services for the long time we need to trigger this error will certainly result in unhappy users.

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

In linux-3.5.x there is no such problem. Can we use all available memory for dma buffers here on armv5 architectures, in contrast to newer kernels?

Regards,
Soeren

#28 lore raw

Soeren,

Quoted 11 lines · Soeren Moch
Quoted 9 lines · Jason Cooper
Quoted 5 lines · Jason Cooper

If my understanding is correct, one of the drivers (most likely one) either asks for too small of a dma buffer, or is not properly deallocating blocks from the per-device pool. Either case leads to exhaustion, and falling back to the atomic pool. Which subsequently gets wiped out as well.

If my hunch is right, could you please try each of the three dvb drivers in turn and see which one (or more than one) causes the error?

Quoted 8 lines

In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk demodulator, and dib0700 usb bridge plus dib7000p demod.

I would bet for em28xx causing the error, but this is not thoroughly tested. Unfortunately testing with removed sticks is not easy, because this is a production system and disabling some services for the long time we need to trigger this error will certainly result in unhappy users.

Just out of curiosity, what board is it?

Quoted 2 lines

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

DMA_API_DEBUG

Quoted 3 lines

In linux-3.5.x there is no such problem. Can we use all available memory for dma buffers here on armv5 architectures, in contrast to newer kernels?

Were the loads exactly the same when you tested 3.5.x? I looked at the changes from v3.5 to v3.7.1 for all four drivers you mentioned as well as sata_mv.

The biggest thing I see is that all of the media drivers got shuffled around into their own subdirectories after v3.5. 'git show -M 0c0d06c' shows it was a clean copy of all the files.

What would be most helpful is if you could do a git bisect between v3.5.x (working) and the oldest version where you know it started failing (v3.7.1 or earlier if you know it).

thx,

Jason.

#29 lore raw
Quoted 17 lines · Jason Cooper

Soeren,

Quoted 14 lines · Soeren Moch
Quoted 9 lines · Jason Cooper
Quoted 5 lines · Jason Cooper

If my understanding is correct, one of the drivers (most likely one) either asks for too small of a dma buffer, or is not properly deallocating blocks from the per-device pool. Either case leads to exhaustion, and falling back to the atomic pool. Which subsequently gets wiped out as well.

If my hunch is right, could you please try each of the three dvb drivers in turn and see which one (or more than one) causes the error?

In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk demodulator, and dib0700 usb bridge plus dib7000p demod.

Quoted 7 lines
Quoted 5 lines

I would bet for em28xx causing the error, but this is not thoroughly tested. Unfortunately testing with removed sticks is not easy, because this is a production system and disabling some services for the long time we need to trigger this error will certainly result in unhappy users.

Just out of curiosity, what board is it?

The kirkwood board? A modified Guruplug Server Plus.

Quoted 5 lines
Quoted 3 lines

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

DMA_API_DEBUG

OK, maybe I can try this.

Quoted 6 lines
Quoted 4 lines

In linux-3.5.x there is no such problem. Can we use all available memory for dma buffers here on armv5 architectures, in contrast to newer kernels?

Were the loads exactly the same when you tested 3.5.x?

Exactly the same, yes.

Quoted 4 lines

I looked at the
changes from v3.5 to v3.7.1 for all four drivers you mentioned as well as sata_mv.

Quoted 8 lines

The biggest thing I see is that all of the media drivers got shuffled around into their own subdirectories after v3.5. 'git show -M 0c0d06c' shows it was a clean copy of all the files.

What would be most helpful is if you could do a git bisect between v3.5.x (working) and the oldest version where you know it started failing (v3.7.1 or earlier if you know it).

I did not bisect it, but Marek mentioned earlier that commit e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced new code for dma allocations. This is probably the root cause for the new (mis-)behavior (due to my tests 3.6.0 is not working anymore). I'm not very familiar with arm mm code, and from the patch itself I cannot understand what's different. Maybe CONFIG_CMA is default also for armv5 (not only v6) now? But I might be totally wrong here, maybe someone of the mm experts can explain the difference?

Regards,
Soeren

#30 lore raw
Quoted 19 lines · Soeren Moch
Quoted 18 lines · Jason Cooper

Soeren,

Quoted 15 lines · Soeren Moch
Quoted 10 lines · Jason Cooper
Quoted 5 lines · Jason Cooper

If my understanding is correct, one of the drivers (most likely one) either asks for too small of a dma buffer, or is not properly deallocating blocks from the per-device pool. Either case leads to exhaustion, and falling back to the atomic pool. Which subsequently gets wiped out as well.

If my hunch is right, could you please try each of the three dvb drivers
in turn and see which one (or more than one) causes the error?

In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk demodulator, and dib0700 usb bridge plus dib7000p demod.

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines

I would bet for em28xx causing the error, but this is not thoroughly tested. Unfortunately testing with removed sticks is not easy, because this is a production system and disabling some services for the long time we need to trigger this error will certainly result in unhappy users.

OK, I could trigger the error
ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!
only with em28xx sticks and sata, dib0700 sticks removed.

Quoted 3 lines
Quoted 1 line

Just out of curiosity, what board is it?

The kirkwood board? A modified Guruplug Server Plus.

em28xx sticks: "TerraTec Cinergy HTC Stick HD" and "PCTV Quatro Stick" dib0700 sticks: "WinTV-NOVA-TD Stick"

Quoted 24 lines
Quoted 5 lines
Quoted 3 lines

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

DMA_API_DEBUG

OK, maybe I can try this.

Quoted 6 lines
Quoted 4 lines

In linux-3.5.x there is no such problem. Can we use all available memory for dma buffers here on armv5 architectures, in contrast to newer kernels?

Were the loads exactly the same when you tested 3.5.x?

Exactly the same, yes.

Quoted 8 lines

I looked at the
changes from v3.5 to v3.7.1 for all four drivers you mentioned as well as sata_mv.

The biggest thing I see is that all of the media drivers got shuffled around into their own subdirectories after v3.5. 'git show -M 0c0d06c' shows it was a clean copy of all the files.

Quoted 8 lines
Quoted 4 lines

What would be most helpful is if you could do a git bisect between v3.5.x (working) and the oldest version where you know it started failing (v3.7.1 or earlier if you know it).

I did not bisect it, but Marek mentioned earlier that commit e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced new code for dma allocations. This is probably the root cause for the new (mis-)behavior (due to my tests 3.6.0 is not working anymore).

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

Quoted 5 lines

I'm not very familiar with arm mm code, and from the patch itself I cannot understand what's different. Maybe CONFIG_CMA is default also for armv5 (not only v6) now? But I might be totally wrong here, maybe someone of the mm experts can explain the difference?

Regards,
Soeren

#31 lore raw

PATCH 1/1 ata: sata_mv: fix sg_tbl_pool alignment

Quoted 10 lines · Soeren Moch
Quoted 4 lines · Soeren Moch
Quoted 3 lines · Jason Cooper
Quoted 2 lines · Soeren Moch
Quoted 1 line · Jason Cooper

On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:

OK, I could trigger the error
ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!
only with em28xx sticks and sata, dib0700 sticks removed.

Did you test the reverse scenario? ie dib0700 with sata_mv and no em28xx.

What kind of throughput are you pushing to the sata disk?

Quoted 11 lines
Quoted 8 lines
Quoted 4 lines

What would be most helpful is if you could do a git bisect between v3.5.x (working) and the oldest version where you know it started failing (v3.7.1 or earlier if you know it).

I did not bisect it, but Marek mentioned earlier that commit e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced new code for dma allocations. This is probably the root cause for the new (mis-)behavior (due to my tests 3.6.0 is not working anymore).

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

Of the four drivers you listed, none are using dma. sata_mv is the only one.

If one is to believe the comments in sata_mv.c:~151, then the alignment is wrong for the sg_tbl_pool.

Could you please try the following patch?

thx,

Jason.

---8<----------

Quoted 1 line

From 566c7e30285e4c31d76724ea4811b016b753f24f Mon Sep 17 00:00:00 2001

From: Jason Cooper <[email protected]>
Date: Wed, 16 Jan 2013 15:43:37 +0000
Subject: [PATCH] ata: sata_mv: fix sg_tbl_pool alignment

If the comment is to be believed, the alignment should be 16B, and the size 4K. The current code sets both to 4K. On some arm boards (kirkwood), this causes:

ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!

Set alignment to 16B to prevent exhausting the atomic_pool.

Signed-off-by Jason Cooper <[email protected]>
---
 drivers/ata/sata_mv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/ata/sata_mv.c +6 -3
index 68f4fb5..e2e5a8a 100644
@@ -148,6 +148,9 @@ enum {
 	 * CRPB needs alignment on a 256B boundary. Size == 256B
 	 * ePRD (SG) entries need alignment on a 16B boundary. Size == 16B
 	 */
+	MV_CRQB_Q_ALIGN		= 1024,
+	MV_CRPB_Q_ALIGN		= 256,
+	MV_SG_TBL_ALIGN		= 16,
 	MV_CRQB_Q_SZ		= (32 * MV_MAX_Q_DEPTH),
 	MV_CRPB_Q_SZ		= (8 * MV_MAX_Q_DEPTH),
 	MV_MAX_SG_CT		= 256,

@@ -3975,17 +3978,17 @@ done:
 static int mv_create_dma_pools(struct mv_host_priv *hpriv, struct device *dev)
 {
 	hpriv->crqb_pool   = dmam_pool_create("crqb_q", dev, MV_CRQB_Q_SZ,
-							     MV_CRQB_Q_SZ, 0);
+							MV_CRQB_Q_ALIGN, 0);
 	if (!hpriv->crqb_pool)
 		return -ENOMEM;
 
 	hpriv->crpb_pool   = dmam_pool_create("crpb_q", dev, MV_CRPB_Q_SZ,
-							     MV_CRPB_Q_SZ, 0);
+							MV_CRPB_Q_ALIGN, 0);
 	if (!hpriv->crpb_pool)
 		return -ENOMEM;
 
 	hpriv->sg_tbl_pool = dmam_pool_create("sg_tbl", dev, MV_SG_TBL_SZ,
-							     MV_SG_TBL_SZ, 0);
+							MV_SG_TBL_ALIGN, 0);
 	if (!hpriv->sg_tbl_pool)
 		return -ENOMEM;
Signature · 1 line
1.8.1.1
#32 lore raw

Re: [PATCH] ata: sata_mv: fix sg_tbl_pool alignment

Quoted 7 lines · Jason Cooper
Quoted 5 lines · Soeren Moch
Quoted 4 lines · Soeren Moch
Quoted 3 lines · Jason Cooper
Quoted 2 lines · Soeren Moch
Quoted 1 line · Jason Cooper

On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:

Quoted 7 lines
Quoted 4 lines

OK, I could trigger the error
ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!
only with em28xx sticks and sata, dib0700 sticks removed.

Did you test the reverse scenario? ie dib0700 with sata_mv and no em28xx.

Maybe I can test this next night.

Quoted 1 line

What kind of throughput are you pushing to the sata disk?

Close to nothing. In the last test I had the root filesystem running on the sata disk plus a few 10 megabytes per hour.

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines

What would be most helpful is if you could do a git bisect between v3.5.x (working) and the oldest version where you know it started failing (v3.7.1 or earlier if you know it).

I did not bisect it, but Marek mentioned earlier that commit e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced new code for dma allocations. This is probably the root cause for the new (mis-)behavior (due to my tests 3.6.0 is not working anymore).

Quoted 5 lines
Quoted 2 lines

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

Of the four drivers you listed, none are using dma. sata_mv is the only one.

usb_core is doing the actual DMA for the usb bridge drivers, I think.

Quoted 4 lines

If one is to believe the comments in sata_mv.c:~151, then the alignment is wrong for the sg_tbl_pool.

Could you please try the following patch?

OK, what should I test first, the setup from last night (em28xx, no dib0700) plus your patch, or the reverse setup (dib0700, no em28xx) without your patch, or my normal setting (all dvb sticks) plus your patch?

Regards,
Soeren

#33 lore raw

Re: [PATCH] ata: sata_mv: fix sg_tbl_pool alignment

Quoted 13 lines · Soeren Moch
Quoted 12 lines · Jason Cooper
Quoted 10 lines · Soeren Moch
Quoted 4 lines · Soeren Moch
Quoted 3 lines · Jason Cooper
Quoted 2 lines · Soeren Moch
Quoted 1 line · Jason Cooper

On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:

OK, I could trigger the error
ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!
only with em28xx sticks and sata, dib0700 sticks removed.

Quoted 4 lines
Quoted 2 lines

Did you test the reverse scenario? ie dib0700 with sata_mv and no em28xx.

Maybe I can test this next night.

Please do, this will tell us if it is in the USB drivers or lower (something in common).

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines

What would be most helpful is if you could do a git bisect between v3.5.x (working) and the oldest version where you know it started failing (v3.7.1 or earlier if you know it).

I did not bisect it, but Marek mentioned earlier that commit e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced new code for dma allocations. This is probably the root cause for the new (mis-)behavior (due to my tests 3.6.0 is not working anymore).

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

Of the four drivers you listed, none are using dma. sata_mv is the only one.

usb_core is doing the actual DMA for the usb bridge drivers, I think.

Yes, my mistake. I'd like to attribute that statement to pre-coffee rambling. :-)

Quoted 9 lines
Quoted 4 lines

If one is to believe the comments in sata_mv.c:~151, then the alignment is wrong for the sg_tbl_pool.

Could you please try the following patch?

OK, what should I test first, the setup from last night (em28xx, no dib0700) plus your patch, or the reverse setup (dib0700, no em28xx) without your patch, or my normal setting (all dvb sticks) plus your patch?

if testing time is limited, please do the test I outlined at the top of this email. I've been digging more into the dma code and while I think the patch is correct, I don't see where it would fix your problem (yet).

thx,

Jason.

#34 lore raw

Re: [PATCH] ata: sata_mv: fix sg_tbl_pool alignment

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines · Soeren Moch
Quoted 2 lines

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

Could you send the output of:

lsusb -v -d VEND:PROD

for the em28xx?

thx,

Jason.

#35 lore raw

Re: [PATCH] ata: sata_mv: fix sg_tbl_pool alignment

Quoted 6 lines · Jason Cooper
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines · Soeren Moch
Quoted 2 lines

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

Could you send the output of:

Quoted 8 lines

lsusb -v -d VEND:PROD

for the em28xx?

thx,

Jason.

Here is the lsusb output for both of my em28xx sticks.

Regards,
Soeren

Bus 001 Device 005: ID 0ccd:00b2 TerraTec Electronic GmbH Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00

   bDeviceClass            0 (Defined at Interface level)
   bDeviceSubClass         0
   bDeviceProtocol         0
   bMaxPacketSize0        64
   idVendor           0x0ccd TerraTec Electronic GmbH
   idProduct          0x00b2
   bcdDevice            1.00
   iManufacturer           3 TERRATEC
   iProduct                1 Cinergy HTC Stick
   iSerial                 2 123456789ABCD
   bNumConfigurations      1

Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 305
bNumInterfaces 1
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 1
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 2
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0ad0 2x 720 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 3
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0c00 2x 1024 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 4
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x1300 3x 768 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 5
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x1380 3x 896 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 6
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x13c0 3x 960 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 7
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x1400 3x 1024 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Device Qualifier (for other device speed):
bLength 10
bDescriptorType 6
bcdUSB 2.00

   bDeviceClass            0 (Defined at Interface level)
   bDeviceSubClass         0
   bDeviceProtocol         0

bMaxPacketSize0 64
bNumConfigurations 1
Device Status: 0x0000
(Bus Powered)

Bus 001 Device 009: ID 2304:0242 Pinnacle Systems, Inc. Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00

   bDeviceClass            0 (Defined at Interface level)
   bDeviceSubClass         0
   bDeviceProtocol         0
   bMaxPacketSize0        64
   idVendor           0x2304 Pinnacle Systems, Inc.
   idProduct          0x0242
   bcdDevice            1.00
   iManufacturer           1 Pinnacle Systems
   iProduct                2 PCTV 510e
   iSerial                 3 123456789012
   bNumConfigurations      1

Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 305
bNumInterfaces 1
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 1
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 2
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0ad0 2x 720 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 3
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x0c00 2x 1024 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 4
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x1300 3x 768 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 5
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x1380 3x 896 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 6
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x13c0 3x 960 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 7
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol 255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0001 1x 1 bytes
bInterval 11
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x1400 3x 1024 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x00c4 1x 196 bytes
bInterval 4
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 1
Transfer Type Isochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x03ac 1x 940 bytes
bInterval 1
Device Qualifier (for other device speed):
bLength 10
bDescriptorType 6
bcdUSB 2.00

   bDeviceClass            0 (Defined at Interface level)
   bDeviceSubClass         0
   bDeviceProtocol         0

bMaxPacketSize0 64
bNumConfigurations 1
Device Status: 0x0000
(Bus Powered)

#36 lore raw

Re: [PATCH] ata: sata_mv: fix sg_tbl_pool alignment

In reply to Jason Cooper
Quoted 47 lines · Jason Cooper
Quoted 13 lines · Soeren Moch
Quoted 12 lines · Jason Cooper
Quoted 10 lines · Soeren Moch
Quoted 4 lines · Soeren Moch
Quoted 3 lines · Jason Cooper
Quoted 2 lines · Soeren Moch
Quoted 1 line · Jason Cooper

On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:

OK, I could trigger the error
ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!
only with em28xx sticks and sata, dib0700 sticks removed.

Quoted 4 lines
Quoted 2 lines

Did you test the reverse scenario? ie dib0700 with sata_mv and no em28xx.

Maybe I can test this next night.

Please do, this will tell us if it is in the USB drivers or lower (something in common).

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines

What would be most helpful is if you could do a git bisect between v3.5.x (working) and the oldest version where you know it started failing (v3.7.1 or earlier if you know it).

I did not bisect it, but Marek mentioned earlier that commit e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced new code for dma allocations. This is probably the root cause for the new (mis-)behavior (due to my tests 3.6.0 is not working anymore).

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

Of the four drivers you listed, none are using dma. sata_mv is the only one.

usb_core is doing the actual DMA for the usb bridge drivers, I think.

Yes, my mistake. I'd like to attribute that statement to pre-coffee rambling. :-)

Quoted 5 lines
Quoted 4 lines

If one is to believe the comments in sata_mv.c:~151, then the alignment is wrong for the sg_tbl_pool.

Could you please try the following patch?

Quoted 8 lines
Quoted 4 lines

OK, what should I test first, the setup from last night (em28xx, no dib0700) plus your patch, or the reverse setup (dib0700, no em28xx) without your patch, or my normal setting (all dvb sticks) plus your patch?

if testing time is limited, please do the test I outlined at the top of this email. I've been digging more into the dma code and while I think the patch is correct, I don't see where it would fix your problem (yet).

Unfortunately test time is limited, and the test has to run about 10 hours to trigger the error.

I also think that sata is not causing the problem.
Maybe your patch even goes in the wrong direction. Perhaps the dma memory pool is not too small, there might be enough memory available, but it is too much fragmented to satisfy larger block allocations. With different drivers allocating totally different block sizes aligned to bytes or words, perhaps we end up with lots of very small free blocks in the dma pool after several hours of runtime?
So maybe it would help to align all allocations in the dma pool to 256B?

Regards,
Soeren

#37 lore raw

Re: [PATCH] ata: sata_mv: fix sg_tbl_pool alignment

In reply to Jason Cooper
Quoted 14 lines · Jason Cooper
Quoted 13 lines · Soeren Moch
Quoted 12 lines · Jason Cooper
Quoted 10 lines · Soeren Moch
Quoted 4 lines · Soeren Moch
Quoted 3 lines · Jason Cooper
Quoted 2 lines · Soeren Moch
Quoted 1 line · Jason Cooper

On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:

OK, I could trigger the error
ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!
only with em28xx sticks and sata, dib0700 sticks removed.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Did you test the reverse scenario? ie dib0700 with sata_mv and no em28xx.

Maybe I can test this next night.

Please do, this will tell us if it is in the USB drivers or lower (something in common).

Until now there is no error with dib0700 + sata, without em28xx.

But to be sure that there is absolutely no problem with this setting we probably need additional testing hours.
BTW, these dib0700 sticks use usb bulk transfers (and maybe smaller dma buffers?).

Regards,
Soeren

#38 lore raw
In reply to Soeren Moch
Quoted 65 lines · Soeren Moch
Quoted 19 lines · Soeren Moch
Quoted 18 lines · Jason Cooper

Soeren,

Quoted 15 lines · Soeren Moch
Quoted 10 lines · Jason Cooper
Quoted 5 lines · Jason Cooper

If my understanding is correct, one of the drivers (most likely one) either asks for too small of a dma buffer, or is not properly deallocating blocks from the per-device pool. Either case leads to exhaustion, and falling back to the atomic pool. Which subsequently gets wiped out as well.

If my hunch is right, could you please try each of the three dvb drivers
in turn and see which one (or more than one) causes the error?

In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk demodulator, and dib0700 usb bridge plus dib7000p demod.

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines

I would bet for em28xx causing the error, but this is not thoroughly tested. Unfortunately testing with removed sticks is not easy, because this is a production system and disabling some services for the long time we need to trigger this error will certainly result in unhappy users.

OK, I could trigger the error
ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!
only with em28xx sticks and sata, dib0700 sticks removed.

Quoted 3 lines
Quoted 1 line

Just out of curiosity, what board is it?

The kirkwood board? A modified Guruplug Server Plus.

em28xx sticks: "TerraTec Cinergy HTC Stick HD" and "PCTV Quatro Stick" dib0700 sticks: "WinTV-NOVA-TD Stick"

Quoted 29 lines
Quoted 5 lines
Quoted 3 lines

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

DMA_API_DEBUG

OK, maybe I can try this.

Quoted 7 lines
Quoted 5 lines

In linux-3.5.x there is no such problem. Can we use all available memory
for dma buffers here on armv5 architectures, in contrast to newer kernels?

Were the loads exactly the same when you tested 3.5.x?

Exactly the same, yes.

Quoted 12 lines

I looked at the
changes from v3.5 to v3.7.1 for all four drivers you mentioned as well as sata_mv.

The biggest thing I see is that all of the media drivers got shuffled around into their own subdirectories after v3.5. 'git show -M 0c0d06c' shows it was a clean copy of all the files.

What would be most helpful is if you could do a git bisect between v3.5.x (working) and the oldest version where you know it started failing (v3.7.1 or earlier if you know it).

Quoted 7 lines
Quoted 4 lines

I did not bisect it, but Marek mentioned earlier that commit e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced new code for dma allocations. This is probably the root cause for the new (mis-)behavior (due to my tests 3.6.0 is not working anymore).

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

The em28xx sticks are using isochronous usb transfers. Is there a special handling for that?

Quoted 5 lines
Quoted 5 lines

I'm not very familiar with arm mm code, and from the patch itself I cannot understand what's different. Maybe CONFIG_CMA is default also for armv5 (not only v6) now? But I might be totally wrong here, maybe someone of the mm experts can explain the difference?

Regards,
Soeren

#39 lore raw
Quoted 7 lines · Soeren Moch
Quoted 6 lines · Soeren Moch
Quoted 4 lines · Soeren Moch

I did not bisect it, but Marek mentioned earlier that commit e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced new code for dma allocations. This is probably the root cause for the new (mis-)behavior (due to my tests 3.6.0 is not working anymore).

Quoted 5 lines
Quoted 2 lines

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

The em28xx sticks are using isochronous usb transfers. Is there a special handling for that?

I'm looking at that now. It looks like the em28xx wants (as a maximum) 655040 bytes (em28xx-core.c:1088). There are 5 transfer buffers, with 64 max packets and 2047 max packet size (runtime reported max & 0x7ff).

If it actually needs all of that, then the answer may be to just increase coherent_pool= when using that driver. I'll keep digging.

thx,

Jason.

#40 lore raw
Quoted 14 lines · Jason Cooper
Quoted 7 lines · Soeren Moch
Quoted 6 lines · Soeren Moch
Quoted 4 lines · Soeren Moch

I did not bisect it, but Marek mentioned earlier that commit e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced new code for dma allocations. This is probably the root cause for the new (mis-)behavior (due to my tests 3.6.0 is not working anymore).

Quoted 5 lines
Quoted 2 lines

I don't want to say that Mareks patch is wrong, probably it triggers a bug somewhere else! (in em28xx?)

The em28xx sticks are using isochronous usb transfers. Is there a special handling for that?

Quoted 6 lines

I'm looking at that now. It looks like the em28xx wants (as a maximum) 655040 bytes (em28xx-core.c:1088). There are 5 transfer buffers, with 64 max packets and 2047 max packet size (runtime reported max & 0x7ff).

If it actually needs all of that, then the answer may be to just increase coherent_pool= when using that driver. I'll keep digging.

I already tested with 4M coherent pool size and could not see significant improvement. Would it make sense to further increase the buffer size?

Regards,
Soeren

#41 lore raw
In reply to Soeren Moch
Quoted 7 lines · Soeren Moch
Quoted 4 lines
Quoted 2 lines

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

DMA_API_DEBUG

OK, maybe I can try this.

Quoted 1 line

Any success with this? It should at least tell you if there is a memory leak in one of the drivers.

Arnd

#42 lore raw
Quoted 11 lines · Arnd Bergmann
Quoted 7 lines · Soeren Moch
Quoted 4 lines
Quoted 2 lines

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

DMA_API_DEBUG

OK, maybe I can try this.

Quoted 1 line

Any success with this? It should at least tell you if there is a memory leak in one of the drivers.

Not yet, sorry. I have to do all the tests in my limited spare time. Can you tell me what to search for in the debug output?

Soeren

#43 lore raw
Quoted 7 lines · Soeren Moch
Quoted 6 lines · Arnd Bergmann
Quoted 5 lines · Soeren Moch
Quoted 4 lines
Quoted 2 lines

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

DMA_API_DEBUG

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

OK, maybe I can try this.

Quoted 1 line

Any success with this? It should at least tell you if there is a memory leak in one of the drivers.

Not yet, sorry. I have to do all the tests in my limited spare time. Can you tell me what to search for in the debug output?

Actually now that I've looked closer, you can't immediately see all the mappings as I thought.

But please try enabling DMA_API_DEBUG in combination with this one-line patch:

arch/arm/mm/dma-mapping.c +1 -0
index 6b2fb87..3df74ac 100644
@@ -497,6 +497,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
 		pr_err_once("ERROR: %u KiB atomic DMA coherent pool is too small!\n"
 			    "Please increase it with coherent_pool= kernel parameter!\n",
 			    (unsigned)pool->size / 1024);
+		debug_dma_dump_mappings(NULL);
 	}
 	spin_unlock_irqrestore(&pool->lock, flags);

That will show every single allocation that is currently active. This lets you see where all the memory went, and if there is a possible leak or excessive fragmentation.

Arnd

#44 lore raw
Quoted 39 lines · Arnd Bergmann
Quoted 7 lines · Soeren Moch
Quoted 6 lines · Arnd Bergmann
Quoted 5 lines · Soeren Moch
Quoted 4 lines
Quoted 2 lines

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

DMA_API_DEBUG

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

OK, maybe I can try this.

Quoted 1 line

Any success with this? It should at least tell you if there is a memory leak in one of the drivers.

Not yet, sorry. I have to do all the tests in my limited spare time. Can you tell me what to search for in the debug output?

Actually now that I've looked closer, you can't immediately see all the mappings as I thought.

But please try enabling DMA_API_DEBUG in combination with this one-line patch:

arch/arm/mm/dma-mapping.c +1 -0
index 6b2fb87..3df74ac 100644
@@ -497,6 +497,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page)
  		pr_err_once("ERROR: %u KiB atomic DMA coherent pool is too small!\n"
  			    "Please increase it with coherent_pool= kernel parameter!\n",
  			    (unsigned)pool->size / 1024);
+		debug_dma_dump_mappings(NULL);
  	}
  	spin_unlock_irqrestore(&pool->lock, flags);

That will show every single allocation that is currently active. This lets you see where all the memory went, and if there is a possible leak or excessive fragmentation.

Quoted 2 lines

Arnd

Please find attached a debug log generated with your patch.

I used the sata disk and two em28xx dvb sticks, no other usb devices, no ethernet cable connected, tuners on saa716x-based card not used.

What I can see in the log: a lot of coherent mappings from sata_mv and orion_ehci, a few from mv643xx_eth, no other coherent mappings. All coherent mappings are page aligned, some of them (from orion_ehci) are not really small (as claimed in __alloc_from_pool).

I don't believe in a memory leak. When I restart vdr (the application utilizing the dvb sticks) then there is enough dma memory available again.

Regards,
Soeren

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dma_debug.log
Type: text/x-log
Size: 215198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130119/365a3ec2/attachment-0001.bin>

maintainer
#45 lore raw
Quoted 5 lines

Please find attached a debug log generated with your patch.

I used the sata disk and two em28xx dvb sticks, no other usb devices, no ethernet cable connected, tuners on saa716x-based card not used.

Quoted 8 lines

What I can see in the log: a lot of coherent mappings from sata_mv and orion_ehci, a few from mv643xx_eth, no other coherent mappings. All coherent mappings are page aligned, some of them (from orion_ehci) are not really small (as claimed in __alloc_from_pool).

I don't believe in a memory leak. When I restart vdr (the application utilizing the dvb sticks) then there is enough dma memory available again.

Hi Soeren

We should be able to rule out a leak. Mount debugfg and then:

while [ /bin/true ] ; do cat /debug/dma-api/num_free_entries ; sleep 60 ; done

while you are capturing. See if the number goes down.

Andrew

#46 lore raw
Quoted 16 lines · Andrew Lunn
Quoted 5 lines

Please find attached a debug log generated with your patch.

I used the sata disk and two em28xx dvb sticks, no other usb devices, no ethernet cable connected, tuners on saa716x-based card not used.

Quoted 8 lines

What I can see in the log: a lot of coherent mappings from sata_mv and orion_ehci, a few from mv643xx_eth, no other coherent mappings. All coherent mappings are page aligned, some of them (from orion_ehci) are not really small (as claimed in __alloc_from_pool).

I don't believe in a memory leak. When I restart vdr (the application utilizing the dvb sticks) then there is enough dma memory available again.

Hi Soeren

Quoted 7 lines

We should be able to rule out a leak. Mount debugfg and then:

while [ /bin/true ] ; do cat /debug/dma-api/num_free_entries ; sleep 60 ; done

while you are capturing. See if the number goes down.

Andrew

Now I built a kernel with debugfs enabled.
It is not clear to me what I can see from the dma-api/num_free_entries output. After reboot (vdr running) I see decreasing numbers (3453 3452 3445 3430...), min_free_entries is lower (3390). Sometimes the output is constant for several minutes ( 3396 3396 3396 3396 3396,...)

Soeren

maintainer
#47 lore raw
Quoted 23 lines · Soeren Moch
Quoted 22 lines · Andrew Lunn
Quoted 5 lines

Please find attached a debug log generated with your patch.

I used the sata disk and two em28xx dvb sticks, no other usb devices, no ethernet cable connected, tuners on saa716x-based card not used.

Quoted 8 lines

What I can see in the log: a lot of coherent mappings from sata_mv and orion_ehci, a few from mv643xx_eth, no other coherent mappings. All coherent mappings are page aligned, some of them (from orion_ehci) are not really small (as claimed in __alloc_from_pool).

I don't believe in a memory leak. When I restart vdr (the application utilizing the dvb sticks) then there is enough dma memory available again.

Hi Soeren

We should be able to rule out a leak. Mount debugfg and then:

while [ /bin/true ] ; do cat /debug/dma-api/num_free_entries ; sleep 60 ; done

while you are capturing. See if the number goes down.

Quoted 8 lines
Quoted 1 line

Andrew

Now I built a kernel with debugfs enabled.
It is not clear to me what I can see from the dma-api/num_free_entries output. After reboot (vdr running) I see decreasing numbers (3453 3452 3445 3430...), min_free_entries is lower (3390). Sometimes the output is constant for several minutes ( 3396 3396 3396 3396 3396,...)

We are interesting in the long term behavior. Does it gradually go down? Or is it stable? If it goes down over time, its clearly a leak somewhere.

Andrew

#48 lore raw
Quoted 33 lines · Andrew Lunn
Quoted 23 lines · Soeren Moch
Quoted 22 lines · Andrew Lunn
Quoted 5 lines

Please find attached a debug log generated with your patch.

I used the sata disk and two em28xx dvb sticks, no other usb devices, no ethernet cable connected, tuners on saa716x-based card not used.

Quoted 8 lines

What I can see in the log: a lot of coherent mappings from sata_mv and orion_ehci, a few from mv643xx_eth, no other coherent mappings. All coherent mappings are page aligned, some of them (from orion_ehci) are not really small (as claimed in __alloc_from_pool).

I don't believe in a memory leak. When I restart vdr (the application utilizing the dvb sticks) then there is enough dma memory available again.

Hi Soeren

We should be able to rule out a leak. Mount debugfg and then:

while [ /bin/true ] ; do cat /debug/dma-api/num_free_entries ; sleep 60 ; done

while you are capturing. See if the number goes down.

Quoted 8 lines
Quoted 1 line

Andrew

Now I built a kernel with debugfs enabled.
It is not clear to me what I can see from the dma-api/num_free_entries output. After reboot (vdr running) I see decreasing numbers (3453 3452 3445 3430...), min_free_entries is lower (3390). Sometimes the output is constant for several minutes ( 3396 3396 3396 3396 3396,...)

Quoted 4 lines

We are interesting in the long term behavior. Does it gradually go down? Or is it stable? If it goes down over time, its clearly a leak somewhere.

Now (in the last hour) stable, occasionally lower numbers:
3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3365 3396 3394 3396 3396 3396 3396 3373 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3353 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3394 3396 3396 3396 3396 3396 3396 3396

Before the last pool exhaustion going down:
3395 3395 3389 3379 3379 3374 3367 3360 3352 3343 3343 3343 3342 3336 3332 3324 3318 3314 3310 3307 3305 3299 3290 3283 3279 3272 3266 3265 3247 3247 3247 3242 3236 3236

Soeren

#49 lore raw
Quoted 49 lines · Soeren Moch
Quoted 34 lines · Andrew Lunn
Quoted 24 lines · Soeren Moch
Quoted 23 lines · Andrew Lunn
Quoted 5 lines

Please find attached a debug log generated with your patch.

I used the sata disk and two em28xx dvb sticks, no other usb devices, no ethernet cable connected, tuners on saa716x-based card not used.

Quoted 8 lines

What I can see in the log: a lot of coherent mappings from sata_mv and orion_ehci, a few from mv643xx_eth, no other coherent mappings. All coherent mappings are page aligned, some of them (from orion_ehci) are not really small (as claimed in __alloc_from_pool).

I don't believe in a memory leak. When I restart vdr (the application utilizing the dvb sticks) then there is enough dma memory available again.

Hi Soeren

We should be able to rule out a leak. Mount debugfg and then:

while [ /bin/true ] ; do cat /debug/dma-api/num_free_entries ; sleep 60 ; done

while you are capturing. See if the number goes down.

Quoted 8 lines
Quoted 1 line

Andrew

Now I built a kernel with debugfs enabled.
It is not clear to me what I can see from the dma-api/num_free_entries output. After reboot (vdr running) I see decreasing numbers (3453 3452 3445 3430...), min_free_entries is lower (3390). Sometimes the output is constant for several minutes ( 3396 3396 3396 3396 3396,...)

Quoted 4 lines

We are interesting in the long term behavior. Does it gradually go down? Or is it stable? If it goes down over time, its clearly a leak somewhere.

Now (in the last hour) stable, occasionally lower numbers:
3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3365 3396 3394 3396 3396 3396 3396 3373 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3353 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3394 3396 3396 3396 3396 3396 3396 3396

Quoted 5 lines

Before the last pool exhaustion going down:
3395 3395 3389 3379 3379 3374 3367 3360 3352 3343 3343 3343 3342 3336 3332 3324 3318 3314 3310 3307 3305 3299 3290 3283 3279 3272 3266 3265 3247 3247 3247 3242 3236 3236

Here I stopped vdr (and so closed all dvb_demux devices), the number was remaining the same 3236, even after restart of vdr (and restart of streaming).

Quoted 1 line

Soeren

maintainer
#50 lore raw
Quoted 11 lines
Quoted 11 lines
Quoted 1 line

Now (in the last hour) stable, occasionally lower numbers:
3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3365 3396 3394 3396 3396 3396 3396 3373 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3353 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3394 3396 3396 3396 3396 3396 3396 3396

Quoted 8 lines
Quoted 5 lines

Before the last pool exhaustion going down:
3395 3395 3389 3379 3379 3374 3367 3360 3352 3343 3343 3343 3342 3336 3332 3324 3318 3314 3310 3307 3305 3299 3290 3283 3279 3272 3266 3265 3247 3247 3247 3242 3236 3236

Here I stopped vdr (and so closed all dvb_demux devices), the number was remaining the same 3236, even after restart of vdr (and restart of streaming).

So it does suggest a leak. Probably somewhere on an error path, e.g. its lost video sync.

Andrew

#51 lore raw
Quoted 16 lines · Andrew Lunn
Quoted 16 lines
Quoted 16 lines
Quoted 1 line

Now (in the last hour) stable, occasionally lower numbers:
3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3365 3396 3394 3396 3396 3396 3396 3373 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3353 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3394 3396 3396 3396 3396 3396 3396 3396

Before the last pool exhaustion going down:
3395 3395 3389 3379 3379 3374 3367 3360 3352 3343 3343 3343 3342 3336 3332 3324 3318 3314 3310 3307 3305 3299 3290 3283 3279 3272 3266 3265 3247 3247 3247 3242 3236 3236

Quoted 7 lines
Quoted 3 lines

Here I stopped vdr (and so closed all dvb_demux devices), the number was remaining the same 3236, even after restart of vdr (and restart of streaming).

So it does suggest a leak. Probably somewhere on an error path, e.g. its lost video sync.

Now I activated the debug messages in em28xx. From the messages I see no correlation of the pool exhaustion and lost sync. Also I cannot see any error messages from the em28xx driver.
I see a lot of init_isoc/stop_urbs (maybe EPG scan?) without draining the coherent pool (checked with 'cat /debug/dma-api/num_free_entries', which gave stable numbers), but after half an hour there are only init_isoc messages without corresponding stop_urbs messages and num_free_entries decreased until coherent pool exhaustion.

Any idea where the memory leak is? What is allocating coherent buffers for orion-ehci?

Soeren

Jan 28 20:46:03 guruvdr kernel: em28xx #0/2-dvb: Using 5 buffers each 
with 64 x 940 bytes
Jan 28 20:46:03 guruvdr kernel: em28xx #0 em28xx_init_isoc :em28xx: 
called em28xx_init_isoc in mode 2
Jan 28 20:46:03 guruvdr kernel: em28xx #1/2-dvb: Using 5 buffers each 
with 64 x 940 bytes
Jan 28 20:46:03 guruvdr kernel: em28xx #1 em28xx_init_isoc :em28xx: 
called em28xx_init_isoc in mode 2
Jan 28 20:46:23 guruvdr kernel: em28xx #0 em28xx_stop_urbs :em28xx: 
called em28xx_stop_urbs
Jan 28 20:46:23 guruvdr kernel: em28xx #1 em28xx_stop_urbs :em28xx: 
called em28xx_stop_urbs
Jan 28 20:46:24 guruvdr kernel: em28xx #0/2-dvb: Using 5 buffers each 
with 64 x 940 bytes
Jan 28 20:46:24 guruvdr kernel: em28xx #0 em28xx_init_isoc :em28xx: 
called em28xx_init_isoc in mode 2
Jan 28 20:46:24 guruvdr kernel: em28xx #1/2-dvb: Using 5 buffers each 
with 64 x 940 bytes
Jan 28 20:46:24 guruvdr kernel: em28xx #1 em28xx_init_isoc :em28xx: 
called em28xx_init_isoc in mode 2
Jan 28 20:46:44 guruvdr kernel: em28xx #1 em28xx_stop_urbs :em28xx: 
called em28xx_stop_urbs
Jan 28 20:46:44 guruvdr kernel: em28xx #0 em28xx_stop_urbs :em28xx: 
called em28xx_stop_urbs
Jan 28 20:46:45 guruvdr kernel: em28xx #1/2-dvb: Using 5 buffers each 
with 64 x 940 bytes
Jan 28 20:46:45 guruvdr kernel: em28xx #1 em28xx_init_isoc :em28xx: 
called em28xx_init_isoc in mode 2
Jan 28 20:46:45 guruvdr kernel: em28xx #0/2-dvb: Using 5 buffers each 
with 64 x 940 bytes
Jan 28 20:46:45 guruvdr kernel: em28xx #0 em28xx_init_isoc :em28xx: 
called em28xx_init_isoc in mode 2
Jan 28 20:54:33 guruvdr kernel: ERROR: 1024 KiB atomic DMA coherent pool 
is too small!
Jan 28 20:54:33 guruvdr kernel: Please increase it with coherent_pool=

kernel parameter!

#52 lore raw
Quoted 35 lines · Soeren Moch
Quoted 16 lines · Andrew Lunn
Quoted 16 lines
Quoted 16 lines
Quoted 1 line

Now (in the last hour) stable, occasionally lower numbers:
3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3365 3396 3394 3396 3396 3396 3396 3373 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3353 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3396 3394 3396 3396 3396 3396 3396 3396 3396

Before the last pool exhaustion going down:
3395 3395 3389 3379 3379 3374 3367 3360 3352 3343 3343 3343 3342 3336 3332 3324 3318 3314 3310 3307 3305 3299 3290 3283 3279 3272 3266 3265 3247 3247 3247 3242 3236 3236

Quoted 7 lines
Quoted 3 lines

Here I stopped vdr (and so closed all dvb_demux devices), the number was remaining the same 3236, even after restart of vdr (and restart of streaming).

So it does suggest a leak. Probably somewhere on an error path, e.g. its lost video sync.

Now I activated the debug messages in em28xx. From the messages I see no correlation of the pool exhaustion and lost sync. Also I cannot see any error messages from the em28xx driver.
I see a lot of init_isoc/stop_urbs (maybe EPG scan?) without draining the coherent pool (checked with 'cat /debug/dma-api/num_free_entries', which gave stable numbers), but after half an hour there are only init_isoc messages without corresponding stop_urbs messages and num_free_entries decreased until coherent pool exhaustion.

Quoted 2 lines

Any idea where the memory leak is? What is allocating coherent buffers for orion-ehci?

Keeping in mind that I am completely unfamiliar with usb dvb, my best guess is that the problem is in em28xx-core.c:1131

According to your log messages, it is in mode 2, which is EM28XX_DIGITAL_MODE.

There seem to be good hints in

86d38d1e [media] em28xx: pre-allocate DVB isoc transfer buffers

I added the relevant parties to the To:...

For Gianluca and Mauro, the whole thread may be found at:

http://markmail.org/message/wm4wlgzoudixd4so#query:+page:1+mid:o7phz7cosmwpcsrz+state:results

thx,

Jason.

Quoted 41 lines

Soeren

Jan 28 20:46:03 guruvdr kernel: em28xx #0/2-dvb: Using 5 buffers
each with 64 x 940 bytes
Jan 28 20:46:03 guruvdr kernel: em28xx #0 em28xx_init_isoc :em28xx:
called em28xx_init_isoc in mode 2
Jan 28 20:46:03 guruvdr kernel: em28xx #1/2-dvb: Using 5 buffers
each with 64 x 940 bytes
Jan 28 20:46:03 guruvdr kernel: em28xx #1 em28xx_init_isoc :em28xx:
called em28xx_init_isoc in mode 2
Jan 28 20:46:23 guruvdr kernel: em28xx #0 em28xx_stop_urbs :em28xx:
called em28xx_stop_urbs
Jan 28 20:46:23 guruvdr kernel: em28xx #1 em28xx_stop_urbs :em28xx:
called em28xx_stop_urbs
Jan 28 20:46:24 guruvdr kernel: em28xx #0/2-dvb: Using 5 buffers
each with 64 x 940 bytes
Jan 28 20:46:24 guruvdr kernel: em28xx #0 em28xx_init_isoc :em28xx:
called em28xx_init_isoc in mode 2
Jan 28 20:46:24 guruvdr kernel: em28xx #1/2-dvb: Using 5 buffers
each with 64 x 940 bytes
Jan 28 20:46:24 guruvdr kernel: em28xx #1 em28xx_init_isoc :em28xx:
called em28xx_init_isoc in mode 2
Jan 28 20:46:44 guruvdr kernel: em28xx #1 em28xx_stop_urbs :em28xx:
called em28xx_stop_urbs
Jan 28 20:46:44 guruvdr kernel: em28xx #0 em28xx_stop_urbs :em28xx:
called em28xx_stop_urbs
Jan 28 20:46:45 guruvdr kernel: em28xx #1/2-dvb: Using 5 buffers
each with 64 x 940 bytes
Jan 28 20:46:45 guruvdr kernel: em28xx #1 em28xx_init_isoc :em28xx:
called em28xx_init_isoc in mode 2
Jan 28 20:46:45 guruvdr kernel: em28xx #0/2-dvb: Using 5 buffers
each with 64 x 940 bytes
Jan 28 20:46:45 guruvdr kernel: em28xx #0 em28xx_init_isoc :em28xx:
called em28xx_init_isoc in mode 2
Jan 28 20:54:33 guruvdr kernel: ERROR: 1024 KiB atomic DMA coherent
pool is too small!
Jan 28 20:54:33 guruvdr kernel: Please increase it with

coherent_pool= kernel parameter!

maintainer
#53 lore raw
In reply to Soeren Moch
Quoted 9 lines

Now I activated the debug messages in em28xx. From the messages I see no correlation of the pool exhaustion and lost sync. Also I cannot see any error messages from the em28xx driver.
I see a lot of init_isoc/stop_urbs (maybe EPG scan?) without draining the coherent pool (checked with 'cat /debug/dma-api/num_free_entries', which gave stable numbers), but after half an hour there are only init_isoc messages without corresponding stop_urbs messages and num_free_entries decreased until coherent pool exhaustion.

Hi Soeren

em28xx_stop_urbs() is only called by em28xx_stop_streaming().

em28xx_stop_streaming() is only called by em28xx_stop_feed() when 0 == dvb->nfeeds.

em28xx_stop_feed()and em28xx_start_feed() look O.K, dvb->nfeeds is protected by a mutex etc.

Now, em28xx_init_isoc() is also called by buffer_prepare(). This uses em28xx_alloc_isoc() to do the actual allocation, and that function sets up the urb such that on completion the function em28xx_irq_callback() is called.

It looks like there might be issues here:

Once the data has been copied out, it resubmits the urb:

       urb->status = usb_submit_urb(urb, GFP_ATOMIC);
        if (urb->status) {
                em28xx_isocdbg("urb resubmit failed (error=%i)\n",
                               urb->status);
        }

However, if the ubs_submit_urb fails, it looks like the urb is lost.

If you look at other code submitting urbs you have this pattern:

               rc = usb_submit_urb(isoc_bufs->urb[i], GFP_ATOMIC);
                if (rc) {
                        em28xx_err("submit of urb %i failed (error=%i)\n", i,
                                   rc);
                        em28xx_uninit_isoc(dev, mode);
                        return rc;
                }

Do you have your build such that you would see "urb resubmit failed" in your logs? Are there any?

Andrew

#54 lore raw
Quoted 47 lines · Andrew Lunn
Quoted 9 lines

Now I activated the debug messages in em28xx. From the messages I see no correlation of the pool exhaustion and lost sync. Also I cannot see any error messages from the em28xx driver.
I see a lot of init_isoc/stop_urbs (maybe EPG scan?) without draining the coherent pool (checked with 'cat /debug/dma-api/num_free_entries', which gave stable numbers), but after half an hour there are only init_isoc messages without corresponding stop_urbs messages and num_free_entries decreased until coherent pool exhaustion.

Hi Soeren

em28xx_stop_urbs() is only called by em28xx_stop_streaming().

em28xx_stop_streaming() is only called by em28xx_stop_feed() when 0 == dvb->nfeeds.

em28xx_stop_feed()and em28xx_start_feed() look O.K, dvb->nfeeds is protected by a mutex etc.

Now, em28xx_init_isoc() is also called by buffer_prepare(). This uses em28xx_alloc_isoc() to do the actual allocation, and that function sets up the urb such that on completion the function em28xx_irq_callback() is called.

It looks like there might be issues here:

Once the data has been copied out, it resubmits the urb:

        urb->status = usb_submit_urb(urb, GFP_ATOMIC);
         if (urb->status) {
                 em28xx_isocdbg("urb resubmit failed (error=%i)\n",
                                urb->status);
         }

However, if the ubs_submit_urb fails, it looks like the urb is lost.

If you look at other code submitting urbs you have this pattern:

                rc = usb_submit_urb(isoc_bufs->urb[i], GFP_ATOMIC);
                 if (rc) {
                         em28xx_err("submit of urb %i failed (error=%i)\n", i,
                                    rc);
                         em28xx_uninit_isoc(dev, mode);
                         return rc;
                 }
Quoted 2 lines

Do you have your build such that you would see "urb resubmit failed" in your logs? Are there any?

I only had "urb resubmit failed" messages _after_ the coherent pool exhaustion. So I guess something below the usb_submit_urb call is allocating (too much) memory, sometimes. Or can dvb_demux allocate memory and blame orion-ehci for it?

Soeren

#55 lore raw
In reply to Soeren Moch
Quoted 4 lines · Soeren Moch

What I can see in the log: a lot of coherent mappings from sata_mv and orion_ehci, a few from mv643xx_eth, no other coherent mappings. All coherent mappings are page aligned, some of them (from orion_ehci) are not really small (as claimed in __alloc_from_pool).

Right. Unfortunately, the output does not show which of the mappings are atomic, so we still need to look through those that can be atomic to understand what's going on. There are a few megabytes of coherent mappings in total according to the output, but it seems that a significant portion of them is atomic, which is a bit unexpected.

Quoted 3 lines

I don't believe in a memory leak. When I restart vdr (the application utilizing the dvb sticks) then there is enough dma memory available again.

I found at least one source line that incorrectly uses an atomic allocation, in ehci_mem_init():

                dma_alloc_coherent (ehci_to_hcd(ehci)->self.controller,
                        ehci->periodic_size * sizeof(__le32),
                        &ehci->periodic_dma, 0);

The last argument is the GFP_ flag, which should never be zero, as that is implicit !wait. This function is called only once, so it is not the actual culprit, but there could be other instances where we accidentally allocate something as GFP_ATOMIC.

The total number of allocations I found for each type are

sata_mv: 66 pages (270336 bytes)
mv643xx_eth: 4 pages == (16384 bytes)
orion_ehci: 154 pages (630784 bytes)
orion_ehci (atomic): 256 pages (1048576 bytes)

from the distribution of the numbers, it seems that there is exactly 1 MB of data allocated between bus addresses 0x1f90000 and 0x1f9ffff, allocated in individual pages. This matches the size of your pool, so it's definitely something coming from USB, and no single other allocation, but it does not directly point to a specific line of code.

One thing I found was that the ARM dma-mapping code seems buggy in the way that it does a bitwise and between the gfp mask and GFP_ATOMIC, which does not work because GFP_ATOMIC is defined by the absence of __GFP_WAIT.

I believe we need the patch below, but it is not clear to me if that issue is related to your problem or now.

arch/arm/mm/dma-mapping.c +2 -2
index 6b2fb87..c57975f 100644
@@ -640,7 +641,7 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
 
 	if (is_coherent || nommu())
 		addr = __alloc_simple_buffer(dev, size, gfp, &page);
-	else if (gfp & GFP_ATOMIC)
+	else if (!(gfp & __GFP_WAIT))
 		addr = __alloc_from_pool(size, &page);
 	else if (!IS_ENABLED(CONFIG_CMA))
 		addr = __alloc_remap_buffer(dev, size, gfp, prot, &page, caller);

@@ -1272,7 +1273,7 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
 	*handle = DMA_ERROR_CODE;
 	size = PAGE_ALIGN(size);
 
-	if (gfp & GFP_ATOMIC)
+	if (!(gfp & __GFP_WAIT))
 		return __iommu_alloc_atomic(dev, size, handle);
 
 	pages = __iommu_alloc_buffer(dev, size, gfp, attrs);

8<-------

There is one more code path I could find, which is usb_submit_urb() => usb_hcd_submit_urb => ehci_urb_enqueue() => submit_async() => qh_append_tds() => qh_make(GFP_ATOMIC) => ehci_qh_alloc() => dma_pool_alloc() => pool_alloc_page() => dma_alloc_coherent()

So even for a GFP_KERNEL passed into usb_submit_urb, the ehci driver causes the low-level allocation to be GFP_ATOMIC, because qh_append_tds() is called under a spinlock. If we have hundreds of URBs in flight, that will exhaust the pool rather quickly.

Arnd

#56 lore raw
Quoted 19 lines · Arnd Bergmann

I found at least one source line that incorrectly uses an atomic allocation, in ehci_mem_init():

                 dma_alloc_coherent (ehci_to_hcd(ehci)->self.controller,
                         ehci->periodic_size * sizeof(__le32),
                         &ehci->periodic_dma, 0);

The last argument is the GFP_ flag, which should never be zero, as that is implicit !wait. This function is called only once, so it is not the actual culprit, but there could be other instances where we accidentally allocate something as GFP_ATOMIC.

The total number of allocations I found for each type are

sata_mv: 66 pages (270336 bytes)
mv643xx_eth: 4 pages == (16384 bytes)
orion_ehci: 154 pages (630784 bytes)
orion_ehci (atomic): 256 pages (1048576 bytes)

Quoted 5 lines

from the distribution of the numbers, it seems that there is exactly 1 MB of data allocated between bus addresses 0x1f90000 and 0x1f9ffff, allocated in individual pages. This matches the size of your pool, so it's definitely something coming from USB, and no single other allocation, but it does not directly point to a specific line of code.

Very interesting, so this is no fragmentation problem nor something caused by sata or ethernet.

Quoted 6 lines

One thing I found was that the ARM dma-mapping code seems buggy in the way that it does a bitwise and between the gfp mask and GFP_ATOMIC, which does not work because GFP_ATOMIC is defined by the absence of __GFP_WAIT.

I believe we need the patch below, but it is not clear to me if that issue is related to your problem or now.

Out of curiosity I checked include/linux/gfp.h. GFP_ATOMIC is defined as __GFP_HIGH (which means 'use emergency pool', and no wait), so this patch should not make any difference for "normal" (GPF_ATOMIC / GFP_KERNEL) allocations, only for gfp_flags accidentally set to zero. So, can a new test with this patch help to debug the pool exhaustion?

Quoted 29 lines
arch/arm/mm/dma-mapping.c +2 -2
index 6b2fb87..c57975f 100644
@@ -640,7 +641,7 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
 
  	if (is_coherent || nommu())
  		addr = __alloc_simple_buffer(dev, size, gfp, &page);
-	else if (gfp & GFP_ATOMIC)
+	else if (!(gfp & __GFP_WAIT))
  		addr = __alloc_from_pool(size, &page);
  	else if (!IS_ENABLED(CONFIG_CMA))
  		addr = __alloc_remap_buffer(dev, size, gfp, prot, &page, caller);

@@ -1272,7 +1273,7 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size,
  	*handle = DMA_ERROR_CODE;
  	size = PAGE_ALIGN(size);
 
-	if (gfp & GFP_ATOMIC)
+	if (!(gfp & __GFP_WAIT))
  		return __iommu_alloc_atomic(dev, size, handle);
 
  	pages = __iommu_alloc_buffer(dev, size, gfp, attrs);

8<-------

There is one more code path I could find, which is usb_submit_urb() => usb_hcd_submit_urb => ehci_urb_enqueue() => submit_async() => qh_append_tds() => qh_make(GFP_ATOMIC) => ehci_qh_alloc() => dma_pool_alloc() => pool_alloc_page() => dma_alloc_coherent()

Quoted 5 lines

So even for a GFP_KERNEL passed into usb_submit_urb, the ehci driver causes the low-level allocation to be GFP_ATOMIC, because qh_append_tds() is called under a spinlock. If we have hundreds of URBs in flight, that will exhaust the pool rather quickly.

Maybe there are hundreds of URBs in flight in my application, I have no idea how to check this. It seems to me that bad reception conditions (lost lock / regained lock messages for some dvb channels) accelerate the buffer exhaustion. But even with a 4MB coherent pool I see the error. Is there any chance to fix this in the usb or dvb subsystem (or wherever)? Should I try to further increase the pool size, or what else can I do besides using an older kernel?

Soeren

#57 lore raw
Quoted 8 lines · Soeren Moch
Quoted 5 lines · Arnd Bergmann

from the distribution of the numbers, it seems that there is exactly 1 MB of data allocated between bus addresses 0x1f90000 and 0x1f9ffff, allocated in individual pages. This matches the size of your pool, so it's definitely something coming from USB, and no single other allocation, but it does not directly point to a specific line of code.

Very interesting, so this is no fragmentation problem nor something caused by sata or ethernet.

Right.

Quoted 10 lines
Quoted 6 lines

One thing I found was that the ARM dma-mapping code seems buggy in the way that it does a bitwise and between the gfp mask and GFP_ATOMIC, which does not work because GFP_ATOMIC is defined by the absence of __GFP_WAIT.

I believe we need the patch below, but it is not clear to me if that issue is related to your problem or now.

Out of curiosity I checked include/linux/gfp.h. GFP_ATOMIC is defined as __GFP_HIGH (which means 'use emergency pool', and no wait), so this patch should not make any difference for "normal" (GPF_ATOMIC / GFP_KERNEL) allocations, only for gfp_flags accidentally set to zero.

Yes, or one of the rare cases where someone intentionally does something like (GFP_ATOMIC & !__GFP_HIGH) or (GFP_KERNEL || __GFP_HIGH), which are both wrong.

Quoted 1 line

So, can a new test with this patch help to debug the pool exhaustion?

Yes, but I would not expect this to change much. It's a bug, but not likely the one you are hitting.

Quoted 7 lines
Quoted 5 lines

So even for a GFP_KERNEL passed into usb_submit_urb, the ehci driver causes the low-level allocation to be GFP_ATOMIC, because qh_append_tds() is called under a spinlock. If we have hundreds of URBs in flight, that will exhaust the pool rather quickly.

Maybe there are hundreds of URBs in flight in my application, I have no idea how to check this.

I don't know a lot about USB, but I always assumed that this was not a normal condition and that there are only a couple of URBs per endpoint used at a time. Maybe Greg or someone else with a USB background can shed some light on this.

Quoted 6 lines

It seems to me that bad reception conditions
(lost lock / regained lock messages for some dvb channels) accelerate the buffer exhaustion. But even with a 4MB coherent pool I see the error. Is there any chance to fix this in the usb or dvb subsystem (or wherever)? Should I try to further increase the pool size, or what else can I do besides using an older kernel?

There are two things that I think can be done if hundreds of URBs is indeed the normal working condition for this driver:

* change the locking in your driver so it can actually call usb_submit_urb using GFP_KERNEL rather than GFP_ATOMIC
* after that is done, rework the ehci_hcd driver so it can do the allocation inside of the submit_urb path to use the mem_flags rather than unconditional GFP_ATOMIC.

Note that the problem you are seeing does not just exist in the case of the atomic coherent pool getting exhausted, but also on any platform that runs into an out-of-memory condition.

Arnd

#58 lore raw
Quoted 37 lines · Arnd Bergmann
Quoted 8 lines · Soeren Moch
Quoted 5 lines · Arnd Bergmann

from the distribution of the numbers, it seems that there is exactly 1 MB of data allocated between bus addresses 0x1f90000 and 0x1f9ffff, allocated in individual pages. This matches the size of your pool, so it's definitely something coming from USB, and no single other allocation, but it does not directly point to a specific line of code.

Very interesting, so this is no fragmentation problem nor something caused by sata or ethernet.

Right.

Quoted 10 lines
Quoted 6 lines

One thing I found was that the ARM dma-mapping code seems buggy in the way that it does a bitwise and between the gfp mask and GFP_ATOMIC, which does not work because GFP_ATOMIC is defined by the absence of __GFP_WAIT.

I believe we need the patch below, but it is not clear to me if that issue is related to your problem or now.

Out of curiosity I checked include/linux/gfp.h. GFP_ATOMIC is defined as __GFP_HIGH (which means 'use emergency pool', and no wait), so this patch should not make any difference for "normal" (GPF_ATOMIC / GFP_KERNEL) allocations, only for gfp_flags accidentally set to zero.

Yes, or one of the rare cases where someone intentionally does something like (GFP_ATOMIC & !__GFP_HIGH) or (GFP_KERNEL || __GFP_HIGH), which are both wrong.

Quoted 1 line

So, can a new test with this patch help to debug the pool exhaustion?

Yes, but I would not expect this to change much. It's a bug, but not likely the one you are hitting.

Quoted 5 lines
Quoted 5 lines

So even for a GFP_KERNEL passed into usb_submit_urb, the ehci driver causes the low-level allocation to be GFP_ATOMIC, because qh_append_tds() is called under a spinlock. If we have hundreds of URBs in flight, that will exhaust the pool rather quickly.

Quoted 7 lines
Quoted 2 lines

Maybe there are hundreds of URBs in flight in my application, I have no idea how to check this.

I don't know a lot about USB, but I always assumed that this was not a normal condition and that there are only a couple of URBs per endpoint used at a time. Maybe Greg or someone else with a USB background can shed some light on this.

There's no restriction on how many URBs a driver can have outstanding at once, and if you have a system with a lot of USB devices running at the same time, there could be lots of URBs in flight depending on the number of host controllers and devices and drivers being used.

Sorry,

greg k-h

#59 lore raw
Quoted 10 lines · Greg KH
Quoted 5 lines

I don't know a lot about USB, but I always assumed that this was not a normal condition and that there are only a couple of URBs per endpoint used at a time. Maybe Greg or someone else with a USB background can shed some light on this.

There's no restriction on how many URBs a driver can have outstanding at once, and if you have a system with a lot of USB devices running at the same time, there could be lots of URBs in flight depending on the number of host controllers and devices and drivers being used.

Ok, thanks for clarifying that. I read some more of the em28xx driver, and while it does have a bunch of URBs in flight, there are only five audio and five video URBs that I see simultaneously being submitted, and then resubmitted from their completion handlers. I think this means that there should be 10 URBs active at any given time in this driver, which does not explain why we get 256 allocations.

I also noticed that the initial submissions are all atomic but don't need to, so it may be worth trying the patch below, which should also help in low-memory situations. We could also try moving the resubmission into a workqueue in order to let those be GFP_KERNEL, but I don't think that will help.

Arnd

drivers/media/usb/em28xx/em28xx-audio.c +3 -3
index 2fdb66e..8b789f4 100644
@@ -177,12 +177,12 @@ static int em28xx_init_audio_isoc(struct em28xx *dev)
 		struct urb *urb;
 		int j, k;
 
-		dev->adev.transfer_buffer[i] = kmalloc(sb_size, GFP_ATOMIC);
+		dev->adev.transfer_buffer[i] = kmalloc(sb_size, GFP_KERNEL);
 		if (!dev->adev.transfer_buffer[i])
 			return -ENOMEM;
 
 		memset(dev->adev.transfer_buffer[i], 0x80, sb_size);
-		urb = usb_alloc_urb(EM28XX_NUM_AUDIO_PACKETS, GFP_ATOMIC);
+		urb = usb_alloc_urb(EM28XX_NUM_AUDIO_PACKETS, GFP_KERNEL);
 		if (!urb) {
 			em28xx_errdev("usb_alloc_urb failed!\n");
 			for (j = 0; j < i; j++) {

@@ -212,7 +212,7 @@ static int em28xx_init_audio_isoc(struct em28xx *dev)
 	}
 
 	for (i = 0; i < EM28XX_AUDIO_BUFS; i++) {
-		errCode = usb_submit_urb(dev->adev.urb[i], GFP_ATOMIC);
+		errCode = usb_submit_urb(dev->adev.urb[i], GFP_KERNEL);
 		if (errCode) {
 			em28xx_errdev("submit of audio urb failed\n");
 			em28xx_deinit_isoc_audio(dev);
drivers/media/usb/em28xx/em28xx-core.c +1 -1
index bed07a6..c5a2c4b 100644
@@ -1166,7 +1166,7 @@ int em28xx_init_isoc(struct em28xx *dev, enum em28xx_mode mode,
 
 	/* submit urbs and enables IRQ */
 	for (i = 0; i < isoc_bufs->num_bufs; i++) {
-		rc = usb_submit_urb(isoc_bufs->urb[i], GFP_ATOMIC);
+		rc = usb_submit_urb(isoc_bufs->urb[i], GFP_KERNEL);
 		if (rc) {
 			em28xx_err("submit of urb %i failed (error=%i)\n", i,
 				   rc);
#60 lore raw
Quoted 11 lines · Arnd Bergmann
Quoted 10 lines · Greg KH
Quoted 5 lines

I don't know a lot about USB, but I always assumed that this was not a normal condition and that there are only a couple of URBs per endpoint used at a time. Maybe Greg or someone else with a USB background can shed some light on this.

There's no restriction on how many URBs a driver can have outstanding at once, and if you have a system with a lot of USB devices running at the same time, there could be lots of URBs in flight depending on the number of host controllers and devices and drivers being used.

I only use one host controller and (in this test) two usb devices with the same driver.

Quoted 6 lines

Ok, thanks for clarifying that. I read some more of the em28xx driver, and while it does have a bunch of URBs in flight, there are only five audio and five video URBs that I see simultaneously being submitted, and then resubmitted from their completion handlers. I think this means that there should be 10 URBs active at any given time in this driver, which does not explain why we get 256 allocations.

I think the audio part of the em28xx bridge is not used in my DVB tests.

Are there other allocations from orion-ehci directly? Maybe something special for isochronous transfers (since there is no problem with my other dvb sticks using bulk transfers)?

Quoted 5 lines

I also noticed that the initial submissions are all atomic but don't need to, so it may be worth trying the patch below, which should also help in low-memory situations. We could also try moving the resubmission into a workqueue in order to let those be GFP_KERNEL, but I don't think that will help.

I built a linux-3.7.4 with the em28xx patch and both of your dma-mapping.c patches. I still see the
ERROR: 1024 KiB atomic DMA coherent pool is too small!

Soeren

maintainer
#61 lore raw
In reply to Arnd Bergmann
Quoted 9 lines · Arnd Bergmann
Quoted 8 lines · Soeren Moch
Quoted 7 lines · Arnd Bergmann
Quoted 6 lines · Soeren Moch
Quoted 4 lines
Quoted 2 lines

I will see what I can do here. Is there an easy way to track the buffer usage without having to wait for complete exhaustion?

DMA_API_DEBUG

OK, maybe I can try this.

I tried this. Not what i expected. We have at least one problem with the ethernet driver:

WARNING: at lib/dma-debug.c:933 check_unmap+0x4b8/0x8a8()
mv643xx_eth_port mv643xx_eth_port.0: DMA-API: device driver failed to check map error[device address=0x000000001f22be00] [size=1536 bytes] [mapped as single]
Modules linked in:
[<c000db10>] (unwind_backtrace+0x0/0xf4) from [<c0016c44>] (warn_slowpath_common+0x4c/0x64)
[<c0016c44>] (warn_slowpath_common+0x4c/0x64) from [<c0016cf0>] (warn_slowpath_fmt+0x30/0x40)
[<c0016cf0>] (warn_slowpath_fmt+0x30/0x40) from [<c01ab540>] (check_unmap+0x4b8/0x8a8)
[<c01ab540>] (check_unmap+0x4b8/0x8a8) from [<c01abbb8>] (debug_dma_unmap_page+0x8c/0x98)
[<c01abbb8>] (debug_dma_unmap_page+0x8c/0x98) from [<c025cb1c>] (mv643xx_eth_poll+0x630/0x800)
[<c025cb1c>] (mv643xx_eth_poll+0x630/0x800) from [<c0331d9c>] (net_rx_action+0xcc/0x1d4)
[<c0331d9c>] (net_rx_action+0xcc/0x1d4) from [<c001e1b0>] (__do_softirq+0xa8/0x170)
[<c001e1b0>] (__do_softirq+0xa8/0x170) from [<c001e3e8>] (do_softirq+0x5c/0x6c)
[<c001e3e8>] (do_softirq+0x5c/0x6c) from [<c001e610>] (local_bh_enable+0xcc/0xdc)
[<c001e610>] (local_bh_enable+0xcc/0xdc) from [<c0359c74>] (ip_finish_output+0x1c8/0x39c)
[<c0359c74>] (ip_finish_output+0x1c8/0x39c) from [<c03571a4>] (ip_local_out+0x28/0x2c)
[<c03571a4>] (ip_local_out+0x28/0x2c) from [<c0359564>] (ip_queue_xmit+0x118/0x338)
[<c0359564>] (ip_queue_xmit+0x118/0x338) from [<c036e58c>] (tcp_transmit_skb+0x3fc/0x8e4)
[<c036e58c>] (tcp_transmit_skb+0x3fc/0x8e4) from [<c0371218>] (tcp_write_xmit+0x228/0xb08)
[<c0371218>] (tcp_write_xmit+0x228/0xb08) from [<c0371b6c>] (__tcp_push_pending_frames+0x30/0x9c)
[<c0371b6c>] (__tcp_push_pending_frames+0x30/0x9c) from [<c0362940>] (tcp_sendmsg+0x158/0xdc4)
[<c0362940>] (tcp_sendmsg+0x158/0xdc4) from [<c0386620>] (inet_sendmsg+0x38/0x74)
[<c0386620>] (inet_sendmsg+0x38/0x74) from [<c031f370>] (sock_aio_write+0x12c/0x138)
[<c031f370>] (sock_aio_write+0x12c/0x138) from [<c00a62f4>] (do_sync_write+0xa0/0xd0)
[<c00a62f4>] (do_sync_write+0xa0/0xd0) from [<c00a6f18>] (vfs_write+0x13c/0x144)
[<c00a6f18>] (vfs_write+0x13c/0x144) from [<c00a6ff0>] (sys_write+0x44/0x70)
[<c00a6ff0>] (sys_write+0x44/0x70) from [<c0008ce0>] (ret_fast_syscall+0x0/0x2c)
---[ end trace b75faa8779652e63 ]---

I'm getting about 4 errors reported a second from the ethernet driver.

Before i look at issues with em28xx i will first try to get the noise from the ethernet driver sorted out.

Andrew

maintainer
#62 lore raw
In reply to Jason Cooper
Quoted 31 lines · Jason Cooper

Greg,

I've added you to the this thread hoping for a little insight into USB drivers and their use of coherent and GFP_ATOMIC. Am I barking up the wrong tree by looking a the drivers?

Quoted 24 lines · Soeren Moch
Quoted 11 lines · Marek Szyprowski

dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag, regardless the flags provided by the caller. This causes excessive pruning of emergency memory pools without any good reason. Additionaly, on ARM architecture any driver which is using dmapools will sooner or later trigger the following error:
"ERROR: 256 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!". Increasing the coherent pool size usually doesn't help much and only delays such error, because all GFP_ATOMIC DMA allocations are always served from the special, very limited memory pool.

Quoted 8 lines

This patch changes the dmapool code to correctly use gfp flags provided by the dmapool caller.

Reported-by Soeren Moch<[email protected]>
Reported-by Thomas Petazzoni<[email protected]>
Signed-off-by Marek Szyprowski<[email protected]>
Tested-by Andrew Lunn<[email protected]>
Tested-by Soeren Moch<[email protected]>

Now I tested linux-3.7.1 (this patch is included there) on my Marvell Kirkwood system. I still see

Quoted 8 lines
Quoted 5 lines

ERROR: 1024 KiB atomic DMA coherent pool is too small! Please increase it with coherent_pool= kernel parameter!

after several hours of runtime under heavy load with SATA and DVB-Sticks (em28xx / drxk and dib0700).

Could you try running the system w/o the em28xx stick and see how it goes with v3.7.1?

Jason,

can you point out what you think we should be looking for?

I grep'd for 'GFP_' in drivers/media/usb and especially for dvb-usb (dib0700) it looks like most of the buffers in usb-urb.c are allocated GFP_ATOMIC. em28xx also allocates some of the buffers atomic.

If we look for a mem leak in one of the above drivers (including sata_mv), is there an easy way to keep track of allocated and freed kernel memory?

Sebastian

#63 lore raw
Quoted 2 lines · Sebastian Hesselbarth

If we look for a mem leak in one of the above drivers (including sata_mv), is there an easy way to keep track of allocated and freed kernel memory?

I'm inclined to think sata_mv is not the cause here, as there are many heavy users of it without error reports. The only thing different here are the three usb dvb dongles.

thx,

Jason.