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

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

[Andrew Morton]

"Adam J. Richter" <adam@yggdrasil.com> wrote:
>
> 	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.

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

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]

On Thu, Jun 17, 2004 at 03:53:13AM -0700, Andrew Morton wrote:
> "Adam J. Richter" <adam@yggdrasil.com> wrote:
> >
> > 	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.

>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.


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

[-- Attachment #2: dnotify.diff2 --]
[-- Type: text/plain, Size: 1808 bytes --]

--- linux-2.6.7/fs/dnotify.c	2004-06-15 22:19:09.000000000 -0700
+++ linux/fs/dnotify.c	2004-06-17 21:38:07.000000000 -0700
@@ -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);

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 910 bytes --]

Hi Adam,

On Thu, 17 Jun 2004 16:38:26 -0700 "Adam J. Richter" <adam@yggdrasil.com> 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.

> 	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.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

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.

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

[-- Attachment #2: dnotify.diff --]
[-- Type: text/plain, Size: 1626 bytes --]

--- linux-2.6.7/fs/dnotify.c	2004-06-15 22:19:09.000000000 -0700
+++ linux/fs/dnotify.c	2004-06-17 15:07:31.000000000 -0700
@@ -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);