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(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index c5ab33b..86de9b2 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -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;
}
--
1.7.9.5
On Thu, Nov 08, 2012 at 07:38:57AM +0100, Marek Szyprowski wrote:
> 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
> ---
> mm/dmapool.c | 27 +++++++--------------------
> 1 file changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index c5ab33b..86de9b2 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -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;
> }
> --
> 1.7.9.5
>
On 11.11.2012 18:22, Andrew Lunn wrote:
> On Thu, Nov 08, 2012 at 07:38:57AM +0100, Marek Szyprowski wrote:
>> 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]>
Please use
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]>
>
> 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
>> ---
>> mm/dmapool.c | 27 +++++++--------------------
>> 1 file changed, 7 insertions(+), 20 deletions(-)
>>
>> diff --git a/mm/dmapool.c b/mm/dmapool.c
>> index c5ab33b..86de9b2 100644
>> --- a/mm/dmapool.c
>> +++ b/mm/dmapool.c
>> @@ -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;
>> }
>> --
>> 1.7.9.5
>>
On Mon, Nov 12, 2012 at 10:48:02AM +0100, Soeren Moch wrote:
> On 11.11.2012 18:22, Andrew Lunn wrote:
> > On Thu, Nov 08, 2012 at 07:38:57AM +0100, Marek Szyprowski wrote:
> >> 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]>
> Please use
> 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]>
> >
> > 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!
Hi Soeren
Could you tell us what DVB devices you are using.
Thanks
Andrew
On 12.11.2012 11:38, Andrew Lunn wrote:
> On Mon, Nov 12, 2012 at 10:48:02AM +0100, Soeren Moch wrote:
>> On 11.11.2012 18:22, Andrew Lunn wrote:
>>> On Thu, Nov 08, 2012 at 07:38:57AM +0100, Marek Szyprowski wrote:
>>>> 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]>
>> Please use
>> 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]>
>>>
>>> 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!
> 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
Marek,
I've added the maintainers for mm/*. Hopefully they can let us know if
this is good for v3.8...
thx,
Jason.
On Thu, Nov 08, 2012 at 07:38:57AM +0100, Marek Szyprowski wrote:
> 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(-)
>
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index c5ab33b..86de9b2 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -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;
> }
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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.
Hello,
On 11/19/2012 11:48 PM, Andrew Morton wrote:
> 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.
> 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.
> 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.
> 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.
> 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
--
Marek Szyprowski
Samsung Poland R&D Center
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(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index c5ab33b..da1b0f0 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -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);
--
1.7.9.5
On Tue, 20 Nov 2012 15:31:45 +0100
Marek Szyprowski <[email protected]> wrote:
> 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?
On Tue, 20 Nov 2012 11:48:44 +0100
Marek Szyprowski <[email protected]> wrote:
> > 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.
On Tue, Nov 20, 2012 at 11:33:25AM -0800, Andrew Morton wrote:
> On Tue, 20 Nov 2012 15:31:45 +0100
> Marek Szyprowski <[email protected]> wrote:
>
> > 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?
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
Hello,
On 11/20/2012 8:33 PM, Andrew Morton wrote:
> On Tue, 20 Nov 2012 15:31:45 +0100
> Marek Szyprowski <[email protected]> wrote:
>
> > 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?
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
--
Marek Szyprowski
Samsung Poland R&D Center
On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <[email protected]> wrote:
> Hello,
>
> On 11/20/2012 8:33 PM, Andrew Morton wrote:
> > On Tue, 20 Nov 2012 15:31:45 +0100
> > Marek Szyprowski <[email protected]> wrote:
> >
> > > 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?
>
> 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.
Hello,
On 11/21/2012 9:36 AM, Andrew Morton wrote:
> On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <[email protected]> wrote:
>
> > Hello,
> >
> > On 11/20/2012 8:33 PM, Andrew Morton wrote:
> > > On Tue, 20 Nov 2012 15:31:45 +0100
> > > Marek Szyprowski <[email protected]> wrote:
> > >
> > > > 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?
> >
> > 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.
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
--
Marek Szyprowski
Samsung Poland R&D Center
On Wed, 21 Nov 2012 10:20:07 +0100
Marek Szyprowski <[email protected]> wrote:
> Hello,
>
> On 11/21/2012 9:36 AM, Andrew Morton wrote:
> > On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <[email protected]> wrote:
> >
> > > Hello,
> > >
> > > On 11/20/2012 8:33 PM, Andrew Morton wrote:
> > > > On Tue, 20 Nov 2012 15:31:45 +0100
> > > > Marek Szyprowski <[email protected]> wrote:
> > > >
> > > > > 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?
> > >
> > > 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.
>
> 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.
On 11/21/2012 8:17 PM, Andrew Morton wrote:
> On Wed, 21 Nov 2012 10:20:07 +0100
> Marek Szyprowski <[email protected]> wrote:
> > On 11/21/2012 9:36 AM, Andrew Morton wrote:
> > > On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <[email protected]> wrote:
> > > > On 11/20/2012 8:33 PM, Andrew Morton wrote:
> > > > > On Tue, 20 Nov 2012 15:31:45 +0100
> > > > > Marek Szyprowski <[email protected]> wrote:
> > > > >
> > > > > > 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?
> > > >
> > > > 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.
> >
> > 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.
OK, I will push it to v3.8-rc1 then and tag for stable.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center