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 \ /
[email protected] | 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);
-}
-
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: [email protected]
Phone: 1-512-838-1896 Fax:
T/L : 6781896
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 \ /
[email protected] | g g d r a s i l