Re: [Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

Re: [Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

[Adam J. Richter]

Hi Maneesh,

	Here is a rewrite of my patch to remove sysfs_dirent.s_count,
reducing the memory chunk that kmalloc uses for sysfs_dirent from
64 to 32 bytes, thereby saving about 120kB for the 3700+ nodes in
/sys on the ordinary PC on which I am typing this message.

	Unlike my original patch, this version retains
sysfs_dentry_ops, because, as you correctly pointed out, the
current implementation uses sysfs_dirent for file-descriptor
based operations that can still be done after a node has
been unlinked.  I still might seek to eliminate sysfs_dentry_ops
in a future patch, but there are other steps I would
like to pursue first.

	This version replaces the check on sysfs_dirent.s_count
with a check that syfs_dirent.s_dentry is NULL and that the
sysfs_dirent is not linked into the directory tree.  If both of
those conditions are satisfied, then sysfs_dirent is freed.

	Note that in sysfs_remove_dir and sysfs_hash_and_remove
I had to move the call to list_del_init(&sd->s_sibling) to occur
just before the call to sysfs_put() to avoid confusion that
can occur in another call to syfs_put() that the previously
intervening call to sysfs_drop_dentry() could cause.

	By the way, I ran a version of sysfs_put that kept the
reference counting, calculated this new check and would complain if
the two checks produced a different result, and I saw no complaints,
including while I ran the test that you previously proposed
for a while (loading and unloading the dummy networking modules,
while running "ls -lR" on the /sys and cat'ing of the networking
/sys files).

	I have also run this cleaned up patch with the same test for
a while with no problems (unlike the situation with my original patch).

	So, if you could take a look at it and send it downstream
for integration if it looks good to you, I would appreciate it.
I would like to get this patch integrated before I try to implement
further reductions in pinned memory for sysfs.  After this patch,
I hope to make a patch to unpin the inode and dentry structures for
sysfs directories, and then perhaps a patch changing attribute groups
to have just one sysfs_dirent for the group instead of one for each
attribute (individual attributes registered directly to a kobject
would still need one sysfs_dirent each).

	Please let me know what you think.

                    __     ______________
Adam J. Richter        \ /
adam@yggdrasil.com      | g g d r a s i l


--- linux-2.6.10-rc2-bk13/include/linux/sysfs.h	2004-11-17 18:59:17.000000000 +0800
+++ linux/include/linux/sysfs.h	2004-12-02 01:18:12.000000000 +0800
@@ -60,7 +60,6 @@
 };
 
 struct sysfs_dirent {
-	atomic_t		s_count;
 	struct list_head	s_sibling;
 	struct list_head	s_children;
 	void 			* s_element;
diff -u linux-2.6.10-rc2-bk13/fs/sysfs/dir.c linux/fs/sysfs/dir.c
--- linux-2.6.10-rc2-bk13/fs/sysfs/dir.c	2004-12-01 11:02:42.000000000 +0800
+++ linux/fs/sysfs/dir.c	2004-12-02 00:06:12.000000000 +0800
@@ -41,7 +41,6 @@
 		return NULL;
 
 	memset(sd, 0, sizeof(*sd));
-	atomic_set(&sd->s_count, 1);
 	INIT_LIST_HEAD(&sd->s_children);
 	list_add(&sd->s_sibling, &parent_sd->s_children);
 	sd->s_element = element;
@@ -62,7 +61,7 @@
 	sd->s_type = type;
 	sd->s_dentry = dentry;
 	if (dentry) {
-		dentry->d_fsdata = sysfs_get(sd);
+		dentry->d_fsdata = sd;
 		dentry->d_op = &sysfs_dentry_ops;
 	}
 
@@ -180,7 +179,7 @@
 		dentry->d_inode->i_fop = &bin_fops;
 	}
 	dentry->d_op = &sysfs_dentry_ops;
-	dentry->d_fsdata = sysfs_get(sd);
+	dentry->d_fsdata = sd;
 	sd->s_dentry = dentry;
 	d_rehash(dentry);
 
@@ -194,7 +193,7 @@
 	err = sysfs_create(dentry, S_IFLNK|S_IRWXUGO, init_symlink);
 	if (!err) {
 		dentry->d_op = &sysfs_dentry_ops;
-		dentry->d_fsdata = sysfs_get(sd);
+		dentry->d_fsdata = sd;
 		sd->s_dentry = dentry;
 		d_rehash(dentry);
 	}
@@ -280,8 +279,8 @@
 	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
 		if (!sd->s_element || !(sd->s_type & SYSFS_NOT_PINNED))
 			continue;
-		list_del_init(&sd->s_sibling);
 		sysfs_drop_dentry(sd, dentry);
+		list_del_init(&sd->s_sibling);
 		sysfs_put(sd);
 	}
 	up(&dentry->d_inode->i_sem);
diff -u linux-2.6.10-rc2-bk13/fs/sysfs/inode.c linux/fs/sysfs/inode.c
--- linux-2.6.10-rc2-bk13/fs/sysfs/inode.c	2004-11-17 18:59:13.000000000 +0800
+++ linux/fs/sysfs/inode.c	2004-12-02 00:05:25.000000000 +0800
@@ -149,8 +149,8 @@
 		if (!sd->s_element)
 			continue;
 		if (!strcmp(sysfs_get_name(sd), name)) {
-			list_del_init(&sd->s_sibling);
 			sysfs_drop_dentry(sd, dir);
+			list_del_init(&sd->s_sibling);
 			sysfs_put(sd);
 			break;
 		}
diff -u linux-2.6.10-rc2-bk13/fs/sysfs/sysfs.h linux/fs/sysfs/sysfs.h
--- linux-2.6.10-rc2-bk13/fs/sysfs/sysfs.h	2004-11-17 18:59:13.000000000 +0800
+++ linux/fs/sysfs/sysfs.h	2004-12-02 00:05:35.000000000 +0800
@@ -77,18 +77,8 @@
 	kfree(sd);
 }
 
-static inline struct sysfs_dirent * sysfs_get(struct sysfs_dirent * sd)
-{
-	if (sd) {
-		WARN_ON(!atomic_read(&sd->s_count));
-		atomic_inc(&sd->s_count);
-	}
-	return sd;
-}
-
 static inline void sysfs_put(struct sysfs_dirent * sd)
 {
-	if (atomic_dec_and_test(&sd->s_count))
+	if (list_empty(&sd->s_sibling) && sd->s_dentry == NULL)
 		release_sysfs_dirent(sd);
 }
-

Re: [Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

[Andrew Morton]

"Adam J. Richter" <adam@yggdrasil.com> wrote:
>
> 	Here is a rewrite of my patch to remove sysfs_dirent.s_count,
>  reducing the memory chunk that kmalloc uses for sysfs_dirent from
>  64 to 32 bytes, thereby saving about 120kB for the 3700+ nodes in
>  /sys on the ordinary PC on which I am typing this message.

That's all well and good, but sysfs_new_dirent() should be using a
standalone slab cache for allocating sysfs_dirent instances.  That way, we
use 36 bytes for each one rather than 64.

Re: [Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

[Chris Wright]

* Andrew Morton (akpm@osdl.org) wrote:
> That's all well and good, but sysfs_new_dirent() should be using a
> standalone slab cache for allocating sysfs_dirent instances.  That way, we
> use 36 bytes for each one rather than 64.

Reasonable, here's a patch (lightly tested).  Without, size-64 looks
like so:

size-64             4064   4108     76   52    1 : tunables   32   16 8 : slabdata     79     79      0 : globalstat    4263   4079    79    0 0    0   84    0 : cpustat  15986    337  12286      3

And with:

size-64             1196   1196     76   52    1 : tunables   32   16 8 : slabdata     23     23      0 : globalstat    1297   1196    23    0 0    0   84    0 : cpustat  12418    108  11349      1
sysfs_dir_cache     2862   2916     48   81    1 : tunables   32   16 8 : slabdata     36     36      0 : globalstat    2931   2874    36    0 0    0  113    0 : cpustat   2756    216    110      0


Allocate sysfs_dirent structures from their own slab.

Signed-off-by: Chris Wright <chrisw@osdl.org>

===== fs/sysfs/dir.c 1.31 vs edited =====
--- 1.31/fs/sysfs/dir.c	2004-11-17 16:00:00 -08:00
+++ edited/fs/sysfs/dir.c	2004-12-01 13:29:11 -08:00
@@ -36,7 +36,7 @@
 {
 	struct sysfs_dirent * sd;
 
-	sd = kmalloc(sizeof(*sd), GFP_KERNEL);
+	sd = kmem_cache_alloc(sysfs_dir_cachep, GFP_KERNEL);
 	if (!sd)
 		return NULL;
 
===== fs/sysfs/sysfs.h 1.14 vs edited =====
--- 1.14/fs/sysfs/sysfs.h	2004-10-30 06:15:11 -07:00
+++ edited/fs/sysfs/sysfs.h	2004-12-01 13:58:42 -08:00
@@ -1,5 +1,6 @@
 
 extern struct vfsmount * sysfs_mount;
+extern kmem_cache_t *sysfs_dir_cachep;
 
 extern struct inode * sysfs_new_inode(mode_t mode);
 extern int sysfs_create(struct dentry *, int mode, int (*init)(struct inode *));
@@ -74,7 +75,7 @@
 		kobject_put(sl->target_kobj);
 		kfree(sl);
 	}
-	kfree(sd);
+	kmem_cache_free(sysfs_dir_cachep, sd);
 }
 
 static inline struct sysfs_dirent * sysfs_get(struct sysfs_dirent * sd)
===== fs/sysfs/mount.c 1.11 vs edited =====
--- 1.11/fs/sysfs/mount.c	2004-10-30 06:10:49 -07:00
+++ edited/fs/sysfs/mount.c	2004-12-01 14:43:44 -08:00
@@ -16,6 +16,7 @@
 
 struct vfsmount *sysfs_mount;
 struct super_block * sysfs_sb = NULL;
+kmem_cache_t *sysfs_dir_cachep;
 
 static struct super_operations sysfs_ops = {
 	.statfs		= simple_statfs,
@@ -76,7 +77,13 @@
 
 int __init sysfs_init(void)
 {
-	int err;
+	int err = -ENOMEM;
+
+	sysfs_dir_cachep = kmem_cache_create("sysfs_dir_cache",
+					      sizeof(struct sysfs_dirent),
+					      0, 0, NULL, NULL);
+	if (!sysfs_dir_cachep)
+		goto out;
 
 	err = register_filesystem(&sysfs_fs_type);
 	if (!err) {
@@ -85,7 +92,13 @@
 			printk(KERN_ERR "sysfs: could not mount!\n");
 			err = PTR_ERR(sysfs_mount);
 			sysfs_mount = NULL;
+			goto out_err;
 		}
-	}
+	} else
+		goto out_err;
+out:
 	return err;
+out_err:
+	kmem_cache_destroy(sysfs_dir_cachep);
+	goto out;
 }

Re: [Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

[Greg KH]

On Wed, Dec 01, 2004 at 03:51:59PM -0800, Chris Wright wrote:
> * Andrew Morton (akpm@osdl.org) wrote:
> > That's all well and good, but sysfs_new_dirent() should be using a
> > standalone slab cache for allocating sysfs_dirent instances.  That way, we
> > use 36 bytes for each one rather than 64.
> 
> Reasonable, here's a patch (lightly tested).  Without, size-64 looks
> like so:
> 
> size-64             4064   4108     76   52    1 : tunables   32   16 8 : slabdata     79     79      0 : globalstat    4263   4079    79    0 0    0   84    0 : cpustat  15986    337  12286      3
> 
> And with:
> 
> size-64             1196   1196     76   52    1 : tunables   32   16 8 : slabdata     23     23      0 : globalstat    1297   1196    23    0 0    0   84    0 : cpustat  12418    108  11349      1
> sysfs_dir_cache     2862   2916     48   81    1 : tunables   32   16 8 : slabdata     36     36      0 : globalstat    2931   2874    36    0 0    0  113    0 : cpustat   2756    216    110      0
> 
> 
> Allocate sysfs_dirent structures from their own slab.

Nice, thanks for doing this.

Applied,

greg k-h

Re: [Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

[Maneesh Soni]

On Wed, Dec 01, 2004 at 07:09:18PM +0000, Adam J. Richter wrote:
> Hi Maneesh,
> 
> 	Here is a rewrite of my patch to remove sysfs_dirent.s_count,
> reducing the memory chunk that kmalloc uses for sysfs_dirent from
> 64 to 32 bytes, thereby saving about 120kB for the 3700+ nodes in
> /sys on the ordinary PC on which I am typing this message.
> 
> 	Unlike my original patch, this version retains
> sysfs_dentry_ops, because, as you correctly pointed out, the
> current implementation uses sysfs_dirent for file-descriptor
> based operations that can still be done after a node has
> been unlinked.  I still might seek to eliminate sysfs_dentry_ops
> in a future patch, but there are other steps I would
> like to pursue first.
> 
> 	This version replaces the check on sysfs_dirent.s_count
> with a check that syfs_dirent.s_dentry is NULL and that the
> sysfs_dirent is not linked into the directory tree.  If both of
> those conditions are satisfied, then sysfs_dirent is freed.
> 
> 	Note that in sysfs_remove_dir and sysfs_hash_and_remove
> I had to move the call to list_del_init(&sd->s_sibling) to occur
> just before the call to sysfs_put() to avoid confusion that
> can occur in another call to syfs_put() that the previously
> intervening call to sysfs_drop_dentry() could cause.
> 
> 	By the way, I ran a version of sysfs_put that kept the
> reference counting, calculated this new check and would complain if
> the two checks produced a different result, and I saw no complaints,
> including while I ran the test that you previously proposed
> for a while (loading and unloading the dummy networking modules,
> while running "ls -lR" on the /sys and cat'ing of the networking
> /sys files).
> 
> 	I have also run this cleaned up patch with the same test for
> a while with no problems (unlike the situation with my original patch).
> 
> 	So, if you could take a look at it and send it downstream
> for integration if it looks good to you, I would appreciate it.
> I would like to get this patch integrated before I try to implement
> further reductions in pinned memory for sysfs.  After this patch,
> I hope to make a patch to unpin the inode and dentry structures for
> sysfs directories, and then perhaps a patch changing attribute groups
> to have just one sysfs_dirent for the group instead of one for each
> attribute (individual attributes registered directly to a kobject
> would still need one sysfs_dirent each).
> 
> 	Please let me know what you think.
> 

Sorry for very late response due to vacations and transitions. I am
looking at the all the patches from you, not yet in any tree and
hopefully will not miss any.

s_count never increases after 2, one at the time of sysfs_dirent
creation and the other count while connecting sysfs_dirent with the dentry.
So, it should be fine without it. I kept it thinking that it might help in
unpinning directories also. The patch looks good but I think, as usual this 
will also need testing in -mm for some time. 

Thanks
Maneesh

>                     __     ______________
> Adam J. Richter        \ /
> adam@yggdrasil.com      | g g d r a s i l
> 
> 
> --- linux-2.6.10-rc2-bk13/include/linux/sysfs.h	2004-11-17 18:59:17.000000000 +0800
> +++ linux/include/linux/sysfs.h	2004-12-02 01:18:12.000000000 +0800
> @@ -60,7 +60,6 @@
>  };
>  
>  struct sysfs_dirent {
> -	atomic_t		s_count;
>  	struct list_head	s_sibling;
>  	struct list_head	s_children;
>  	void 			* s_element;
> diff -u linux-2.6.10-rc2-bk13/fs/sysfs/dir.c linux/fs/sysfs/dir.c
> --- linux-2.6.10-rc2-bk13/fs/sysfs/dir.c	2004-12-01 11:02:42.000000000 +0800
> +++ linux/fs/sysfs/dir.c	2004-12-02 00:06:12.000000000 +0800
> @@ -41,7 +41,6 @@
>  		return NULL;
>  
>  	memset(sd, 0, sizeof(*sd));
> -	atomic_set(&sd->s_count, 1);
>  	INIT_LIST_HEAD(&sd->s_children);
>  	list_add(&sd->s_sibling, &parent_sd->s_children);
>  	sd->s_element = element;
> @@ -62,7 +61,7 @@
>  	sd->s_type = type;
>  	sd->s_dentry = dentry;
>  	if (dentry) {
> -		dentry->d_fsdata = sysfs_get(sd);
> +		dentry->d_fsdata = sd;
>  		dentry->d_op = &sysfs_dentry_ops;
>  	}
>  
> @@ -180,7 +179,7 @@
>  		dentry->d_inode->i_fop = &bin_fops;
>  	}
>  	dentry->d_op = &sysfs_dentry_ops;
> -	dentry->d_fsdata = sysfs_get(sd);
> +	dentry->d_fsdata = sd;
>  	sd->s_dentry = dentry;
>  	d_rehash(dentry);
>  
> @@ -194,7 +193,7 @@
>  	err = sysfs_create(dentry, S_IFLNK|S_IRWXUGO, init_symlink);
>  	if (!err) {
>  		dentry->d_op = &sysfs_dentry_ops;
> -		dentry->d_fsdata = sysfs_get(sd);
> +		dentry->d_fsdata = sd;
>  		sd->s_dentry = dentry;
>  		d_rehash(dentry);
>  	}
> @@ -280,8 +279,8 @@
>  	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
>  		if (!sd->s_element || !(sd->s_type & SYSFS_NOT_PINNED))
>  			continue;
> -		list_del_init(&sd->s_sibling);
>  		sysfs_drop_dentry(sd, dentry);
> +		list_del_init(&sd->s_sibling);
>  		sysfs_put(sd);
>  	}
>  	up(&dentry->d_inode->i_sem);
> diff -u linux-2.6.10-rc2-bk13/fs/sysfs/inode.c linux/fs/sysfs/inode.c
> --- linux-2.6.10-rc2-bk13/fs/sysfs/inode.c	2004-11-17 18:59:13.000000000 +0800
> +++ linux/fs/sysfs/inode.c	2004-12-02 00:05:25.000000000 +0800
> @@ -149,8 +149,8 @@
>  		if (!sd->s_element)
>  			continue;
>  		if (!strcmp(sysfs_get_name(sd), name)) {
> -			list_del_init(&sd->s_sibling);
>  			sysfs_drop_dentry(sd, dir);
> +			list_del_init(&sd->s_sibling);
>  			sysfs_put(sd);
>  			break;
>  		}
> diff -u linux-2.6.10-rc2-bk13/fs/sysfs/sysfs.h linux/fs/sysfs/sysfs.h
> --- linux-2.6.10-rc2-bk13/fs/sysfs/sysfs.h	2004-11-17 18:59:13.000000000 +0800
> +++ linux/fs/sysfs/sysfs.h	2004-12-02 00:05:35.000000000 +0800
> @@ -77,18 +77,8 @@
>  	kfree(sd);
>  }
>  
> -static inline struct sysfs_dirent * sysfs_get(struct sysfs_dirent * sd)
> -{
> -	if (sd) {
> -		WARN_ON(!atomic_read(&sd->s_count));
> -		atomic_inc(&sd->s_count);
> -	}
> -	return sd;
> -}
> -
>  static inline void sysfs_put(struct sysfs_dirent * sd)
>  {
> -	if (atomic_dec_and_test(&sd->s_count))
> +	if (list_empty(&sd->s_sibling) && sd->s_dentry == NULL)
>  		release_sysfs_dirent(sd);
>  }
> -
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Maneesh Soni
Linux Technology Center, 
IBM India Software Labs,
Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044990

Re: [Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

[Adam J. Richter]

Chris Wright wrote:
>* Andrew Morton (akpm@osdl.org) wrote:
>> That's all well and good, but sysfs_new_dirent() should be using a
>> standalone slab cache for allocating sysfs_dirent instances.  That way, we
>> use 36 bytes for each one rather than 64.

>Reasonable, here's a patch (lightly tested).  [...]

	Great.  That way 32-bit architectures should be able to
benefit from some another shrink of sysfs_dirent that I'd like
to do, and it will also help 64-bit architectures (where sysfs_dirent
is still larger than 32 bytes even with my change).

	I applied Chris's patch on top of my own (which still
saves about 14kB on my machine and shrinks the source code by 11
lines) and I am running it now.  It looks fine to me, although I
haven't looked into making a separate slab cache before.  I guess
it's okay that there is no alignment requirement on it for now,
as there are a lot of sysfs_dirent structure and more every day,
and programs that would access these structures frequently would
also probably cause access to a lot of them at the same time (by
scanning the sysfs tree), so inter-processor cache line contention
might be offset by the more efficient utilization of the processor's
cache.

	Chris's patch file applied without conflict (there was a
one line offset in fs/sysfs/sysfs.h), so it should be easy to
integrate both patches.  Please let me know if there is anything
further I should or can do to help make that happen, as I'd like
to have my fs/sysfs tree in sync before trying to unpin the
struct dirent and struct inode for sysfs directories, as I
expect that to be a more complex patch (and unpin hundreds of kbytes).

                    __     ______________
Adam J. Richter        \ /
adam@yggdrasil.com      | g g d r a s i l

Re: [Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

[Adam J. Richter]

On Mon, 22 Nov 2004 16:53:09 -0600, Maneesh Soni wrote:
>There can be open files (live dentries) but files getting removed.

	I think you're right.  I did have a problem with the
test that you suggested.  I think I may be able to accomodate
the problem though.

	I had not realized that file->f_dentry will keep the dentry
around even after the file has been removed, at which point
file->f_dentry->d_fsdata will point to garbage rather than a
valid sysfs_direntry.  This is a problem in fs/sysfs/file.c
where sysfs_read_file calls fill_read_buffer, which calls to_attr,
which tries to read sysfs_direnty->s_element.

	I believe I can avoid the need for any of the IO routines
to access sysfs_direntry->s_element by having create_file()
put a copy in inode->u.generic_ip.  If it works, I will send
you an updated patch.  However, it may be a day before I can
do it.

>IMO,
>without having ref count for sysfs_dirent, we could end up loosing the
>sysfs_dirent and end up in inconsistent sysfs_dirent tree with respect to
>dentry tree.

	I believe that, with the patch I posted, the sysfs_dirent
is still removed from the list of the parent directory's children
at exactly the same times as before.

[...]
>I will also test the patch tonight.

	No need.  I hope to have an updated patch for you soon though.
Thanks for the quick and detailed response.

                    __     ______________
Adam J. Richter        \ /
adam@yggdrasil.com      | g g d r a s i l

Re: [Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

[Maneesh Soni]

On Tue, Nov 23, 2004 at 03:17:33AM +0800, Adam J. Richter wrote:
> 	The following patch against linux-2.6.10-rc2-bk6 removes
> the s_count field from sysfs_dirent.  This reduces sizeof(sysfs_dirent)
> from 36 to 32 bytes on 32-bit machines, resulting in big space
> savings because it reduces the size that kmalloc actually uses for
> the allocation from 64 to 32 bytes, and there is one of these for
> every node in sysfs, of which there are 3405 on the modest desktop
> machine that I'm using to compose this email.  That's a savings of
> 108,960 bytes of unswappable kernel memory in this case.
> 
> 	Reference counting appears to me to be unnecessary on this
> data structure.  sysfs_dirent exists when a node name is registered in
> sysfs, and it does not exist when the node name is not registered.
> It does not matter if a program is still holding a reference to the
> struct inode when sysfs_dirent is deleted, since sysfs_dirent is only
> relevant to directory lookup operations.  It also should not matter if the
> system is freeing the struct inode and the struct dentry to save
> space.  As long as the file is registered in sysfs, the sysfs_dirent
> is not freed.
> 
> 	Removing sysfs_dirent.s_count results in the removal of other
> supporting code, including sysfs_dentry_ops, for a net deletion of
> 39 lines of code.
> 
> 	I have only tested this patch by mounting /sys, and running
> some "find" commands on it, plugging and unplugging a USB device,
> and verifying that the number of entries in sysfs increased and
> decreased accordingly.  I am running it on the system on which I
> am composing this email.
> 

The idea for having ref count for sysfs_dirent was to keep the sysfs_dirents
around as long as there are live dentries corresponding to sysfs objects.
There can be open files (live dentries) but files getting removed. IMO,
without having ref count for sysfs_dirent, we could end up loosing the 
sysfs_dirent and end up in inconsistent sysfs_dirent tree with respect to
dentry tree. If we could maintain the consistency without refcounts then
it is good to reduce the size of sysfs_dirent structure.

Could you also test the patch like this, on an SMP box. Basically
opening/closing sysfs files and simultaneously inserting & removing dummy
module. This is just to make sure that we don't have any races 
particularly in dir and file operations. I will also test the patch tonight.

# while true; do find /sys/class/net/ | xargs cat; done
# while true; do insmod dummy.ko; rmmod dummy; done
# while true; do ls -lR /sys > /dev/null; done


Thanks
Maneesh

-- 
Maneesh Soni
Linux Technology Center, 
IBM Austin
email: maneesh@in.ibm.com
Phone: 1-512-838-1896 Fax: 
T/L : 6781896

[Patch] Delete sysfs_dirent.s_count, saving ~100kB on my system

[Adam J. Richter]

	The following patch against linux-2.6.10-rc2-bk6 removes
the s_count field from sysfs_dirent.  This reduces sizeof(sysfs_dirent)
from 36 to 32 bytes on 32-bit machines, resulting in big space
savings because it reduces the size that kmalloc actually uses for
the allocation from 64 to 32 bytes, and there is one of these for
every node in sysfs, of which there are 3405 on the modest desktop
machine that I'm using to compose this email.  That's a savings of
108,960 bytes of unswappable kernel memory in this case.

	Reference counting appears to me to be unnecessary on this
data structure.  sysfs_dirent exists when a node name is registered in
sysfs, and it does not exist when the node name is not registered.
It does not matter if a program is still holding a reference to the
struct inode when sysfs_dirent is deleted, since sysfs_dirent is only
relevant to directory lookup operations.  It also should not matter if the
system is freeing the struct inode and the struct dentry to save
space.  As long as the file is registered in sysfs, the sysfs_dirent
is not freed.

	Removing sysfs_dirent.s_count results in the removal of other
supporting code, including sysfs_dentry_ops, for a net deletion of
39 lines of code.

	I have only tested this patch by mounting /sys, and running
some "find" commands on it, plugging and unplugging a USB device,
and verifying that the number of entries in sysfs increased and
decreased accordingly.  I am running it on the system on which I
am composing this email.

	By the way, I think there is hope in future for reducing
sizeof(sysfs_dirent) to 32 bytes or less on platforms with 64-bit
pointers too, by some combination of changing s_sibling and s_children
to singly linked lists, eliminating s_dentry, only allocating s_children
for directories, and/or stufffing s_type and s_mode into unused poniter
bits (although I doubt it would come to this last resort, and that might
not be worth the maintainability cost if it did).

                    __     ______________ 
Adam J. Richter        \ /
adam@yggdrasil.com      | g g d r a s i l

--- linux-2.6.10-rc2-bk6/include/linux/sysfs.h	2004-11-17 18:59:17.000000000 +0800
+++ linux/include/linux/sysfs.h	2004-11-23 01:40:35.000000000 +0800
@@ -60,7 +60,6 @@
 };
 
 struct sysfs_dirent {
-	atomic_t		s_count;
 	struct list_head	s_sibling;
 	struct list_head	s_children;
 	void 			* s_element;
diff -u linux-2.6.10-rc2-bk6/fs/sysfs/dir.c linux/fs/sysfs/dir.c
--- linux-2.6.10-rc2-bk6/fs/sysfs/dir.c	2004-11-22 21:15:11.000000000 +0800
+++ linux/fs/sysfs/dir.c	2004-11-23 01:56:18.000000000 +0800
@@ -12,22 +12,6 @@
 
 DECLARE_RWSEM(sysfs_rename_sem);
 
-static void sysfs_d_iput(struct dentry * dentry, struct inode * inode)
-{
-	struct sysfs_dirent * sd = dentry->d_fsdata;
-
-	if (sd) {
-		BUG_ON(sd->s_dentry != dentry);
-		sd->s_dentry = NULL;
-		sysfs_put(sd);
-	}
-	iput(inode);
-}
-
-static struct dentry_operations sysfs_dentry_ops = {
-	.d_iput		= sysfs_d_iput,
-};
-
 /*
  * Allocates a new sysfs_dirent and links it to the parent sysfs_dirent
  */
@@ -41,7 +25,6 @@
 		return NULL;
 
 	memset(sd, 0, sizeof(*sd));
-	atomic_set(&sd->s_count, 1);
 	INIT_LIST_HEAD(&sd->s_children);
 	list_add(&sd->s_sibling, &parent_sd->s_children);
 	sd->s_element = element;
@@ -61,10 +44,8 @@
 	sd->s_mode = mode;
 	sd->s_type = type;
 	sd->s_dentry = dentry;
-	if (dentry) {
-		dentry->d_fsdata = sysfs_get(sd);
-		dentry->d_op = &sysfs_dentry_ops;
-	}
+	if (dentry)
+		dentry->d_fsdata = sd;
 
 	return 0;
 }
@@ -107,7 +88,6 @@
 						SYSFS_DIR);
 			if (!error) {
 				p->d_inode->i_nlink++;
-				(*d)->d_op = &sysfs_dentry_ops;
 				d_rehash(*d);
 			}
 		}
@@ -179,8 +159,7 @@
 		dentry->d_inode->i_size = bin_attr->size;
 		dentry->d_inode->i_fop = &bin_fops;
 	}
-	dentry->d_op = &sysfs_dentry_ops;
-	dentry->d_fsdata = sysfs_get(sd);
+	dentry->d_fsdata = sd;
 	sd->s_dentry = dentry;
 	d_rehash(dentry);
 
@@ -193,8 +172,7 @@
 
 	err = sysfs_create(dentry, S_IFLNK|S_IRWXUGO, init_symlink);
 	if (!err) {
-		dentry->d_op = &sysfs_dentry_ops;
-		dentry->d_fsdata = sysfs_get(sd);
+		dentry->d_fsdata = sd;
 		sd->s_dentry = dentry;
 		d_rehash(dentry);
 	}
@@ -239,7 +217,7 @@
 	d_delete(d);
 	sd = d->d_fsdata;
  	list_del_init(&sd->s_sibling);
-	sysfs_put(sd);
+	release_sysfs_dirent(sd);
 	if (d->d_inode)
 		simple_rmdir(parent->d_inode,d);
 
@@ -282,7 +260,7 @@
 			continue;
 		list_del_init(&sd->s_sibling);
 		sysfs_drop_dentry(sd, dentry);
-		sysfs_put(sd);
+		release_sysfs_dirent(sd);
 	}
 	up(&dentry->d_inode->i_sem);
 
diff -u linux-2.6.10-rc2-bk6/fs/sysfs/inode.c linux/fs/sysfs/inode.c
--- linux-2.6.10-rc2-bk6/fs/sysfs/inode.c	2004-11-17 18:59:13.000000000 +0800
+++ linux/fs/sysfs/inode.c	2004-11-23 01:51:26.000000000 +0800
@@ -151,7 +151,7 @@
 		if (!strcmp(sysfs_get_name(sd), name)) {
 			list_del_init(&sd->s_sibling);
 			sysfs_drop_dentry(sd, dir);
-			sysfs_put(sd);
+			release_sysfs_dirent(sd);
 			break;
 		}
 	}
diff -u linux-2.6.10-rc2-bk6/fs/sysfs/sysfs.h linux/fs/sysfs/sysfs.h
--- linux-2.6.10-rc2-bk6/fs/sysfs/sysfs.h	2004-11-17 18:59:13.000000000 +0800
+++ linux/fs/sysfs/sysfs.h	2004-11-23 01:54:30.000000000 +0800
@@ -76,19 +76,3 @@
 	}
 	kfree(sd);
 }
-
-static inline struct sysfs_dirent * sysfs_get(struct sysfs_dirent * sd)
-{
-	if (sd) {
-		WARN_ON(!atomic_read(&sd->s_count));
-		atomic_inc(&sd->s_count);
-	}
-	return sd;
-}
-
-static inline void sysfs_put(struct sysfs_dirent * sd)
-{
-	if (atomic_dec_and_test(&sd->s_count))
-		release_sysfs_dirent(sd);
-}
-