[PATCH 1/1] afs: afs_alloc_inode: use kmem_cache_zalloc

[PATCH 1/1] afs: afs_alloc_inode: use kmem_cache_zalloc

[Fabian Frederick]

afs_vnode is currently cleared with 2 memsets after allocation and 
1 in constructor (afs_i_init_once).
	-This patch calls zalloc for explicit zero fill.
	-Fix some typos.

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 fs/afs/super.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/afs/super.c b/fs/afs/super.c
index c486155..e1fc569 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -76,7 +76,7 @@ static const match_table_t afs_options_list = {
 };
 
 /*
- * initialise the filesystem
+ * Initialize the filesystem
  */
 int __init afs_fs_init(void)
 {
@@ -448,19 +448,19 @@ error:
 static void afs_kill_super(struct super_block *sb)
 {
 	struct afs_super_info *as = sb->s_fs_info;
+
 	kill_anon_super(sb);
 	afs_put_volume(as->volume);
 	kfree(as);
 }
 
 /*
- * initialise an inode cache slab element prior to any use
+ * Initialize an inode cache slab element prior to any use
  */
 static void afs_i_init_once(void *_vnode)
 {
 	struct afs_vnode *vnode = _vnode;
 
-	memset(vnode, 0, sizeof(*vnode));
 	inode_init_once(&vnode->vfs_inode);
 	init_waitqueue_head(&vnode->update_waitq);
 	mutex_init(&vnode->permits_lock);
@@ -481,15 +481,12 @@ static struct inode *afs_alloc_inode(struct super_block *sb)
 {
 	struct afs_vnode *vnode;
 
-	vnode = kmem_cache_alloc(afs_inode_cachep, GFP_KERNEL);
+	vnode = kmem_cache_zalloc(afs_inode_cachep, GFP_KERNEL);
 	if (!vnode)
 		return NULL;
 
 	atomic_inc(&afs_count_active_inodes);
 
-	memset(&vnode->fid, 0, sizeof(vnode->fid));
-	memset(&vnode->status, 0, sizeof(vnode->status));
-
 	vnode->volume		= NULL;
 	vnode->update_cnt	= 0;
 	vnode->flags		= 1 << AFS_VNODE_UNSET;
@@ -503,6 +500,7 @@ static void afs_i_callback(struct rcu_head *head)
 {
 	struct inode *inode = container_of(head, struct inode, i_rcu);
 	struct afs_vnode *vnode = AFS_FS_I(inode);
+
 	kmem_cache_free(afs_inode_cachep, vnode);
 }
 
-- 
1.8.1.4

Re: [PATCH 1/1] afs: afs_alloc_inode: use kmem_cache_zalloc

[Joe Perches]

On Thu, 2014-02-20 at 21:16 +0800, Fabian Frederick wrote:
> afs_vnode is currently cleared with 2 memsets after allocation and 
> 1 in constructor (afs_i_init_once).

Trivial comments:

> 	-This patch calls zalloc for explicit zero fill.

OK, but see below.

> 	-Fix some typos.

Well...

> diff --git a/fs/afs/super.c b/fs/afs/super.c
[]
> @@ -76,7 +76,7 @@ static const match_table_t afs_options_list = {
[]
> - * initialise the filesystem
> + * Initialize the filesystem
> @@ -448,19 +448,19 @@ error:
[]
> - * initialise an inode cache slab element prior to any use
> + * Initialize an inode cache slab element prior to any use

English spellings don't need to be changed to American.

> @@ -481,15 +481,12 @@ static struct inode *afs_alloc_inode(struct super_block *sb)
[]
> -	vnode = kmem_cache_alloc(afs_inode_cachep, GFP_KERNEL);
> +	vnode = kmem_cache_zalloc(afs_inode_cachep, GFP_KERNEL);
[]
> -	memset(&vnode->fid, 0, sizeof(vnode->fid));
> -	memset(&vnode->status, 0, sizeof(vnode->status));
> -
>  	vnode->volume		= NULL;
>  	vnode->update_cnt	= 0;

These could be removed too.

Re: [PATCH 1/1] afs: afs_alloc_inode: use kmem_cache_zalloc

[David Howells]

Joe Perches <joe@perches.com> wrote:

> > - * initialise an inode cache slab element prior to any use
> > + * Initialize an inode cache slab element prior to any use
> 
> English spellings don't need to be changed to American.

Indeed! :-)

David

Has slab ctor operation changed? -- was [PATCH 1/1] afs: afs_alloc_inode: use kmem_cache_zalloc

[David Howells]

Fabian Frederick <fabf@skynet.be> wrote:

> afs_vnode is currently cleared with 2 memsets after allocation and 
> 1 in constructor (afs_i_init_once).
> 	-This patch calls zalloc for explicit zero fill.

Ummm...  This patch isn't necessarily correct in the substantiative portions.

Since afs_i_init_once() is called by the slab allocator during the course of
kmem_cache_alloc(), how does kmem_cache_zalloc() interact with that?

IIRC, it used to be that the ctor() function was called when the pages were
allocated to the slab - and it wasn't called again, even if the object was
allocated, deallocated and reallocated.  This means that things like locks and
lists don't need reinitialising after allocation.

So afs_i_init_once() theoretically constructs the stuff that can be reused,
and afs_alloc_inode() therefore has to clear the non-reusable state.

Of course, it's possible that the slab allocator no longer works like this...

David

Re: Has slab ctor operation changed? -- was [PATCH 1/1] afs: afs_alloc_inode: use kmem_cache_zalloc

[Fabian Frederick]

On Thu, 20 Feb 2014 22:23:15 +0000
David Howells <dhowells@redhat.com> wrote:

> Fabian Frederick <fabf@skynet.be> wrote:
> 
> > afs_vnode is currently cleared with 2 memsets after allocation and 
> > 1 in constructor (afs_i_init_once).
> > 	-This patch calls zalloc for explicit zero fill.
> 
> Ummm...  This patch isn't necessarily correct in the substantiative portions.
> 
> Since afs_i_init_once() is called by the slab allocator during the course of
> kmem_cache_alloc(), how does kmem_cache_zalloc() interact with that?

Object of this patch was to replace any kmem_cache_alloc by kmem_cache_zalloc
so I guess what is done in the patch v2 in afs_alloc_inode below zalloc
would be ok (leaving ctor how it is) ?

> 
> IIRC, it used to be that the ctor() function was called when the pages were
> allocated to the slab - and it wasn't called again, even if the object was
> allocated, deallocated and reallocated.  This means that things like locks and
> lists don't need reinitialising after allocation.
> 
> So afs_i_init_once() theoretically constructs the stuff that can be reused,
> and afs_alloc_inode() therefore has to clear the non-reusable state.
> 
> Of course, it's possible that the slab allocator no longer works like this...
> 
> David

Re: Has slab ctor operation changed? -- was [PATCH 1/1] afs: afs_alloc_inode: use kmem_cache_zalloc

[Dave Chinner]

On Thu, Feb 20, 2014 at 10:23:15PM +0000, David Howells wrote:
> Fabian Frederick <fabf@skynet.be> wrote:
> 
> > afs_vnode is currently cleared with 2 memsets after allocation and 
> > 1 in constructor (afs_i_init_once).
> > 	-This patch calls zalloc for explicit zero fill.
> 
> Ummm...  This patch isn't necessarily correct in the substantiative portions.
> 
> Since afs_i_init_once() is called by the slab allocator during the course of
> kmem_cache_alloc(), how does kmem_cache_zalloc() interact with that?

It breaks it. ;)

> IIRC, it used to be that the ctor() function was called when the pages were
> allocated to the slab - and it wasn't called again, even if the object was
> allocated, deallocated and reallocated.  This means that things like locks and
> lists don't need reinitialising after allocation.
> 
> So afs_i_init_once() theoretically constructs the stuff that can be reused,
> and afs_alloc_inode() therefore has to clear the non-reusable state.
> 
> Of course, it's possible that the slab allocator no longer works like this...

AFAIA the slab constructor behaviour has not changed. If it did, I'm
pretty sure most filesystems would end up with inode cache problems
because most of them rely on this ctor behaviour to avoid
unnecessary reinitialisation of inode state....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: Has slab ctor operation changed? -- was [PATCH 1/1] afs: afs_alloc_inode: use kmem_cache_zalloc

[Christoph Lameter]

On Thu, 20 Feb 2014, David Howells wrote:

> Of course, it's possible that the slab allocator no longer works like this...

They still work like that (aside from SLOB that calls the ctor on each
alloc but that also has never changed).