sysfs_remove_dir

PATCH In review Maneesh Soni 3 participants 5 messages Sep 15, 2003 – Sep 23, 2003

Subsystems (2)

Networking Driver core

lore

Share & tools

lore

Maneesh Soni

author

2003-09-15 10:21 UTC #0 lore raw

PATCH 1/1 sysfs_remove_dir

Hi Pat,

sysfs_remove_dir() does not remove the contents of subdirs corresponding to the attribute groups of a kobject. The following patch fixes this by first removing the subdir contents and then removing thus emptied subdirs along with the other attribute files of the kobject and plugs the memory leakage resulting from orphan dentries.

I tested it by inserting and removing "dummy.o" network module and verifying that dentires corresponding to "statistics" attribute group are removed.

Please comment.

Thanks
Maneesh

o sysfs_remove_dir() has to remove the files in the subdirs (corresponding to the attribute groups) and then remove such empty subdirs along with the other attribute files for the given kobject. The following patch does this assuming that there are/will be no attribute sub-groups.

 fs/sysfs/dir.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_remove_dir-fix +17 -5

diff -puN fs/sysfs/dir.c~sysfs_remove_dir-fix fs/sysfs/dir.c
@@ -119,23 +119,30 @@ void sysfs_remove_dir(struct kobject * k
 {
 	struct list_head * node;
 	struct dentry * dentry = dget(kobj->dentry);
+	struct dentry * parent;
 
 	if (!dentry)
 		return;
 
 	pr_debug("sysfs %s: removing dir\n",dentry->d_name.name);
-	down(&dentry->d_inode->i_sem);
 
+	parent = dentry;
+	down(&parent->d_inode->i_sem);
 	spin_lock(&dcache_lock);
-	node = dentry->d_subdirs.next;
-	while (node != &dentry->d_subdirs) {
-		struct dentry * d = list_entry(node,struct dentry,d_child);
+repeat:
+	node = parent->d_subdirs.next;
+	while (node != &parent->d_subdirs) {
+		struct dentry * d = list_entry(node, struct dentry, d_child);
 		list_del_init(node);
 
 		pr_debug(" o %s (%d): ",d->d_name.name,atomic_read(&d->d_count));
 		if (d->d_inode) {
 			d = dget_locked(d);
 			pr_debug("removing");
+			if (!list_empty(&d->d_subdirs)) {
+				parent = d;
+				goto repeat;
+			}
 
 			/**
 			 * Unlink and unhash.

@@ -147,7 +154,12 @@ void sysfs_remove_dir(struct kobject * k
 			spin_lock(&dcache_lock);
 		}
 		pr_debug(" done\n");
-		node = dentry->d_subdirs.next;
+		node = parent->d_subdirs.next;
+	}
+
+	if (!list_empty(&dentry->d_subdirs)) {
+		parent = dentry;
+		goto repeat;
 	}
 	list_del_init(&dentry->d_child);
 	spin_unlock(&dcache_lock);

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696

Hanna Linder

2003-09-15 21:34 UTC #1 lore raw

Maneesh, Pat,

Just wanted to let you know I ran this patch on 2.6.0-test5 with my input class patch and there were no problems. Still working on my patch though.

Hanna

Quoted 97 lines · --On Monday, September 15, 2003 03:51:28 PM +0530 Maneesh Soni…

Hi Pat,
sysfs_remove_dir() does not remove the contents of subdirs corresponding to the attribute groups of a kobject. The following patch fixes this by first removing the subdir contents and then removing thus emptied subdirs along with the other attribute files of the kobject and plugs the memory leakage resulting from orphan dentries.
I tested it by inserting and removing "dummy.o" network module and verifying that dentires corresponding to "statistics" attribute group are removed.
Please comment.
Thanks
Maneesh
o sysfs_remove_dir() has to remove the files in the subdirs (corresponding to the attribute groups) and then remove such empty subdirs along with the other attribute files for the given kobject. The following patch does this assuming that there are/will be no attribute sub-groups.
 fs/sysfs/dir.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)
diff -puN fs/sysfs/dir.c~sysfs_remove_dir-fix fs/sysfs/dir.c
fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs_remove_dir-fix +17 -5
@@ -119,23 +119,30 @@ void sysfs_remove_dir(struct kobject * k
 {
 	struct list_head * node;
 	struct dentry * dentry = dget(kobj->dentry);
+	struct dentry * parent;
 
 	if (!dentry)
 		return;
 
 	pr_debug("sysfs %s: removing dir\n",dentry->d_name.name);
-	down(&dentry->d_inode->i_sem);
 
+	parent = dentry;
+	down(&parent->d_inode->i_sem);
 	spin_lock(&dcache_lock);
-	node = dentry->d_subdirs.next;
-	while (node != &dentry->d_subdirs) {
-		struct dentry * d = list_entry(node,struct dentry,d_child);
+repeat:
+	node = parent->d_subdirs.next;
+	while (node != &parent->d_subdirs) {
+		struct dentry * d = list_entry(node, struct dentry, d_child);
 		list_del_init(node);
 
 		pr_debug(" o %s (%d): ",d->d_name.name,atomic_read(&d->d_count));
 		if (d->d_inode) {
 			d = dget_locked(d);
 			pr_debug("removing");
+			if (!list_empty(&d->d_subdirs)) {
+				parent = d;
+				goto repeat;
+			}
 
 			/**
 			 * Unlink and unhash.
@@ -147,7 +154,12 @@ void sysfs_remove_dir(struct kobject * k
 			spin_lock(&dcache_lock);
 		}
 		pr_debug(" done\n");
-		node = dentry->d_subdirs.next;
+		node = parent->d_subdirs.next;
+	}
+
+	if (!list_empty(&dentry->d_subdirs)) {
+		parent = dentry;
+		goto repeat;
 	}
 	list_del_init(&dentry->d_child);
 	spin_unlock(&dcache_lock);
_
Signature · 11 lines
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patrick Mochel

2003-09-19 23:07 UTC #2 lore raw

In reply to Maneesh Soni

Quoted 9 lines

sysfs_remove_dir() does not remove the contents of subdirs corresponding to the attribute groups of a kobject. The following patch fixes this by first removing the subdir contents and then removing thus emptied subdirs along with the other attribute files of the kobject and plugs the memory leakage resulting from orphan dentries.
I tested it by inserting and removing "dummy.o" network module and verifying that dentires corresponding to "statistics" attribute group are removed.

Sorry it's taken a while to reply, but I even now haven't had a chance to look that deep into this problem. I will say that this is the wrong approach to the problem.

Upon initial inspection, it looks like it will do the right thing in all cases, except when a parent is removed before a child is (but we don't technically support that now).

However, I'd like to move away from the automatic cleanup that sysfs was doing. attribute groups make it easier to clean up all the files that are created for an object when the object is removed. I would like to see that removal call inserted where necessary, instead of adding this complication to the sysfs core.

I intend to remove the automatic cleanup in sysfs_remove_dir(), but haven't gotten around to it..

Thanks,

Pat

Maneesh Soni

author

2003-09-22 04:58 UTC #3 lore raw

Quoted 18 lines · Patrick Mochel

Quoted 9 lines
sysfs_remove_dir() does not remove the contents of subdirs corresponding to the attribute groups of a kobject. The following patch fixes this by first removing the subdir contents and then removing thus emptied subdirs along with the other attribute files of the kobject and plugs the memory leakage resulting from orphan dentries.
I tested it by inserting and removing "dummy.o" network module and verifying that dentires corresponding to "statistics" attribute group are removed.
Sorry it's taken a while to reply, but I even now haven't had a chance to look that deep into this problem. I will say that this is the wrong approach to the problem.
Upon initial inspection, it looks like it will do the right thing in all cases, except when a parent is removed before a child is (but we don't technically support that now).

Quoted 9 lines

However, I'd like to move away from the automatic cleanup that sysfs was doing. attribute groups make it easier to clean up all the files that are created for an object when the object is removed. I would like to see that removal call inserted where necessary, instead of adding this complication to the sysfs core.
I intend to remove the automatic cleanup in sysfs_remove_dir(), but haven't gotten around to it..

ok.. my intention was not to change callers for removing attribute group and keep the changes within sysfs. But if you prefer otherway (I too :-)), please see the patch below. As of now I can see only netdev not releasing attribute group. I guess power subsystem don't need to remove attribute groups created in pm_init() and pm_disk_init() as these don't unregister from sysfs.

Thanks
Maneesh

o attribute group should be removed while unregistering from sysfs. netdev does not do this and leaks dentries belonging to the attribute group. The following patch removes the attribute group when netdev is unregistered.

 net/core/dev.c       |    3 ++-
 net/core/net-sysfs.c |   25 +++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 3 deletions(-)

net/core/net-sysfs.c renamed from net/core/net-sysfs.c~sysfs-dentry-leak-fix +23 -2

diff -puN net/core/net-sysfs.c~sysfs-dentry-leak-fix net/core/net-sysfs.c
@@ -383,6 +383,21 @@ static struct class net_class = {
 #endif
 };
 
+void netdev_unregister_sysfs(struct net_device * net)
+{
+	struct class_device * class_dev = &(net->class_dev);
+
+	if (net->get_stats)
+		sysfs_remove_group(&class_dev->kobj, &netstat_group);
+
+#ifdef WIRELESS_EXT
+	if (net->get_wireless_stats)
+		sysfs_remove_group(&class_dev->kobj, &wireless_group);
+#endif
+	class_device_del(class_dev);
+
+}
+
 /* Create sysfs entries for network device. */
 int netdev_register_sysfs(struct net_device *net)
 {

@@ -411,9 +426,15 @@ int netdev_register_sysfs(struct net_dev
 #ifdef WIRELESS_EXT
 	if (net->get_wireless_stats &&
 	    (ret = sysfs_create_group(&class_dev->kobj, &wireless_group)))
-		goto out_unreg; 
-#endif
+		goto out_cleanup; 
+
+	return 0;
+out_cleanup:
+	if (net->get_stats)
+		sysfs_remove_group(&class_dev->kobj, &netstat_group);
+#else
 	return 0;
+#endif
 
 out_unreg:
 	printk(KERN_WARNING "%s: sysfs attribute registration failed %d\n",

net/core/dev.c renamed from net/core/dev.c~sysfs-dentry-leak-fix +2 -1

diff -puN net/core/dev.c~sysfs-dentry-leak-fix net/core/dev.c
@@ -183,6 +183,7 @@ int netdev_fastroute_obstacles;
 
 extern int netdev_sysfs_init(void);
 extern int netdev_register_sysfs(struct net_device *);
+extern int netdev_unregister_sysfs(struct net_device *);
 #ifdef CONFIG_KGDB
 extern int kgdb_net_interrupt(struct sk_buff *skb);
 #endif

@@ -2834,7 +2835,7 @@ void netdev_run_todo(void)
 			break;
 
 		case NETREG_UNREGISTERING:
-			class_device_del(&dev->class_dev);
+			netdev_unregister_sysfs(dev);
 			dev->reg_state = NETREG_UNREGISTERED;
 
 			netdev_wait_allrefs(dev);

Maneesh Soni

author

2003-09-23 04:09 UTC #4 lore raw

Thanks Stephen for letting me know.

Maneesh

Rediffed for 2.6.0-test5.

 net/core/dev.c       |    3 ++-
 net/core/net-sysfs.c |   25 +++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 3 deletions(-)

net/core/net-sysfs.c renamed from net/core/net-sysfs.c~sysfs-dentry-leak-fix +23 -2

diff -puN net/core/net-sysfs.c~sysfs-dentry-leak-fix net/core/net-sysfs.c
@@ -383,6 +383,21 @@ static struct class net_class = {
 #endif
 };
 
+void netdev_unregister_sysfs(struct net_device * net)
+{
+	struct class_device * class_dev = &(net->class_dev);
+
+	if (net->get_stats)
+		sysfs_remove_group(&class_dev->kobj, &netstat_group);
+
+#ifdef WIRELESS_EXT
+	if (net->get_wireless_stats)
+		sysfs_remove_group(&class_dev->kobj, &wireless_group);
+#endif
+	class_device_del(class_dev);
+
+}
+
 /* Create sysfs entries for network device. */
 int netdev_register_sysfs(struct net_device *net)
 {

@@ -411,9 +426,15 @@ int netdev_register_sysfs(struct net_dev
 #ifdef WIRELESS_EXT
 	if (net->get_wireless_stats &&
 	    (ret = sysfs_create_group(&class_dev->kobj, &wireless_group)))
-		goto out_unreg; 
-#endif
+		goto out_cleanup; 
+
+	return 0;
+out_cleanup:
+	if (net->get_stats)
+		sysfs_remove_group(&class_dev->kobj, &netstat_group);
+#else
 	return 0;
+#endif
 
 out_unreg:
 	printk(KERN_WARNING "%s: sysfs attribute registration failed %d\n",

net/core/dev.c renamed from net/core/dev.c~sysfs-dentry-leak-fix +2 -1

diff -puN net/core/dev.c~sysfs-dentry-leak-fix net/core/dev.c
@@ -183,6 +183,7 @@ int netdev_fastroute_obstacles;
 
 extern int netdev_sysfs_init(void);
 extern int netdev_register_sysfs(struct net_device *);
+extern int netdev_unregister_sysfs(struct net_device *);
 
 
 /*******************************************************************************

@@ -2819,7 +2820,7 @@ void netdev_run_todo(void)
 			break;
 
 		case NETREG_UNREGISTERING:
-			class_device_del(&dev->class_dev);
+			netdev_unregister_sysfs(dev);
 			dev->reg_state = NETREG_UNREGISTERED;
 
 			netdev_wait_allrefs(dev);

Signature · 6 lines

Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: [email protected]
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696