Patch: 2.6.7/fs/dnotify.c - make dn_lock a regular spinlock

PATCH Adam J. Richter 3 participants 4 messages Jun 17, 2004 VFS

Share & tools

Adam J. Richter

author

2004-06-17 08:42 UTC #0 lore raw

Hello Stephen,

In linux-2.6.7/fs/dnotify.c, the local varible dn_lock is
declared as rw_lock_t, but the lock is only taken exclusively. So, let's "document" this fact, save a few bytes and save a few cycles by changing it to spinlock_t.

I have tried running the dnotify user level program on
a multiprocessing kernel with this change, and it seems fine.

In the near future, I expect to try to eliminate dn_lock by
using parent_inode->i_sem instead, as the kmem_cache_t in dnotify.c does not need to be protected by a separate lock. However, such a change would require changes to the callers into dnotify, which currently make conflicting assumptions about whether they should be holding parent_inode->i_sem, child_inode->i_sem or neither when they call dnotify_parent or inode_dnotify, so that will require modifying many of the places that call into dnotify. So, I'd like to integrate this minor change first.

If this patch looks acceptable to you, could you please
tell the appropriate person to integrate it or advise me what to do if you want me to proceed some other way? I don't know if you submit your patches directly to Linus or through someone else, like Al Viro.

Signature · 3 lines

                    __     ______________ 
Adam J. Richter        \ /
[email protected]      | g g d r a s i l

fs/dnotify.c +7 -7

@@ -23,7 +23,7 @@
 
 int dir_notify_enable = 1;
 
-static rwlock_t dn_lock = RW_LOCK_UNLOCKED;
+static spinlock_t dn_lock = SPIN_LOCK_UNLOCKED;
 static kmem_cache_t *dn_cache;
 
 static void redo_inode_mask(struct inode *inode)

@@ -46,7 +46,7 @@
 	inode = filp->f_dentry->d_inode;
 	if (!S_ISDIR(inode->i_mode))
 		return;
-	write_lock(&dn_lock);
+	spin_lock(&dn_lock);
 	prev = &inode->i_dnotify;
 	while ((dn = *prev) != NULL) {
 		if ((dn->dn_owner == id) && (dn->dn_filp == filp)) {

@@ -57,7 +57,7 @@
 		}
 		prev = &dn->dn_next;
 	}
-	write_unlock(&dn_lock);
+	spin_unlock(&dn_lock);
 }
 
 int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)

@@ -81,7 +81,7 @@
 	dn = kmem_cache_alloc(dn_cache, SLAB_KERNEL);
 	if (dn == NULL)
 		return -ENOMEM;
-	write_lock(&dn_lock);
+	spin_lock(&dn_lock);
 	prev = &inode->i_dnotify;
 	while ((odn = *prev) != NULL) {
 		if ((odn->dn_owner == id) && (odn->dn_filp == filp)) {

@@ -105,7 +105,7 @@
 	dn->dn_next = inode->i_dnotify;
 	inode->i_dnotify = dn;
 out:
-	write_unlock(&dn_lock);
+	spin_unlock(&dn_lock);
 	return error;
 out_free:
 	kmem_cache_free(dn_cache, dn);

@@ -119,7 +119,7 @@
 	struct fown_struct *	fown;
 	int			changed = 0;
 
-	write_lock(&dn_lock);
+	spin_lock(&dn_lock);
 	prev = &inode->i_dnotify;
 	while ((dn = *prev) != NULL) {
 		if ((dn->dn_mask & event) == 0) {

@@ -138,7 +138,7 @@
 	}
 	if (changed)
 		redo_inode_mask(inode);
-	write_unlock(&dn_lock);
+	spin_unlock(&dn_lock);
 }
 
 EXPORT_SYMBOL(__inode_dir_notify);

Andrew Morton

2004-06-17 10:54 UTC #1 lore raw

Quoted 4 lines · Adam J. Richter

In the near future, I expect to try to eliminate dn_lock by
using parent_inode->i_sem instead, as the kmem_cache_t in dnotify.c does not need to be protected by a separate lock.

inode->i_lock would be better. Take care to keep it an "innermost" VFS lock though. Move kmem_cache_free() outside the lock altoghter.

Adam J. Richter

author

2004-06-17 14:55 UTC #2 lore raw

Quoted 8 lines · Andrew Morton

Quoted 4 lines · Adam J. Richter
In the near future, I expect to try to eliminate dn_lock by
using parent_inode->i_sem instead, as the kmem_cache_t in dnotify.c does not need to be protected by a separate lock.
inode->i_lock would be better. Take care to keep it an "innermost" VFS lock though.

	Thank you for the suggestion.  I was not aware of inode->i_lock.

	Looking at other users of inode->i_lock, I believe that using

inode->i_lock should not cause any conflict. The lock for inode->i_dnotify is only taken when someone calls the dnotify ioctl or if there actually is a match to some dnotify event, so the change in lock contention between a single dn_lock and inode->i_lock (which is obviously used elsewhere) should be minimal. The text + data of the .o file generate on x86 was actually 32 bytes smaller when I switched to inode->i_lock, and, of course, it made the source code 1 line shorter.

Quoted 1 line

Move kmem_cache_free() outside the lock altoghter.

Per your suggestion, I've done that in fcntl_dirnotify().
This wipes out the trivial space saving from switching to inode->i_lock, but it's probably more important to avoid holding inode->i_lock unnecessarily anyhow. My removal of one of the goto labels had no effect on object code size on my x86 configuration.

The other places that call kmem_cache_free() with the lock
held do so because they are iterating through inode->i_dnotify, and may have to call kmem_cache_free() repeatedly.

Here is an updated patch.

Signature · 3 lines

                    __     ______________
Adam J. Richter        \ /
[email protected]      | g g d r a s i l

fs/dnotify.c +10 ~~-10~~

@@ -23,7 +23,6 @@
 
 int dir_notify_enable = 1;
 
-static rwlock_t dn_lock = RW_LOCK_UNLOCKED;
 static kmem_cache_t *dn_cache;
 
 static void redo_inode_mask(struct inode *inode)

@@ -46,7 +45,7 @@
 	inode = filp->f_dentry->d_inode;
 	if (!S_ISDIR(inode->i_mode))
 		return;
-	write_lock(&dn_lock);
+	spin_lock(&inode->i_lock);
 	prev = &inode->i_dnotify;
 	while ((dn = *prev) != NULL) {
 		if ((dn->dn_owner == id) && (dn->dn_filp == filp)) {

@@ -57,7 +56,7 @@
 		}
 		prev = &dn->dn_next;
 	}
-	write_unlock(&dn_lock);
+	spin_unlock(&inode->i_lock);
 }
 
 int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)

@@ -81,7 +80,7 @@
 	dn = kmem_cache_alloc(dn_cache, SLAB_KERNEL);
 	if (dn == NULL)
 		return -ENOMEM;
-	write_lock(&dn_lock);
+	spin_lock(&inode->i_lock);
 	prev = &inode->i_dnotify;
 	while ((odn = *prev) != NULL) {
 		if ((odn->dn_owner == id) && (odn->dn_filp == filp)) {

@@ -104,12 +103,13 @@
 	inode->i_dnotify_mask |= arg & ~DN_MULTISHOT;
 	dn->dn_next = inode->i_dnotify;
 	inode->i_dnotify = dn;
-out:
-	write_unlock(&dn_lock);
-	return error;
+	spin_unlock(&inode->i_lock);
+	return 0;
+
 out_free:
+	spin_unlock(&inode->i_lock);
 	kmem_cache_free(dn_cache, dn);
-	goto out;
+	return error;
 }
 
 void __inode_dir_notify(struct inode *inode, unsigned long event)

@@ -119,7 +119,7 @@
 	struct fown_struct *	fown;
 	int			changed = 0;
 
-	write_lock(&dn_lock);
+	spin_lock(&inode->i_lock);
 	prev = &inode->i_dnotify;
 	while ((dn = *prev) != NULL) {
 		if ((dn->dn_mask & event) == 0) {

@@ -138,7 +138,7 @@
 	}
 	if (changed)
 		redo_inode_mask(inode);
-	write_unlock(&dn_lock);
+	spin_unlock(&inode->i_lock);
 }
 
 EXPORT_SYMBOL(__inode_dir_notify);

Stephen Rothwell

2004-06-17 15:27 UTC #3 lore raw

In reply to Adam J. Richter

Hi Adam,

Quoted 5 lines · On Thu, 17 Jun 2004 16:38:26 -0700 "Adam J. Richter" <[email protected]> wrote:

In linux-2.6.7/fs/dnotify.c, the local varible dn_lock is
declared as rw_lock_t, but the lock is only taken exclusively. So, let's "document" this fact, save a few bytes and save a few cycles by changing it to spinlock_t.

Fine by me, but other opinions are always welcome.

Quoted 5 lines

If this patch looks acceptable to you, could you please
tell the appropriate person to integrate it or advise me what to do if you want me to proceed some other way? I don't know if you submit your patches directly to Linus or through someone else, like Al Viro.

Well, Al was the last to modify the dnotify code, if memory serves. Getting his OK is always a good idea for this sort of thing. Otherwise, fine my me.

Signature · 3 lines

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/