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

[Mel Gorman <mel@csn.ul.ie>]

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>