[RFC,PATCH 0/2] dmapool: allocation gfp changes

[RFC,PATCH 0/2] dmapool: allocation gfp changes

[Roger Oksanen]

Hi,
When introducing dma pools to the e100 driver, it was noticed that allocation
failure warnings were being generated to dmesg when allocations were being
retried (in dma_pool_alloc). Patch #1 changes the allocator to suppress most
warnings (a warning every 10th retry).

Patch #2, which applies on top of patch #1, changes the GFP_ATOMIC
allocation, which allows emergency pool usage, to use the callers GFP_*
flags. This is a scary change that can cause delays in the allocation path,
but is still imho the correct way. Discussion about this approach is welcome!

Additionally, I'm wondering if dma_pool_alloc(..) should be allowed to
fail after a while even when using __GFP_WAIT. Currently it loops forever
if it can't find the requested memory. A quick grep reveals that memory
allocation failures are handled in most drivers using (pci|dma)_pool_alloc.

PATCH 1/2
Don't warn when allowed to retry allocation.

PATCH 2/2
Honor GFP_* flags.

 mm/dmapool.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

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

[RFC,PATCH 1/2] dmapool: Don't warn when allowed to retry allocation.

[Roger Oksanen]

dmapool: Don't warn when allowed to retry allocation.

dmapool uses it's own wait logic, so allocations failing may be retried
if the called specified a waiting GFP_*. Unnecessary warnings only cause
confusion.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
Signed-off-by: Roger Oksanen <roger.oksanen@cs.helsinki.fi>
---
 mm/dmapool.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index 3df0637..e270f7f 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -310,6 +310,8 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 	struct dma_page *page;
 	size_t offset;
 	void *retval;
+	int tries = 0;
+	const gfp_t can_wait = mem_flags & __GFP_WAIT;
 
 	spin_lock_irqsave(&pool->lock, flags);
  restart:
@@ -317,9 +321,11 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 		if (page->offset < pool->allocation)
 			goto ready;
 	}
-	page = pool_alloc_page(pool, GFP_ATOMIC);
+	tries++;
+	page = pool_alloc_page(pool, GFP_ATOMIC | (can_wait && tries % 10
+						  ? __GFP_NOWARN : 0));
 	if (!page) {
-		if (mem_flags & __GFP_WAIT) {
+		if (can_wait) {
 			DECLARE_WAITQUEUE(wait, current);
 
 			__set_current_state(TASK_INTERRUPTIBLE);

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

[RFC,PATCH 2/2] dmapool: Honor GFP_* flags.

[Roger Oksanen]

dmapool: Honor GFP_* flags.

dmapool silently discarded GFP flags and was always allowed to use the 
emergency pool.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
Signed-off-by: Roger Oksanen <roger.oksanen@cs.helsinki.fi>
---
 mm/dmapool.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index 2fdd7a1..e270f7f 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -312,6 +312,8 @@
 	void *retval;
 	int tries = 0;
 	const gfp_t can_wait = mem_flags & __GFP_WAIT;
+	/* dma_pool_alloc uses its own wait logic */
+	mem_flags &= ~__GFP_WAIT;
 
 	spin_lock_irqsave(&pool->lock, flags);
  restart:
@@ -320,7 +322,7 @@
 			goto ready;
 	}
 	tries++;
-	page = pool_alloc_page(pool, GFP_ATOMIC | (can_wait && tries % 10
+	page = pool_alloc_page(pool, mem_flags | (can_wait && tries % 10
 						  ? __GFP_NOWARN : 0));
 	if (!page) {
 		if (can_wait) {

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

Re: [RFC,PATCH 1/2] dmapool: Don't warn when allowed to retry allocation.

[Christoph Lameter]

On Wed, 2 Dec 2009, Roger Oksanen wrote:

> dmapool: Don't warn when allowed to retry allocation.

It warns after 10 attempts even when allowed to retry? Description is not
entirely accurate.

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

Re: [RFC,PATCH 2/2] dmapool: Honor GFP_* flags.

[Christoph Lameter]

On Wed, 2 Dec 2009, Roger Oksanen wrote:

>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index 2fdd7a1..e270f7f 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -312,6 +312,8 @@
>  	void *retval;
>  	int tries = 0;
>  	const gfp_t can_wait = mem_flags & __GFP_WAIT;
> +	/* dma_pool_alloc uses its own wait logic */
> +	mem_flags &= ~__GFP_WAIT;

Why mask the wait flag? If you can call the page allocator with __GFP_WAIT
then you dont have to loop.

> -	page = pool_alloc_page(pool, GFP_ATOMIC | (can_wait && tries % 10
> +	page = pool_alloc_page(pool, mem_flags | (can_wait && tries % 10
>  						  ? __GFP_NOWARN : 0));

You are now uselessly calling the page allocator with __GFP_WAIT cleared
although the context allows you to wait.

Just pass through the mem_flags? Rename them gfp_flags for consistencies
sake?

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

Re: [RFC,PATCH 1/2] dmapool: Don't warn when allowed to retry allocation.

[Roger Oksanen]

On Wednesday 02 December 2009 21:56:10 Christoph Lameter wrote:
> On Wed, 2 Dec 2009, Roger Oksanen wrote:
> > dmapool: Don't warn when allowed to retry allocation.
> 
> It warns after 10 attempts even when allowed to retry? Description is not
> entirely accurate.

I left one part off by mistake. The whole descriptions should have read
"dmapool uses it's own wait logic, so allocations failing may be retried
if the called specified a waiting GFP_*. Unnecessary warnings only cause
confusion. Every 10th retry will still cause a warning, to disclose a 
possible problem."

10 retries (* POOL_TIMEOUT_JIFFIES) roughly means 1s, so then I assume there 
is really some problems in finding the requested memory. If the pool allocator 
was allowed to fail after n retries, then that point would probably be the 
best place to warn on.

best regards,
-- 
Roger Oksanen <roger.oksanen@cs.helsinki.fi>
http://www.cs.helsinki.fi/u/raoksane
+358 50 355 1990

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

Re: [RFC,PATCH 2/2] dmapool: Honor GFP_* flags.

[Roger Oksanen]

On Wednesday 02 December 2009 22:00:56 Christoph Lameter wrote:
> On Wed, 2 Dec 2009, Roger Oksanen wrote:
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/dmapool.c b/mm/dmapool.c
> > index 2fdd7a1..e270f7f 100644
> > --- a/mm/dmapool.c
> > +++ b/mm/dmapool.c
> > @@ -312,6 +312,8 @@
> >  	void *retval;
> >  	int tries = 0;
> >  	const gfp_t can_wait = mem_flags & __GFP_WAIT;
> > +	/* dma_pool_alloc uses its own wait logic */
> > +	mem_flags &= ~__GFP_WAIT;
> 
> Why mask the wait flag? If you can call the page allocator with __GFP_WAIT
> then you dont have to loop.

That would fundamentally change how the pool allocator works. Currently it 
waits on its own wait queue for returned memory from dma_pool_free(..). 
Waiting in the page allocator won't allow it to claim memory returned there. 
Now if it would be possible (is it?), it should sit on both wait queues.


> > -	page = pool_alloc_page(pool, GFP_ATOMIC | (can_wait && tries % 10
> > +	page = pool_alloc_page(pool, mem_flags | (can_wait && tries % 10
> >  						  ? __GFP_NOWARN : 0));
> 
> You are now uselessly calling the page allocator with __GFP_WAIT cleared
> although the context allows you to wait.
> 
> Just pass through the mem_flags? Rename them gfp_flags for consistencies
> sake?

Killing off the pools own wait logic is indeed possible. The question is 
though, is it desirable? Is allocating dma memory on some arches so expensive 
that its crucial to try to avoid any extra allocations when pooled memory may 
be freed up in a few moments?

best regards,
-- 
Roger Oksanen <roger.oksanen@cs.helsinki.fi>
http://www.cs.helsinki.fi/u/raoksane
+358 50 355 1990

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

Re: [RFC,PATCH 2/2] dmapool: Honor GFP_* flags.

[Christoph Lameter]

On Wed, 2 Dec 2009, Roger Oksanen wrote:

> That would fundamentally change how the pool allocator works. Currently it
> waits on its own wait queue for returned memory from dma_pool_free(..).

Plus it also has a timeout. What usually triggers first? Repeated attempts
and a timeout... All smells like heuristics that better be avoided.

> Waiting in the page allocator won't allow it to claim memory returned there.

If __GFP_WAIT is set then the page allocator can perform direct reclaim
getting you the memory you want!


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

Re: [RFC,PATCH 2/2] dmapool: Honor GFP_* flags.

[Mel Gorman]

On Wed, Dec 02, 2009 at 02:00:56PM -0600, Christoph Lameter wrote:
> On Wed, 2 Dec 2009, Roger Oksanen wrote:
> 
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/dmapool.c b/mm/dmapool.c
> > index 2fdd7a1..e270f7f 100644
> > --- a/mm/dmapool.c
> > +++ b/mm/dmapool.c
> > @@ -312,6 +312,8 @@
> >  	void *retval;
> >  	int tries = 0;
> >  	const gfp_t can_wait = mem_flags & __GFP_WAIT;
> > +	/* dma_pool_alloc uses its own wait logic */
> > +	mem_flags &= ~__GFP_WAIT;
> 
> Why mask the wait flag? If you can call the page allocator with __GFP_WAIT
> then you dont have to loop.
> 

Because the wait logic in the dma pool is significantly different to
what the page allocator itself does. It's not obvious why that is
or what the consequences would be if it was changed.

What I would guess (but have not researched) is that the pages in use by
the pool are expected to be more or less fixed and requests that exceed
the pool size are rare. In the unlikely event the pool is depleted, it's
preferred by the caller to wait for a short period instead of entering
direct reclaim which may take far longer. Their expectation is that a
short wait will be enough for a pool page to be returned and less costly
than the normal wait logic.

The intent of the patch is to cover the case where dma_pool_alloc() is called
with a zone modifier. Grep doesn't show up cases where that happens but if
a new user comes along and specifies GFP_DMA32 and doesn't test on a machine
with enough memory, they'll get a lovely surprise.

I don't think it's worth the risk at this time of converting the DMA pool
to use the page allocators wait logic. The conversion itself would be
simple enough but the testing is not and any potential benefits are
unclear at best.

> > -	page = pool_alloc_page(pool, GFP_ATOMIC | (can_wait && tries % 10
> > +	page = pool_alloc_page(pool, mem_flags | (can_wait && tries % 10
> >  						  ? __GFP_NOWARN : 0));
> 
> You are now uselessly calling the page allocator with __GFP_WAIT cleared
> although the context allows you to wait.
> 
> Just pass through the mem_flags? Rename them gfp_flags for consistencies
> sake?
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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