[RFC/PATCH] ext3: remove inode constructor

[RFC/PATCH] ext3: remove inode constructor

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

As explained by Christoph Lameter, ext3_alloc_inode() touches the same
cache line as init_once() so we gain nothing from using slab
constructors.  The SLUB allocator will be more effective without it
(free pointer can be placed inside the free'd object), so move inode
initialization to ext3_alloc_inode completely.

[postmark: numbers = 10000, transactions = 10000, 2 GHz Pentium M]

2.6.21 vanilla:

  real	0m19.006s
  user	0m0.144s
  sys	0m7.424s

  real	0m9.040s
  user	0m0.156s
  sys	0m5.164s

  real	0m8.939s
  user	0m0.128s
  sys	0m5.180s

2.6.21 + ext3-remove-inode-constructor:

  real	0m19.176s
  user	0m0.176s
  sys	0m7.436s

  real	0m9.030s
  user	0m0.172s
  sys	0m5.120s

  real	0m8.966s
  user	0m0.168s
  sys	0m5.132s

Cc: Stephen C. Tweedie <sct@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andreas Dilger <adilger@clusterfs.com>
Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 fs/ext3/super.c |   30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

Index: 2.6/fs/ext3/super.c
===================================================================
--- 2.6.orig/fs/ext3/super.c	2007-05-04 12:57:09.000000000 +0300
+++ 2.6/fs/ext3/super.c	2007-05-04 13:01:27.000000000 +0300
@@ -444,17 +444,26 @@ static struct kmem_cache *ext3_inode_cac
 static struct inode *ext3_alloc_inode(struct super_block *sb)
 {
 	struct ext3_inode_info *ei;
+	struct inode *inode;
 
 	ei = kmem_cache_alloc(ext3_inode_cachep, GFP_NOFS);
 	if (!ei)
 		return NULL;
+        INIT_LIST_HEAD(&ei->i_orphan);
+#ifdef CONFIG_EXT3_FS_XATTR
+        init_rwsem(&ei->xattr_sem);
+#endif
+        mutex_init(&ei->truncate_mutex);
 #ifdef CONFIG_EXT3_FS_POSIX_ACL
 	ei->i_acl = EXT3_ACL_NOT_CACHED;
 	ei->i_default_acl = EXT3_ACL_NOT_CACHED;
 #endif
 	ei->i_block_alloc_info = NULL;
-	ei->vfs_inode.i_version = 1;
-	return &ei->vfs_inode;
+
+	inode = &ei->vfs_inode;
+	inode_init_once(inode);
+	inode->i_version = 1;
+	return inode;
 }
 
 static void ext3_destroy_inode(struct inode *inode)
@@ -462,28 +471,13 @@ static void ext3_destroy_inode(struct in
 	kmem_cache_free(ext3_inode_cachep, EXT3_I(inode));
 }
 
-static void init_once(void * foo, struct kmem_cache * cachep, unsigned long flags)
-{
-	struct ext3_inode_info *ei = (struct ext3_inode_info *) foo;
-
-	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
-	    SLAB_CTOR_CONSTRUCTOR) {
-		INIT_LIST_HEAD(&ei->i_orphan);
-#ifdef CONFIG_EXT3_FS_XATTR
-		init_rwsem(&ei->xattr_sem);
-#endif
-		mutex_init(&ei->truncate_mutex);
-		inode_init_once(&ei->vfs_inode);
-	}
-}
-
 static int init_inodecache(void)
 {
 	ext3_inode_cachep = kmem_cache_create("ext3_inode_cache",
 					     sizeof(struct ext3_inode_info),
 					     0, (SLAB_RECLAIM_ACCOUNT|
 						SLAB_MEM_SPREAD),
-					     init_once, NULL);
+					     NULL, NULL);
 	if (ext3_inode_cachep == NULL)
 		return -ENOMEM;
 	return 0;

Re: [RFC/PATCH] ext3: remove inode constructor

[Christoph Lameter]

On Fri, 4 May 2007, Pekka J Enberg wrote:

> Index: 2.6/fs/ext3/super.c
> ===================================================================
> --- 2.6.orig/fs/ext3/super.c	2007-05-04 12:57:09.000000000 +0300
> +++ 2.6/fs/ext3/super.c	2007-05-04 13:01:27.000000000 +0300
> @@ -444,17 +444,26 @@ static struct kmem_cache *ext3_inode_cac
>  static struct inode *ext3_alloc_inode(struct super_block *sb)
>  {
>  	struct ext3_inode_info *ei;
> +	struct inode *inode;
>  
>  	ei = kmem_cache_alloc(ext3_inode_cachep, GFP_NOFS);
>  	if (!ei)
>  		return NULL;
> +        INIT_LIST_HEAD(&ei->i_orphan);
> +#ifdef CONFIG_EXT3_FS_XATTR
> +        init_rwsem(&ei->xattr_sem);
> +#endif
> +        mutex_init(&ei->truncate_mutex);

^^^ whitespace issues

Also could this be generalized to also apply to the generic inode 
allocation in fs/inode.c?

Re: [RFC/PATCH] ext3: remove inode constructor

[Andrew Morton]

On Fri, 4 May 2007 13:14:35 +0300 (EEST)
Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

> As explained by Christoph Lameter, ext3_alloc_inode() touches the same
> cache line as init_once() so we gain nothing from using slab
> constructors.  The SLUB allocator will be more effective without it
> (free pointer can be placed inside the free'd object), so move inode
> initialization to ext3_alloc_inode completely.

I got 100% rejects against this because Christoph has already had
his paws all over the slab constructor code everywhere.

Was going to fix it up but then decided that we ought to make changes
like this to ext4 as well.  Ideally beforehand, but simultaneously is
OK as long as it's simple enough.

btw, for a benchmark I'd suggest just a silly create-10000-files
tight loop rather than something more complex like postmark.

Re: [RFC/PATCH] ext3: remove inode constructor

[Pekka J Enberg]

On Fri, 4 May 2007, Andrew Morton wrote:
> I got 100% rejects against this because Christoph has already had
> his paws all over the slab constructor code everywhere.
> 
> Was going to fix it up but then decided that we ought to make changes
> like this to ext4 as well.  Ideally beforehand, but simultaneously is
> OK as long as it's simple enough.

I'll send you proper patches for them (and will convert other filesystems 
too).

On Fri, 4 May 2007, Andrew Morton wrote:
> btw, for a benchmark I'd suggest just a silly create-10000-files
> tight loop rather than something more complex like postmark.

Do you want me to redo the benchmarks or are you happy enough with the 
postmark numbers?

				Pekka

Re: [RFC/PATCH] ext3: remove inode constructor

[Andrew Morton]

On Sat, 5 May 2007 11:58:45 +0300 (EEST) Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

> On Fri, 4 May 2007, Andrew Morton wrote:
> > I got 100% rejects against this because Christoph has already had
> > his paws all over the slab constructor code everywhere.
> > 
> > Was going to fix it up but then decided that we ought to make changes
> > like this to ext4 as well.  Ideally beforehand, but simultaneously is
> > OK as long as it's simple enough.
> 
> I'll send you proper patches for them (and will convert other filesystems 
> too).

May as well.

> On Fri, 4 May 2007, Andrew Morton wrote:
> > btw, for a benchmark I'd suggest just a silly create-10000-files
> > tight loop rather than something more complex like postmark.
> 
> Do you want me to redo the benchmarks or are you happy enough with the 
> postmark numbers?

I doubt if this is measurable, really.  It'll be something like the
difference between an L1 hit and an L2 hit in amongst all the other stuff
we do on a per-inode basis.

Re: [RFC/PATCH] ext3: remove inode constructor

[Pekka J Enberg]

On Sat, 5 May 2007, Andrew Morton wrote:
> I doubt if this is measurable, really.  It'll be something like the
> difference between an L1 hit and an L2 hit in amongst all the other stuff
> we do on a per-inode basis.

The cache effects are probably not easily measurable but I was worried 
about CPU time. The slab constructors and destructors were originally 
designed to reduce CPU time spent on initializing objects. But I don't 
think that's relevant anymore on todays CPUs.

			Pekka

Re: [RFC/PATCH] ext3: remove inode constructor

[Pekka J Enberg]

On Fri, 4 May 2007, Christoph Lameter wrote:
> Also could this be generalized to also apply to the generic inode 
> allocation in fs/inode.c?

I think we want to stick inode_init_ince() in alloc_inode() but we can't 
do that until all filesystems are converted over.

			Pekka