* [Patch] Do not allocate sysfs_dirent.s_children for non-directories
@ 2004-12-03 3:20 Adam J. Richter
2004-12-30 13:35 ` Maneesh Soni
0 siblings, 1 reply; 3+ messages in thread
From: Adam J. Richter @ 2004-12-03 3:20 UTC (permalink / raw)
To: maneesh; +Cc: akpm, chrisw, greg, linux-kernel, viro
The following patch, against a heavily hacked 2.6.10-rc2-bk15
sysfs tree, removes the s_children field from sysfs_dirent and
creates a new structure just for directories named sysfs_dir, which
embeds a sysfs_dirent and also adds s_children. Directories allocate
a sysfs_dir; non-directories allocate a sysfs_dirent. There are
two separate kmem caches for the different data types.
Not allocating s_children from each non-directory saves
two pointers (8 bytes) for each of the 2573 non-directory nodes
in my sysfs tree, or about 20kB on unswappable memory, but
having another kmem cache probably wastes an average of half
a page in memory fragmentation and then there is are few
bytes from the new code and the additional kmem_cache_t
structure, so I would guess it probably saves about 16kB in
practice.
In the future, I hope to make a similar change for symbolic
links.
By the way, this patch will also make it easier for me to
try to unpin sysfs directories because there are a few other
fields specific to directories that I would want to store
in sysfs_dir.
__ ______________
Adam J. Richter \ /
adam@yggdrasil.com | g g d r a s i l
diff -u5 linux.prev/fs/sysfs/dir.c linux/fs/sysfs/dir.c
--- linux.prev/fs/sysfs/dir.c 2004-12-02 14:51:01.000000000 +0800
+++ linux/fs/sysfs/dir.c 2004-12-03 10:36:20.000000000 +0800
@@ -29,38 +29,46 @@
};
/*
* Allocates a new sysfs_dirent and links it to the parent sysfs_dirent
*/
-static struct sysfs_dirent * sysfs_new_dirent(struct sysfs_dirent * parent_sd,
- void * element)
+static struct sysfs_dirent * sysfs_new_dirent(struct sysfs_dir * parent_sd,
+ void * element, int type)
{
struct sysfs_dirent * sd;
- sd = kmem_cache_alloc(sysfs_dir_cachep, GFP_KERNEL);
- if (!sd)
- return NULL;
+ if (sysfs_type_dir(type)) {
+ struct sysfs_dir * sdir;
+ sdir = kmem_cache_alloc(sysfs_dir_cachep, GFP_KERNEL);
+ if (!sdir)
+ return NULL;
+ INIT_LIST_HEAD(&sdir->s_children);
+ sd = &sdir->s_ent;
+ } else {
+ sd = kmem_cache_alloc(sysfs_dirent_cachep, GFP_KERNEL);
+ if (!sd)
+ return NULL;
+ }
memset(sd, 0, sizeof(*sd));
- INIT_LIST_HEAD(&sd->s_children);
list_add(&sd->s_sibling, &parent_sd->s_children);
sd->s_element = element;
+ sd->s_type = type;
return sd;
}
-int sysfs_make_dirent(struct sysfs_dirent * parent_sd, struct dentry * dentry,
+int sysfs_make_dirent(struct sysfs_dir * parent_sd, struct dentry * dentry,
void * element, umode_t mode, int type)
{
struct sysfs_dirent * sd;
- sd = sysfs_new_dirent(parent_sd, element);
+ sd = sysfs_new_dirent(parent_sd, element, type);
if (!sd)
return -ENOMEM;
sd->s_mode = mode;
- sd->s_type = type;
sd->s_dentry = dentry;
if (dentry) {
dentry->d_fsdata = sd;
dentry->d_op = &sysfs_dentry_ops;
}
@@ -94,17 +102,19 @@
static int create_dir(void *element, struct dentry * p,
const char * n, struct dentry ** d, int type)
{
int error;
umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
+ struct sysfs_dir *parent_sd;
down(&p->d_inode->i_sem);
*d = sysfs_get_dentry(p,n);
if (!IS_ERR(*d)) {
error = sysfs_create(*d, mode, init_dir);
if (!error) {
- error = sysfs_make_dirent(p->d_fsdata, *d, element,
+ parent_sd = dentry_to_sysfs_dir(p);
+ error = sysfs_make_dirent(parent_sd, *d, element,
mode, type);
if (!error) {
p->d_inode->i_nlink++;
(*d)->d_op = &sysfs_dentry_ops;
d_rehash(*d);
@@ -122,11 +132,12 @@
int sysfs_create_subdir(struct kobject * k,
const struct attribute_group *grp,
struct dentry ** d)
{
- return create_dir(grp, k->dentry, grp->name, d, SYSFS_ATTR_GROUP);
+ return create_dir((void*)grp, k->dentry, grp->name, d,
+ SYSFS_ATTR_GROUP);
}
/**
* sysfs_create_dir - create a directory for an object.
* @parent: parent parent object.
@@ -203,11 +214,11 @@
}
static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
struct nameidata *nd)
{
- struct sysfs_dirent * parent_sd = dentry->d_parent->d_fsdata;
+ struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dentry->d_parent);
struct sysfs_dirent * sd;
int err = 0;
list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
if (sd->s_type & SYSFS_NOT_PINNED) {
@@ -267,20 +278,20 @@
*/
void sysfs_remove_dir(struct kobject * kobj)
{
struct dentry * dentry = dget(kobj->dentry);
- struct sysfs_dirent * parent_sd;
+ struct sysfs_dir * parent_sd;
struct sysfs_dirent * sd, * tmp;
if (!dentry)
return;
pr_debug("sysfs %s: removing dir\n",dentry->d_name.name);
down(&dentry->d_inode->i_sem);
- parent_sd = dentry->d_fsdata;
- list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
+ parent_sd = dentry_to_sysfs_dir(dentry);
+ list_for_each_entry_safe(sd,tmp,&parent_sd->s_children,s_sibling) {
if (!sd->s_element || !(sd->s_type & SYSFS_NOT_PINNED))
continue;
sysfs_drop_dentry(sd, dentry);
list_del_init(&sd->s_sibling);
sysfs_put(sd);
@@ -331,14 +342,14 @@
}
static int sysfs_dir_open(struct inode *inode, struct file *file)
{
struct dentry * dentry = file->f_dentry;
- struct sysfs_dirent * parent_sd = dentry->d_fsdata;
+ struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dentry);
down(&dentry->d_inode->i_sem);
- file->private_data = sysfs_new_dirent(parent_sd, NULL);
+ file->private_data = sysfs_new_dirent(parent_sd, NULL, SYSFS_CURSOR);
up(&dentry->d_inode->i_sem);
return file->private_data ? 0 : -ENOMEM;
}
@@ -350,10 +361,13 @@
down(&dentry->d_inode->i_sem);
list_del_init(&cursor->s_sibling);
up(&dentry->d_inode->i_sem);
+ BUG_ON(cursor->s_dentry != NULL);
+ release_sysfs_dirent(cursor);
+
return 0;
}
/* Relationship between s_mode and the DT_xxx types */
static inline unsigned char dt_type(struct sysfs_dirent *sd)
@@ -362,11 +376,11 @@
}
static int sysfs_readdir(struct file * filp, void * dirent, filldir_t filldir)
{
struct dentry *dentry = filp->f_dentry;
- struct sysfs_dirent * parent_sd = dentry->d_fsdata;
+ struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dentry);
struct sysfs_dirent *cursor = filp->private_data;
struct list_head *p, *q = &cursor->s_sibling;
ino_t ino;
int i = filp->f_pos;
@@ -395,10 +409,11 @@
const char * name;
int len;
next = list_entry(p, struct sysfs_dirent,
s_sibling);
+
if (!next->s_element)
continue;
name = sysfs_get_name(next);
len = strlen(name);
@@ -436,11 +451,11 @@
return -EINVAL;
}
if (offset != file->f_pos) {
file->f_pos = offset;
if (file->f_pos >= 2) {
- struct sysfs_dirent *sd = dentry->d_fsdata;
+ struct sysfs_dir *sd = dentry_to_sysfs_dir(dentry);
struct sysfs_dirent *cursor = file->private_data;
struct list_head *p;
loff_t n = file->f_pos - 2;
list_del(&cursor->s_sibling);
diff -u5 linux.prev/fs/sysfs/file.c linux/fs/sysfs/file.c
--- linux.prev/fs/sysfs/file.c 2004-11-07 13:02:53.000000000 +0800
+++ linux/fs/sysfs/file.c 2004-12-02 23:19:56.000000000 +0800
@@ -355,11 +355,11 @@
};
int sysfs_add_file(struct dentry * dir, const struct attribute * attr, int type)
{
- struct sysfs_dirent * parent_sd = dir->d_fsdata;
+ struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dir);
umode_t mode = (attr->mode & S_IALLUGO) | S_IFREG;
int error = 0;
down(&dir->d_inode->i_sem);
error = sysfs_make_dirent(parent_sd, NULL, (void *) attr, mode, type);
diff -u5 linux.prev/fs/sysfs/inode.c linux/fs/sysfs/inode.c
--- linux.prev/fs/sysfs/inode.c 2004-12-02 14:51:01.000000000 +0800
+++ linux/fs/sysfs/inode.c 2004-12-03 00:43:39.000000000 +0800
@@ -146,11 +146,11 @@
}
void sysfs_hash_and_remove(struct dentry * dir, const char * name)
{
struct sysfs_dirent * sd;
- struct sysfs_dirent * parent_sd = dir->d_fsdata;
+ struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dir);
down(&dir->d_inode->i_sem);
list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
if (!sd->s_element)
continue;
diff -u5 linux.prev/fs/sysfs/mount.c linux/fs/sysfs/mount.c
--- linux.prev/fs/sysfs/mount.c 2004-12-02 11:55:02.000000000 +0800
+++ linux/fs/sysfs/mount.c 2004-12-02 23:27:39.000000000 +0800
@@ -15,21 +15,24 @@
#define SYSFS_MAGIC 0x62656572
struct vfsmount *sysfs_mount;
struct super_block * sysfs_sb = NULL;
kmem_cache_t *sysfs_dir_cachep;
+kmem_cache_t *sysfs_dirent_cachep;
static struct super_operations sysfs_ops = {
.statfs = simple_statfs,
.drop_inode = generic_delete_inode,
};
-static struct sysfs_dirent sysfs_root = {
- .s_sibling = LIST_HEAD_INIT(sysfs_root.s_sibling),
+static struct sysfs_dir sysfs_root = {
.s_children = LIST_HEAD_INIT(sysfs_root.s_children),
- .s_element = NULL,
- .s_type = SYSFS_ROOT,
+ .s_ent = {
+ .s_sibling = LIST_HEAD_INIT(sysfs_root.s_ent.s_sibling),
+ .s_element = NULL,
+ .s_type = SYSFS_ROOT,
+ }
};
static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
{
struct inode *inode;
@@ -56,11 +59,11 @@
if (!root) {
pr_debug("%s: could not get root dentry!\n",__FUNCTION__);
iput(inode);
return -ENOMEM;
}
- root->d_fsdata = &sysfs_root;
+ root->d_fsdata = &sysfs_root.s_ent;
sb->s_root = root;
return 0;
}
static struct super_block *sysfs_get_sb(struct file_system_type *fs_type,
@@ -77,16 +80,22 @@
int __init sysfs_init(void)
{
int err = -ENOMEM;
- sysfs_dir_cachep = kmem_cache_create("sysfs_dir_cache",
- sizeof(struct sysfs_dirent),
+ sysfs_dir_cachep = kmem_cache_create("sysfs_dir",
+ sizeof(struct sysfs_dir),
0, 0, NULL, NULL);
if (!sysfs_dir_cachep)
goto out;
+ sysfs_dirent_cachep = kmem_cache_create("sysfs_dirent",
+ sizeof(struct sysfs_dirent),
+ 0, 0, NULL, NULL);
+ if (!sysfs_dirent_cachep)
+ goto out_no_dirent_cachep;
+
err = register_filesystem(&sysfs_fs_type);
if (!err) {
sysfs_mount = kern_mount(&sysfs_fs_type);
if (IS_ERR(sysfs_mount)) {
printk(KERN_ERR "sysfs: could not mount!\n");
@@ -97,8 +106,10 @@
} else
goto out_err;
out:
return err;
out_err:
+ kmem_cache_destroy(sysfs_dirent_cachep);
+out_no_dirent_cachep:
kmem_cache_destroy(sysfs_dir_cachep);
goto out;
}
diff -u5 linux.prev/fs/sysfs/symlink.c linux/fs/sysfs/symlink.c
--- linux.prev/fs/sysfs/symlink.c 2004-11-07 13:02:53.000000000 +0800
+++ linux/fs/sysfs/symlink.c 2004-12-02 23:57:01.000000000 +0800
@@ -43,11 +43,11 @@
}
}
static int sysfs_add_link(struct dentry * parent, char * name, struct kobject * target)
{
- struct sysfs_dirent * parent_sd = parent->d_fsdata;
+ struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(parent);
struct sysfs_symlink * sl;
int error = 0;
error = -ENOMEM;
sl = kmalloc(sizeof(*sl), GFP_KERNEL);
diff -u5 linux.prev/fs/sysfs/sysfs.h linux/fs/sysfs/sysfs.h
--- linux.prev/fs/sysfs/sysfs.h 2004-12-02 15:29:39.000000000 +0800
+++ linux/fs/sysfs/sysfs.h 2004-12-03 00:51:44.000000000 +0800
@@ -1,32 +1,38 @@
#include <linux/time.h>
#include <linux/sysfs.h>
+#define SYSFS_CURSOR 0
#define SYSFS_ROOT 0x0001
#define SYSFS_DIR 0x0002
#define SYSFS_KOBJ_ATTR 0x0004
#define SYSFS_KOBJ_BIN_ATTR 0x0008
#define SYSFS_ATTR_GROUP 0x0010
#define SYSFS_KOBJ_LINK 0x0020
#define SYSFS_NOT_PINNED (SYSFS_KOBJ_ATTR | SYSFS_KOBJ_BIN_ATTR | SYSFS_KOBJ_LINK)
struct sysfs_dirent {
struct list_head s_sibling;
- struct list_head s_children;
void * s_element;
- int s_type;
+ unsigned short s_type;
umode_t s_mode;
struct dentry * s_dentry;
};
+struct sysfs_dir {
+ struct list_head s_children;
+ struct sysfs_dirent s_ent;
+};
+
extern struct vfsmount * sysfs_mount;
extern kmem_cache_t *sysfs_dir_cachep;
+extern kmem_cache_t *sysfs_dirent_cachep;
extern struct inode * sysfs_new_inode(mode_t mode);
extern int sysfs_create(struct dentry *, int mode, int (*init)(struct inode *));
-extern int sysfs_make_dirent(struct sysfs_dirent *, struct dentry *, void *,
+extern int sysfs_make_dirent(struct sysfs_dir *, struct dentry *, void *,
umode_t, int);
extern struct dentry * sysfs_get_dentry(struct dentry *, const char *);
extern int sysfs_add_file(struct dentry *, const struct attribute *, int);
extern void sysfs_hash_and_remove(struct dentry * dir, const char * name);
@@ -48,10 +54,29 @@
struct sysfs_symlink {
char * link_name;
struct kobject * target_kobj;
};
+static inline int sysfs_type_dir(int s_type)
+{
+ return (s_type == SYSFS_DIR ||
+ s_type == SYSFS_ATTR_GROUP ||
+ s_type == SYSFS_ROOT);
+}
+
+static inline struct sysfs_dir * to_sysfs_dir(struct sysfs_dirent *ent)
+{
+ BUG_ON(!sysfs_type_dir(ent->s_type));
+ return container_of(ent, struct sysfs_dir, s_ent);
+}
+
+static inline struct sysfs_dir *dentry_to_sysfs_dir(struct dentry * dentry)
+{
+ struct sysfs_dirent * sd = dentry->d_fsdata;
+ return to_sysfs_dir(sd);
+}
+
static inline struct kobject * to_kobj(struct dentry * dentry)
{
struct sysfs_dirent * sd = dentry->d_fsdata;
if (sd->s_type == SYSFS_ATTR_GROUP)
@@ -101,11 +126,14 @@
struct sysfs_symlink * sl = sd->s_element;
kfree(sl->link_name);
kobject_put(sl->target_kobj);
kfree(sl);
}
- kmem_cache_free(sysfs_dir_cachep, sd);
+ if (sysfs_type_dir(sd->s_type))
+ kmem_cache_free(sysfs_dir_cachep, to_sysfs_dir(sd));
+ else
+ kmem_cache_free(sysfs_dirent_cachep, sd);
}
static inline void sysfs_put(struct sysfs_dirent * sd)
{
if (list_empty(&sd->s_sibling) && sd->s_dentry == NULL)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Patch] Do not allocate sysfs_dirent.s_children for non-directories
2004-12-03 3:20 [Patch] Do not allocate sysfs_dirent.s_children for non-directories Adam J. Richter
@ 2004-12-30 13:35 ` Maneesh Soni
0 siblings, 0 replies; 3+ messages in thread
From: Maneesh Soni @ 2004-12-30 13:35 UTC (permalink / raw)
To: Adam J. Richter; +Cc: akpm, chrisw, greg, linux-kernel, viro
On Thu, Dec 02, 2004 at 07:20:35PM -0800, Adam J. Richter wrote:
> The following patch, against a heavily hacked 2.6.10-rc2-bk15
> sysfs tree, removes the s_children field from sysfs_dirent and
> creates a new structure just for directories named sysfs_dir, which
> embeds a sysfs_dirent and also adds s_children. Directories allocate
> a sysfs_dir; non-directories allocate a sysfs_dirent. There are
> two separate kmem caches for the different data types.
>
> Not allocating s_children from each non-directory saves
> two pointers (8 bytes) for each of the 2573 non-directory nodes
> in my sysfs tree, or about 20kB on unswappable memory, but
> having another kmem cache probably wastes an average of half
> a page in memory fragmentation and then there is are few
> bytes from the new code and the additional kmem_cache_t
> structure, so I would guess it probably saves about 16kB in
> practice.
>
> In the future, I hope to make a similar change for symbolic
> links.
>
> By the way, this patch will also make it easier for me to
> try to unpin sysfs directories because there are a few other
> fields specific to directories that I would want to store
> in sysfs_dir.
>
Apart from a couple of diff'ing related comments, I feel the code looks
some what complicated. I think we can directly link sysfs_dir to
directory dentries and sysfs_dirent to non-directory dentries instead
of always linking sysfs_dirent to d_fsdata. To differentiate between the
two types of structures linked to dentry's d_fsdata field, we can use
S_ISDIR(dentry->d_inode->i_mode). This will avoid using container_of() and
the dentry_to_sysfs_dir() conversions.
Most of the places we may not need to find what type of struct d_fsdata points
to. Like in sysfs_make_dirent(), first param has to be sysfs_dir as the parent
dentry corresponds to a sysfs directory.
In sysfs_lookup() also, we know parent dentry corresponds to sysfs directory
so dentry's d_fsdata will point to sysfs_dir instead of sysfs_dirent.
>
> diff -u5 linux.prev/fs/sysfs/dir.c linux/fs/sysfs/dir.c
> --- linux.prev/fs/sysfs/dir.c 2004-12-02 14:51:01.000000000 +0800
> +++ linux/fs/sysfs/dir.c 2004-12-03 10:36:20.000000000 +0800
> @@ -29,38 +29,46 @@
> };
>
> /*
> * Allocates a new sysfs_dirent and links it to the parent sysfs_dirent
> */
> -static struct sysfs_dirent * sysfs_new_dirent(struct sysfs_dirent * parent_sd,
> - void * element)
> +static struct sysfs_dirent * sysfs_new_dirent(struct sysfs_dir * parent_sd,
> + void * element, int type)
> {
> struct sysfs_dirent * sd;
>
> - sd = kmem_cache_alloc(sysfs_dir_cachep, GFP_KERNEL);
> - if (!sd)
> - return NULL;
> + if (sysfs_type_dir(type)) {
> + struct sysfs_dir * sdir;
> + sdir = kmem_cache_alloc(sysfs_dir_cachep, GFP_KERNEL);
> + if (!sdir)
> + return NULL;
> + INIT_LIST_HEAD(&sdir->s_children);
> + sd = &sdir->s_ent;
> + } else {
> + sd = kmem_cache_alloc(sysfs_dirent_cachep, GFP_KERNEL);
> + if (!sd)
> + return NULL;
> + }
>
> memset(sd, 0, sizeof(*sd));
> - INIT_LIST_HEAD(&sd->s_children);
> list_add(&sd->s_sibling, &parent_sd->s_children);
> sd->s_element = element;
> + sd->s_type = type;
>
> return sd;
> }
>
> -int sysfs_make_dirent(struct sysfs_dirent * parent_sd, struct dentry * dentry,
> +int sysfs_make_dirent(struct sysfs_dir * parent_sd, struct dentry * dentry,
> void * element, umode_t mode, int type)
> {
> struct sysfs_dirent * sd;
>
> - sd = sysfs_new_dirent(parent_sd, element);
> + sd = sysfs_new_dirent(parent_sd, element, type);
> if (!sd)
> return -ENOMEM;
>
> sd->s_mode = mode;
> - sd->s_type = type;
> sd->s_dentry = dentry;
> if (dentry) {
> dentry->d_fsdata = sd;
> dentry->d_op = &sysfs_dentry_ops;
> }
> @@ -94,17 +102,19 @@
> static int create_dir(void *element, struct dentry * p,
> const char * n, struct dentry ** d, int type)
> {
> int error;
> umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
> + struct sysfs_dir *parent_sd;
>
> down(&p->d_inode->i_sem);
> *d = sysfs_get_dentry(p,n);
> if (!IS_ERR(*d)) {
> error = sysfs_create(*d, mode, init_dir);
> if (!error) {
> - error = sysfs_make_dirent(p->d_fsdata, *d, element,
> + parent_sd = dentry_to_sysfs_dir(p);
> + error = sysfs_make_dirent(parent_sd, *d, element,
> mode, type);
> if (!error) {
> p->d_inode->i_nlink++;
> (*d)->d_op = &sysfs_dentry_ops;
> d_rehash(*d);
> @@ -122,11 +132,12 @@
>
> int sysfs_create_subdir(struct kobject * k,
> const struct attribute_group *grp,
> struct dentry ** d)
> {
> - return create_dir(grp, k->dentry, grp->name, d, SYSFS_ATTR_GROUP);
> + return create_dir((void*)grp, k->dentry, grp->name, d,
> + SYSFS_ATTR_GROUP);
> }
>
> /**
> * sysfs_create_dir - create a directory for an object.
> * @parent: parent parent object.
> @@ -203,11 +214,11 @@
> }
>
> static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
> struct nameidata *nd)
> {
> - struct sysfs_dirent * parent_sd = dentry->d_parent->d_fsdata;
> + struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dentry->d_parent);
> struct sysfs_dirent * sd;
> int err = 0;
>
> list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
> if (sd->s_type & SYSFS_NOT_PINNED) {
> @@ -267,20 +278,20 @@
> */
>
> void sysfs_remove_dir(struct kobject * kobj)
> {
> struct dentry * dentry = dget(kobj->dentry);
> - struct sysfs_dirent * parent_sd;
> + struct sysfs_dir * parent_sd;
> struct sysfs_dirent * sd, * tmp;
>
> if (!dentry)
> return;
>
> pr_debug("sysfs %s: removing dir\n",dentry->d_name.name);
> down(&dentry->d_inode->i_sem);
> - parent_sd = dentry->d_fsdata;
> - list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
> + parent_sd = dentry_to_sysfs_dir(dentry);
> + list_for_each_entry_safe(sd,tmp,&parent_sd->s_children,s_sibling) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
removing space before each parameter is not saving anything but creating
extra diff.
> if (!sd->s_element || !(sd->s_type & SYSFS_NOT_PINNED))
> continue;
> sysfs_drop_dentry(sd, dentry);
> list_del_init(&sd->s_sibling);
> sysfs_put(sd);
> @@ -331,14 +342,14 @@
> }
>
> static int sysfs_dir_open(struct inode *inode, struct file *file)
> {
> struct dentry * dentry = file->f_dentry;
> - struct sysfs_dirent * parent_sd = dentry->d_fsdata;
> + struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dentry);
>
> down(&dentry->d_inode->i_sem);
> - file->private_data = sysfs_new_dirent(parent_sd, NULL);
> + file->private_data = sysfs_new_dirent(parent_sd, NULL, SYSFS_CURSOR);
> up(&dentry->d_inode->i_sem);
>
> return file->private_data ? 0 : -ENOMEM;
>
> }
> @@ -350,10 +361,13 @@
>
> down(&dentry->d_inode->i_sem);
> list_del_init(&cursor->s_sibling);
> up(&dentry->d_inode->i_sem);
>
> + BUG_ON(cursor->s_dentry != NULL);
> + release_sysfs_dirent(cursor);
> +
> return 0;
> }
>
> /* Relationship between s_mode and the DT_xxx types */
> static inline unsigned char dt_type(struct sysfs_dirent *sd)
> @@ -362,11 +376,11 @@
> }
>
> static int sysfs_readdir(struct file * filp, void * dirent, filldir_t filldir)
> {
> struct dentry *dentry = filp->f_dentry;
> - struct sysfs_dirent * parent_sd = dentry->d_fsdata;
> + struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dentry);
> struct sysfs_dirent *cursor = filp->private_data;
> struct list_head *p, *q = &cursor->s_sibling;
> ino_t ino;
> int i = filp->f_pos;
>
> @@ -395,10 +409,11 @@
> const char * name;
> int len;
>
> next = list_entry(p, struct sysfs_dirent,
> s_sibling);
> +
> if (!next->s_element)
> continue;
>
> name = sysfs_get_name(next);
> len = strlen(name);
^^^^^^^^^^^^^^^^^^
unnecessarry increase in patch size due to extra blank line.
> @@ -436,11 +451,11 @@
> return -EINVAL;
> }
> if (offset != file->f_pos) {
> file->f_pos = offset;
> if (file->f_pos >= 2) {
> - struct sysfs_dirent *sd = dentry->d_fsdata;
> + struct sysfs_dir *sd = dentry_to_sysfs_dir(dentry);
> struct sysfs_dirent *cursor = file->private_data;
> struct list_head *p;
> loff_t n = file->f_pos - 2;
>
> list_del(&cursor->s_sibling);
> diff -u5 linux.prev/fs/sysfs/file.c linux/fs/sysfs/file.c
> --- linux.prev/fs/sysfs/file.c 2004-11-07 13:02:53.000000000 +0800
> +++ linux/fs/sysfs/file.c 2004-12-02 23:19:56.000000000 +0800
> @@ -355,11 +355,11 @@
> };
>
>
> int sysfs_add_file(struct dentry * dir, const struct attribute * attr, int type)
> {
> - struct sysfs_dirent * parent_sd = dir->d_fsdata;
> + struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dir);
> umode_t mode = (attr->mode & S_IALLUGO) | S_IFREG;
> int error = 0;
>
> down(&dir->d_inode->i_sem);
> error = sysfs_make_dirent(parent_sd, NULL, (void *) attr, mode, type);
> diff -u5 linux.prev/fs/sysfs/inode.c linux/fs/sysfs/inode.c
> --- linux.prev/fs/sysfs/inode.c 2004-12-02 14:51:01.000000000 +0800
> +++ linux/fs/sysfs/inode.c 2004-12-03 00:43:39.000000000 +0800
> @@ -146,11 +146,11 @@
> }
>
> void sysfs_hash_and_remove(struct dentry * dir, const char * name)
> {
> struct sysfs_dirent * sd;
> - struct sysfs_dirent * parent_sd = dir->d_fsdata;
> + struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(dir);
>
> down(&dir->d_inode->i_sem);
> list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
> if (!sd->s_element)
> continue;
> diff -u5 linux.prev/fs/sysfs/mount.c linux/fs/sysfs/mount.c
> --- linux.prev/fs/sysfs/mount.c 2004-12-02 11:55:02.000000000 +0800
> +++ linux/fs/sysfs/mount.c 2004-12-02 23:27:39.000000000 +0800
> @@ -15,21 +15,24 @@
> #define SYSFS_MAGIC 0x62656572
>
> struct vfsmount *sysfs_mount;
> struct super_block * sysfs_sb = NULL;
> kmem_cache_t *sysfs_dir_cachep;
> +kmem_cache_t *sysfs_dirent_cachep;
>
> static struct super_operations sysfs_ops = {
> .statfs = simple_statfs,
> .drop_inode = generic_delete_inode,
> };
>
> -static struct sysfs_dirent sysfs_root = {
> - .s_sibling = LIST_HEAD_INIT(sysfs_root.s_sibling),
> +static struct sysfs_dir sysfs_root = {
> .s_children = LIST_HEAD_INIT(sysfs_root.s_children),
> - .s_element = NULL,
> - .s_type = SYSFS_ROOT,
> + .s_ent = {
> + .s_sibling = LIST_HEAD_INIT(sysfs_root.s_ent.s_sibling),
> + .s_element = NULL,
> + .s_type = SYSFS_ROOT,
> + }
> };
>
> static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
> {
> struct inode *inode;
> @@ -56,11 +59,11 @@
> if (!root) {
> pr_debug("%s: could not get root dentry!\n",__FUNCTION__);
> iput(inode);
> return -ENOMEM;
> }
> - root->d_fsdata = &sysfs_root;
> + root->d_fsdata = &sysfs_root.s_ent;
> sb->s_root = root;
> return 0;
> }
>
> static struct super_block *sysfs_get_sb(struct file_system_type *fs_type,
> @@ -77,16 +80,22 @@
>
> int __init sysfs_init(void)
> {
> int err = -ENOMEM;
>
> - sysfs_dir_cachep = kmem_cache_create("sysfs_dir_cache",
> - sizeof(struct sysfs_dirent),
> + sysfs_dir_cachep = kmem_cache_create("sysfs_dir",
> + sizeof(struct sysfs_dir),
> 0, 0, NULL, NULL);
> if (!sysfs_dir_cachep)
> goto out;
>
> + sysfs_dirent_cachep = kmem_cache_create("sysfs_dirent",
> + sizeof(struct sysfs_dirent),
> + 0, 0, NULL, NULL);
> + if (!sysfs_dirent_cachep)
> + goto out_no_dirent_cachep;
> +
> err = register_filesystem(&sysfs_fs_type);
> if (!err) {
> sysfs_mount = kern_mount(&sysfs_fs_type);
> if (IS_ERR(sysfs_mount)) {
> printk(KERN_ERR "sysfs: could not mount!\n");
> @@ -97,8 +106,10 @@
> } else
> goto out_err;
> out:
> return err;
> out_err:
> + kmem_cache_destroy(sysfs_dirent_cachep);
> +out_no_dirent_cachep:
> kmem_cache_destroy(sysfs_dir_cachep);
> goto out;
> }
> diff -u5 linux.prev/fs/sysfs/symlink.c linux/fs/sysfs/symlink.c
> --- linux.prev/fs/sysfs/symlink.c 2004-11-07 13:02:53.000000000 +0800
> +++ linux/fs/sysfs/symlink.c 2004-12-02 23:57:01.000000000 +0800
> @@ -43,11 +43,11 @@
> }
> }
>
> static int sysfs_add_link(struct dentry * parent, char * name, struct kobject * target)
> {
> - struct sysfs_dirent * parent_sd = parent->d_fsdata;
> + struct sysfs_dir * parent_sd = dentry_to_sysfs_dir(parent);
> struct sysfs_symlink * sl;
> int error = 0;
>
> error = -ENOMEM;
> sl = kmalloc(sizeof(*sl), GFP_KERNEL);
> diff -u5 linux.prev/fs/sysfs/sysfs.h linux/fs/sysfs/sysfs.h
> --- linux.prev/fs/sysfs/sysfs.h 2004-12-02 15:29:39.000000000 +0800
> +++ linux/fs/sysfs/sysfs.h 2004-12-03 00:51:44.000000000 +0800
> @@ -1,32 +1,38 @@
> #include <linux/time.h>
> #include <linux/sysfs.h>
>
> +#define SYSFS_CURSOR 0
> #define SYSFS_ROOT 0x0001
> #define SYSFS_DIR 0x0002
> #define SYSFS_KOBJ_ATTR 0x0004
> #define SYSFS_KOBJ_BIN_ATTR 0x0008
> #define SYSFS_ATTR_GROUP 0x0010
> #define SYSFS_KOBJ_LINK 0x0020
> #define SYSFS_NOT_PINNED (SYSFS_KOBJ_ATTR | SYSFS_KOBJ_BIN_ATTR | SYSFS_KOBJ_LINK)
>
> struct sysfs_dirent {
> struct list_head s_sibling;
> - struct list_head s_children;
> void * s_element;
> - int s_type;
> + unsigned short s_type;
> umode_t s_mode;
> struct dentry * s_dentry;
> };
>
> +struct sysfs_dir {
> + struct list_head s_children;
> + struct sysfs_dirent s_ent;
> +};
> +
> extern struct vfsmount * sysfs_mount;
> extern kmem_cache_t *sysfs_dir_cachep;
> +extern kmem_cache_t *sysfs_dirent_cachep;
>
> extern struct inode * sysfs_new_inode(mode_t mode);
> extern int sysfs_create(struct dentry *, int mode, int (*init)(struct inode *));
>
> -extern int sysfs_make_dirent(struct sysfs_dirent *, struct dentry *, void *,
> +extern int sysfs_make_dirent(struct sysfs_dir *, struct dentry *, void *,
> umode_t, int);
> extern struct dentry * sysfs_get_dentry(struct dentry *, const char *);
>
> extern int sysfs_add_file(struct dentry *, const struct attribute *, int);
> extern void sysfs_hash_and_remove(struct dentry * dir, const char * name);
> @@ -48,10 +54,29 @@
> struct sysfs_symlink {
> char * link_name;
> struct kobject * target_kobj;
> };
>
> +static inline int sysfs_type_dir(int s_type)
> +{
> + return (s_type == SYSFS_DIR ||
> + s_type == SYSFS_ATTR_GROUP ||
> + s_type == SYSFS_ROOT);
> +}
> +
> +static inline struct sysfs_dir * to_sysfs_dir(struct sysfs_dirent *ent)
> +{
> + BUG_ON(!sysfs_type_dir(ent->s_type));
> + return container_of(ent, struct sysfs_dir, s_ent);
> +}
> +
> +static inline struct sysfs_dir *dentry_to_sysfs_dir(struct dentry * dentry)
> +{
> + struct sysfs_dirent * sd = dentry->d_fsdata;
> + return to_sysfs_dir(sd);
> +}
> +
> static inline struct kobject * to_kobj(struct dentry * dentry)
> {
> struct sysfs_dirent * sd = dentry->d_fsdata;
>
> if (sd->s_type == SYSFS_ATTR_GROUP)
> @@ -101,11 +126,14 @@
> struct sysfs_symlink * sl = sd->s_element;
> kfree(sl->link_name);
> kobject_put(sl->target_kobj);
> kfree(sl);
> }
> - kmem_cache_free(sysfs_dir_cachep, sd);
> + if (sysfs_type_dir(sd->s_type))
> + kmem_cache_free(sysfs_dir_cachep, to_sysfs_dir(sd));
> + else
> + kmem_cache_free(sysfs_dirent_cachep, sd);
> }
>
> static inline void sysfs_put(struct sysfs_dirent * sd)
> {
> if (list_empty(&sd->s_sibling) && sd->s_dentry == NULL)
>
--
Maneesh Soni
Linux Technology Center,
IBM India Software Labs,
Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044990
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Patch] Do not allocate sysfs_dirent.s_children for non-directories
@ 2004-12-30 17:33 Adam J. Richter
0 siblings, 0 replies; 3+ messages in thread
From: Adam J. Richter @ 2004-12-30 17:33 UTC (permalink / raw)
To: maneesh; +Cc: akpm, chrisw, greg, linux-kernel, viro
On Thu, 30 Dec 2004 19:05:29 +0530, Maneesh Soni wrote:
>On Thu, Dec 02, 2004 at 07:20:35PM -0800, Adam J. Richter wrote:
>> The following patch, against a heavily hacked 2.6.10-rc2-bk15
>> sysfs tree, removes the s_children field from sysfs_dirent and
>> creates a new structure just for directories named sysfs_dir, which
>> embeds a sysfs_dirent and also adds s_children. Directories allocate
>> a sysfs_dir; non-directories allocate a sysfs_dirent. There are
>> two separate kmem caches for the different data types.
>>
>> Not allocating s_children from each non-directory saves
>> two pointers (8 bytes) for each of the 2573 non-directory nodes
>> in my sysfs tree, or about 20kB on unswappable memory, but
>> having another kmem cache probably wastes an average of half
>> a page in memory fragmentation and then there is are few
>> bytes from the new code and the additional kmem_cache_t
>> structure, so I would guess it probably saves about 16kB in
>> practice.
>>
>> In the future, I hope to make a similar change for symbolic
>> links.
>>
>> By the way, this patch will also make it easier for me to
>> try to unpin sysfs directories because there are a few other
>> fields specific to directories that I would want to store
>> in sysfs_dir.
>>
>Apart from a couple of diff'ing related comments, I feel the code looks
>some what complicated. I think we can directly link sysfs_dir to
>directory dentries and sysfs_dirent to non-directory dentries instead
>of always linking sysfs_dirent to d_fsdata. To differentiate between the
>two types of structures linked to dentry's d_fsdata field, we can use
>S_ISDIR(dentry->d_inode->i_mode). This will avoid using container_of() and
>the dentry_to_sysfs_dir() conversions.
>Most of the places we may not need to find what type of struct d_fsdata points
>to. Like in sysfs_make_dirent(), first param has to be sysfs_dir as the parent
>dentry corresponds to a sysfs directory.
>In sysfs_lookup() also, we know parent dentry corresponds to sysfs directory
>so dentry's d_fsdata will point to sysfs_dir instead of sysfs_dirent.
In the future, I want to make a change so that attributes in
a named struct attribute_group do not each have a struct dirent.
In that case, it is possible that the attribute_group will only need
a struct sysfs_dirent, not a struct sysfs_dir.
__ ______________
Adam J. Richter \ /
adam@yggdrasil.com | g g d r a s i l
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2004-12-30 17:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-12-03 3:20 [Patch] Do not allocate sysfs_dirent.s_children for non-directories Adam J. Richter
2004-12-30 13:35 ` Maneesh Soni
2004-12-30 17:33 Adam J. Richter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).