Re: [PATCH 26/30] nfs: remove mempools

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Fri, 2008-07-25 at 00:46 +1000, Nick Piggin wrote:
> On Friday 25 July 2008 00:01, Peter Zijlstra wrote:
> > With the introduction of the shared dirty page accounting in .19, NFS
> > should not be able to surpise the VM with all dirty pages. Thus it should
> > always be able to free some memory. Hence no more need for mempools.
> 
> It seems like a very backward step to me to go from a hard guarantee
> to some heuristic that could break for someone in some particular
> setup.
> 
> Filling with dirty pages isn't the only way to exaust free reclaimable
> memory remember, it can also happen maybe due to mlock or kernel
> allocations.
> 
> Is there pressing reason to remove them?

There was some funny interaction between mempools and PF_MEMALLOC as
used to write out swap pages iirc.

Specifially, the mempool would strip it and not provide memory even
though we needed it.

I've never hit a deadlock due to this patch in my testing, although
admittedly there might be a hole in my testing.

I can either 'fix' mempools, or replace it with the reserve framework
introduced earlier in this patch-set.

> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > ---
> >  fs/nfs/read.c  |   15 +++------------
> >  fs/nfs/write.c |   27 +++++----------------------
> >  2 files changed, 8 insertions(+), 34 deletions(-)
> >
> > Index: linux-2.6/fs/nfs/read.c
> > ===================================================================
> > --- linux-2.6.orig/fs/nfs/read.c
> > +++ linux-2.6/fs/nfs/read.c
> > @@ -33,13 +33,10 @@ static const struct rpc_call_ops nfs_rea
> >  static const struct rpc_call_ops nfs_read_full_ops;
> >
> >  static struct kmem_cache *nfs_rdata_cachep;
> > -static mempool_t *nfs_rdata_mempool;
> > -
> > -#define MIN_POOL_READ	(32)
> >
> >  struct nfs_read_data *nfs_readdata_alloc(unsigned int pagecount)
> >  {
> > -	struct nfs_read_data *p = mempool_alloc(nfs_rdata_mempool, GFP_NOFS);
> > +	struct nfs_read_data *p = kmem_cache_alloc(nfs_rdata_cachep, GFP_NOFS);
> >
> >  	if (p) {
> >  		memset(p, 0, sizeof(*p));
> > @@ -50,7 +47,7 @@ struct nfs_read_data *nfs_readdata_alloc
> >  		else {
> >  			p->pagevec = kcalloc(pagecount, sizeof(struct page *), GFP_NOFS);
> >  			if (!p->pagevec) {
> > -				mempool_free(p, nfs_rdata_mempool);
> > +				kmem_cache_free(nfs_rdata_cachep, p);
> >  				p = NULL;
> >  			}
> >  		}
> > @@ -62,7 +59,7 @@ static void nfs_readdata_free(struct nfs
> >  {
> >  	if (p && (p->pagevec != &p->page_array[0]))
> >  		kfree(p->pagevec);
> > -	mempool_free(p, nfs_rdata_mempool);
> > +	kmem_cache_free(nfs_rdata_cachep, p);
> >  }
> >
> >  void nfs_readdata_release(void *data)
> > @@ -614,16 +611,10 @@ int __init nfs_init_readpagecache(void)
> >  	if (nfs_rdata_cachep == NULL)
> >  		return -ENOMEM;
> >
> > -	nfs_rdata_mempool = mempool_create_slab_pool(MIN_POOL_READ,
> > -						     nfs_rdata_cachep);
> > -	if (nfs_rdata_mempool == NULL)
> > -		return -ENOMEM;
> > -
> >  	return 0;
> >  }
> >
> >  void nfs_destroy_readpagecache(void)
> >  {
> > -	mempool_destroy(nfs_rdata_mempool);
> >  	kmem_cache_destroy(nfs_rdata_cachep);
> >  }
> > Index: linux-2.6/fs/nfs/write.c
> > ===================================================================
> > --- linux-2.6.orig/fs/nfs/write.c
> > +++ linux-2.6/fs/nfs/write.c
> > @@ -28,9 +28,6 @@
> >
> >  #define NFSDBG_FACILITY		NFSDBG_PAGECACHE
> >
> > -#define MIN_POOL_WRITE		(32)
> > -#define MIN_POOL_COMMIT		(4)
> > -
> >  /*
> >   * Local function declarations
> >   */
> > @@ -45,12 +42,10 @@ static const struct rpc_call_ops nfs_wri
> >  static const struct rpc_call_ops nfs_commit_ops;
> >
> >  static struct kmem_cache *nfs_wdata_cachep;
> > -static mempool_t *nfs_wdata_mempool;
> > -static mempool_t *nfs_commit_mempool;
> >
> >  struct nfs_write_data *nfs_commitdata_alloc(void)
> >  {
> > -	struct nfs_write_data *p = mempool_alloc(nfs_commit_mempool, GFP_NOFS);
> > +	struct nfs_write_data *p = kmem_cache_alloc(nfs_wdata_cachep, GFP_NOFS);
> >
> >  	if (p) {
> >  		memset(p, 0, sizeof(*p));
> > @@ -63,12 +58,12 @@ void nfs_commit_free(struct nfs_write_da
> >  {
> >  	if (p && (p->pagevec != &p->page_array[0]))
> >  		kfree(p->pagevec);
> > -	mempool_free(p, nfs_commit_mempool);
> > +	kmem_cache_free(nfs_wdata_cachep, p);
> >  }
> >
> >  struct nfs_write_data *nfs_writedata_alloc(unsigned int pagecount)
> >  {
> > -	struct nfs_write_data *p = mempool_alloc(nfs_wdata_mempool, GFP_NOFS);
> > +	struct nfs_write_data *p = kmem_cache_alloc(nfs_wdata_cachep, GFP_NOFS);
> >
> >  	if (p) {
> >  		memset(p, 0, sizeof(*p));
> > @@ -79,7 +74,7 @@ struct nfs_write_data *nfs_writedata_all
> >  		else {
> >  			p->pagevec = kcalloc(pagecount, sizeof(struct page *), GFP_NOFS);
> >  			if (!p->pagevec) {
> > -				mempool_free(p, nfs_wdata_mempool);
> > +				kmem_cache_free(nfs_wdata_cachep, p);
> >  				p = NULL;
> >  			}
> >  		}
> > @@ -91,7 +86,7 @@ static void nfs_writedata_free(struct nf
> >  {
> >  	if (p && (p->pagevec != &p->page_array[0]))
> >  		kfree(p->pagevec);
> > -	mempool_free(p, nfs_wdata_mempool);
> > +	kmem_cache_free(nfs_wdata_cachep, p);
> >  }
> >
> >  void nfs_writedata_release(void *data)
> > @@ -1552,16 +1547,6 @@ int __init nfs_init_writepagecache(void)
> >  	if (nfs_wdata_cachep == NULL)
> >  		return -ENOMEM;
> >
> > -	nfs_wdata_mempool = mempool_create_slab_pool(MIN_POOL_WRITE,
> > -						     nfs_wdata_cachep);
> > -	if (nfs_wdata_mempool == NULL)
> > -		return -ENOMEM;
> > -
> > -	nfs_commit_mempool = mempool_create_slab_pool(MIN_POOL_COMMIT,
> > -						      nfs_wdata_cachep);
> > -	if (nfs_commit_mempool == NULL)
> > -		return -ENOMEM;
> > -
> >  	/*
> >  	 * NFS congestion size, scale with available memory.
> >  	 *
> > @@ -1587,8 +1572,6 @@ int __init nfs_init_writepagecache(void)
> >
> >  void nfs_destroy_writepagecache(void)
> >  {
> > -	mempool_destroy(nfs_commit_mempool);
> > -	mempool_destroy(nfs_wdata_mempool);
> >  	kmem_cache_destroy(nfs_wdata_cachep);
> >  }

--
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>