optimize DNAME_INLINE_LEN

optimize DNAME_INLINE_LEN

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]

The dcache entries are allocated with SLAB_HWCACHE_ALIGN. For better
memory usage, we should increase DNAME_INLINE_LEN so that sizeof(struct
dentry) becomes a multiple of the L1 cache line size.

On i386 the DNAME_INLINE_LEN becomes 36 bytes instead of 16, which saves
thousands of kmallocs for external file names. (12818 on my debug
system, after updatedb)

The attached patch is preliminary, it doesn't compile with egcs-1.1.2.
Which gcc version added support for unnamed structures?

--
	Manfred

[-- Attachment #2: patch-dcache --]
[-- Type: text/plain, Size: 742 bytes --]

--- 2.5/include/linux/dcache.h	Thu Oct 11 15:20:18 2001
+++ build-2.5/include/linux/dcache.h	Thu Dec 13 22:55:50 2001
@@ -61,9 +61,7 @@
 	return end_name_hash(hash);
 }
 
-#define DNAME_INLINE_LEN 16
-
-struct dentry {
+struct dentry_nameless {
 	atomic_t d_count;
 	unsigned int d_flags;
 	struct inode  * d_inode;	/* Where the name belongs to - NULL is negative */
@@ -80,6 +78,12 @@
 	struct super_block * d_sb;	/* The root of the dentry tree */
 	unsigned long d_vfs_flags;
 	void * d_fsdata;		/* fs-specific data */
+};
+#define DNAME_INLINE_LEN \
+	(16 + L1_CACHE_BYTES - (16+sizeof(struct dentry_nameless))%L1_CACHE_BYTES)
+
+struct dentry {
+	struct dentry_nameless;
 	unsigned char d_iname[DNAME_INLINE_LEN]; /* small names */
 };
 

Re: optimize DNAME_INLINE_LEN

[Andreas Dilger]

On Dec 13, 2001  23:22 +0100, Manfred Spraul wrote:
> The dcache entries are allocated with SLAB_HWCACHE_ALIGN. For better
> memory usage, we should increase DNAME_INLINE_LEN so that sizeof(struct
> dentry) becomes a multiple of the L1 cache line size.
> 
> On i386 the DNAME_INLINE_LEN becomes 36 bytes instead of 16, which saves
> thousands of kmallocs for external file names. (12818 on my debug
> system, after updatedb)

If the dentries are already aligned on cachelines, I don't see any reason
NOT to do this.  Why waste all the memory for alignment when it can be
used for something else?

> The attached patch is preliminary, it doesn't compile with egcs-1.1.2.
> Which gcc version added support for unnamed structures?

Hmm, I thought it was gcc 3.0 that supported unnames structures.  For
sure gcc 2.95.2 does not, because unnamed structs are used in the NTFS
tools, and I couldn't compile them.

If you wanted to keep compatibility for older compilers (may be a good idea)
you could do something ugly like using a named struct like "du" and then:

#define d_inode du.du_inode
#define d_count du.du_count
.
.
.
#define d_fsdata du.du_fsdata



Alternately (also ugly) you could just define struct dentry the as now,
but have a fixed size declaration for d_iname, like:

#define DNAME_INLINE_MIN 16

	unsigned char d_iname[DNAME_INLINE_MIN];
	
and only set DNAME_INLINE_LEN afterwards like:

#define DNAME_INLINE_LEN \
	(DNAME_INLINE_MIN+L1_CACHE_BYTES - sizeof(struct dentry)%L1_CACHE_BYTES)

This _should_ work for all code that uses DNAME_INLINE_LEN, and if the
alignment doesn't change.  It will break horribly if you do sizeof(struct
dentry), declare a dentry on the stack, or if someone changes the alignment.

You can also do preprocessor macro tricks to get something like an unnamed
union in a struct, but it is also a bit ugly.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Re: optimize DNAME_INLINE_LEN

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 331 bytes --]

Andreas Dilger wrote:
> 
> You can also do preprocessor macro tricks to get something like an unnamed
> union in a struct, but it is also a bit ugly.
>
I'd prefer that (patch attached, tested with egcs-1.1.2)

Unless I receive complaints, I'll send it to Linus tomorrow, and then
backport to 2.4 after Linus merged it.

--
	Manfred

[-- Attachment #2: patch-dcache --]
[-- Type: text/plain, Size: 1958 bytes --]

--- 2.5/include/linux/dcache.h	Thu Oct 11 15:20:18 2001
+++ build-2.5/include/linux/dcache.h	Thu Dec 13 23:39:32 2001
@@ -61,25 +61,33 @@
 	return end_name_hash(hash);
 }
 
-#define DNAME_INLINE_LEN 16
+#define DENTRY_MEMBERS \
+	atomic_t d_count; \
+	unsigned int d_flags; \
+	struct inode  * d_inode;	/* Where the name belongs to - NULL is negative */ \
+	struct dentry * d_parent;	/* parent directory */ \
+	struct list_head d_hash;	/* lookup hash list */ \
+	struct list_head d_lru;		/* d_count = 0 LRU list */ \
+	struct list_head d_child;	/* child of parent list */ \
+	struct list_head d_subdirs;	/* our children */ \
+	struct list_head d_alias;	/* inode alias list */ \
+	int d_mounted; \
+	struct qstr d_name; \
+	unsigned long d_time;		/* used by d_revalidate */ \
+	struct dentry_operations  *d_op; \
+	struct super_block * d_sb;	/* The root of the dentry tree */ \
+	unsigned long d_vfs_flags; \
+	void * d_fsdata;		/* fs-specific data */
+
+struct dentry_nameless {
+	DENTRY_MEMBERS
+};
+/* +15 to guarantee a minimal inline len of 16 bytes */
+#define DNAME_INLINE_LEN \
+	(15 + L1_CACHE_BYTES - (15+sizeof(struct dentry_nameless))%L1_CACHE_BYTES)
 
 struct dentry {
-	atomic_t d_count;
-	unsigned int d_flags;
-	struct inode  * d_inode;	/* Where the name belongs to - NULL is negative */
-	struct dentry * d_parent;	/* parent directory */
-	struct list_head d_hash;	/* lookup hash list */
-	struct list_head d_lru;		/* d_count = 0 LRU list */
-	struct list_head d_child;	/* child of parent list */
-	struct list_head d_subdirs;	/* our children */
-	struct list_head d_alias;	/* inode alias list */
-	int d_mounted;
-	struct qstr d_name;
-	unsigned long d_time;		/* used by d_revalidate */
-	struct dentry_operations  *d_op;
-	struct super_block * d_sb;	/* The root of the dentry tree */
-	unsigned long d_vfs_flags;
-	void * d_fsdata;		/* fs-specific data */
+	DENTRY_MEMBERS
 	unsigned char d_iname[DNAME_INLINE_LEN]; /* small names */
 };
 

Re: optimize DNAME_INLINE_LEN

[Andi Kleen]

On Thu, Dec 13, 2001 at 04:07:06PM -0700, Andreas Dilger wrote:
> Alternately (also ugly) you could just define struct dentry the as now,
> but have a fixed size declaration for d_iname, like:
> 
> #define DNAME_INLINE_MIN 16
> 
> 	unsigned char d_iname[DNAME_INLINE_MIN];
                   Using [0] here would also work 
and fixing other code to add DNAME_INLINE_MIN as needed. Unfortunately
this "fixing other code" would likely prevent the patch going into 2.4,
which would be bad. 

#define d_... has a similar problem => the potential to break previously
compiling source code.

Probably just using an compiler #ifdef is best, and perhaps doing it 
cleanly (with using d_iname[0]) on 2.5.


-Andi

P.S.: I originally picked the 16 number and it was totally arbitary, so 
an increase on the fallback to 20-30 would be likely ok. 

Re: optimize DNAME_INLINE_LEN

[Andreas Dilger]

On Dec 14, 2001  00:29 +0100, Andi Kleen wrote:
> On Thu, Dec 13, 2001 at 04:07:06PM -0700, Andreas Dilger wrote:
> > Alternately (also ugly) you could just define struct dentry the as now,
> > but have a fixed size declaration for d_iname, like:
> > 
> > #define DNAME_INLINE_MIN 16
> > 
> > 	unsigned char d_iname[DNAME_INLINE_MIN];
>                    Using [0] here would also work 

Well, not really.  If we wanted to have a minimum size for the d_iname
field, then if we declare it as zero and it just squeaks into a chacheline,
then we may be stuck with 0 bytes of inline names, and _all_ names will
be kmalloced.

> #define d_... has a similar problem => the potential to break previously
> compiling source code.

Again, not really.  The #define d_... scheme would leave all of the fields
in their original locations, just giving them new names within the named
struct, and the defines would be the backwards compatible (and probably
still preferrable) way to access these fields.  I don't _think_ it would
cause any compiler struct alignment issues to just put the same fields
in another struct, but I could be wrong.

Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/

Re: optimize DNAME_INLINE_LEN

[Andi Kleen]

On Thu, Dec 13, 2001 at 05:03:50PM -0700, Andreas Dilger wrote:
> On Dec 14, 2001  00:29 +0100, Andi Kleen wrote:
> > On Thu, Dec 13, 2001 at 04:07:06PM -0700, Andreas Dilger wrote:
> > > Alternately (also ugly) you could just define struct dentry the as now,
> > > but have a fixed size declaration for d_iname, like:
> > > 
> > > #define DNAME_INLINE_MIN 16
> > > 
> > > 	unsigned char d_iname[DNAME_INLINE_MIN];
> >                    Using [0] here would also work 
> 
> Well, not really.  If we wanted to have a minimum size for the d_iname
> field, then if we declare it as zero and it just squeaks into a chacheline,
> then we may be stuck with 0 bytes of inline names, and _all_ names will
> be kmalloced.

That can be avoided by using a suitable formula for DNAME_INLINE_MIN

> 
> > #define d_... has a similar problem => the potential to break previously
> > compiling source code.
> 
> Again, not really.  The #define d_... scheme would leave all of the fields
> in their original locations, just giving them new names within the named
> struct, and the defines would be the backwards compatible (and probably
> still preferrable) way to access these fields.  I don't _think_ it would
> cause any compiler struct alignment issues to just put the same fields
> in another struct, but I could be wrong.

It won't help if any code is doing

void function()
{ 
	int d_name;
	...
}

Preprocessor doesn't care about scopes so it would turn that into
an syntax error.

Anyways it is moot as Manfred's latest solution looks like the way to go.

-Andi