use mutex instead of semaphore in tty_io.c

PATCH In review Matthias Kaehlcke 3 participants 9 messages Apr 25, 2007 – Jun 1, 2007 Char and misc drivers

lore

Share & tools

lore

Matthias Kaehlcke

author

2007-04-25 15:46 UTC #0 lore raw

PATCH 1/1 use mutex instead of semaphore in tty_io.c

drivers/char/tty_io.c uses a semaphore as mutex. use the mutex API instead of the (binary) semaphore

Signed-off-by Matthias Kaehlcke <[email protected]>

drivers/char/tty_io.c +10 ~~-10~~

index 7a32df5..4496fd2 100644
@@ -137,7 +137,7 @@ EXPORT_SYMBOL(tty_mutex);
 extern struct tty_driver *ptm_driver;	/* Unix98 pty masters; for /dev/ptmx */
 extern int pty_limit;		/* Config limit on Unix98 ptys */
 static DEFINE_IDR(allocated_ptys);
-static DECLARE_MUTEX(allocated_ptys_lock);
+static DEFINE_MUTEX(allocated_ptys_lock);
 static int ptmx_open(struct inode *, struct file *);
 #endif

@@ -2471,9 +2471,9 @@ static void release_dev(struct file * filp)
 #ifdef CONFIG_UNIX98_PTYS
 	/* Make this pty number available for reallocation */
 	if (devpts) {
-		down(&allocated_ptys_lock);
+		mutex_lock(&allocated_ptys_lock);
 		idr_remove(&allocated_ptys, idx);
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
 	}
 #endif

@@ -2639,24 +2639,24 @@ static int ptmx_open(struct inode * inode, struct file * filp)
 	nonseekable_open(inode, filp);
 
 	/* find a device that is not in use. */
-	down(&allocated_ptys_lock);
+	mutex_lock(&allocated_ptys_lock);
 	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL)) {
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
 		return -ENOMEM;
 	}
 	idr_ret = idr_get_new(&allocated_ptys, NULL, &index);
 	if (idr_ret < 0) {
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
 		if (idr_ret == -EAGAIN)
 			return -ENOMEM;
 		return -EIO;
 	}
 	if (index >= pty_limit) {
 		idr_remove(&allocated_ptys, index);
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
 		return -EIO;
 	}
-	up(&allocated_ptys_lock);
+	mutex_unlock(&allocated_ptys_lock);
 
 	mutex_lock(&tty_mutex);
 	retval = init_dev(ptm_driver, index, &tty);

@@ -2681,9 +2681,9 @@ out1:
 	release_dev(filp);
 	return retval;
 out:
-	down(&allocated_ptys_lock);
+	mutex_lock(&allocated_ptys_lock);
 	idr_remove(&allocated_ptys, index);
-	up(&allocated_ptys_lock);
+	mutex_unlock(&allocated_ptys_lock);
 	return retval;
 }
 #endif

Signature · 11 lines

Matthias Kaehlcke
Linux Application Developer
Barcelona

            You can chain me, you can torture me, you can even
          destroy this body, but you will never imprison my mind
                            (Mahatma Gandhi)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Christoph Hellwig

2007-04-25 19:14 UTC #1 lore raw

Quoted 2 lines · Matthias Kaehlcke

drivers/char/tty_io.c uses a semaphore as mutex. use the mutex API instead of the (binary) semaphore

This looks like it should be a spinlock:

drivers/char/tty_io.c static void release_dev(struct file * filp) · line 2476

Quoted 5 lines

drivers/char/tty_io.c +2 -2

-		down(&allocated_ptys_lock);
+		mutex_lock(&allocated_ptys_lock);
 		idr_remove(&allocated_ptys, idx);
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);

idr_remove is a quick operation that doesn't sleep.

drivers/char/tty_io.c static int ptmx_open(struct inode * inode, struct file * filp) · line 2644

Quoted 8 lines

drivers/char/tty_io.c +1 -2

@@ -2639,24 +2639,24 @@ static int ptmx_open(struct inode * inode, struct file * filp)
 	nonseekable_open(inode, filp);
 
 	/* find a device that is not in use. */
-	down(&allocated_ptys_lock);
+	mutex_lock(&allocated_ptys_lock);
 	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL)) {
-		up(&allocated_ptys_lock);

The idr_pre_get should be moved out of the lock, that's the whole point for it's existance..

Quoted 9 lines

drivers/char/tty_io.c +2 -1

+		mutex_unlock(&allocated_ptys_lock);
 		return -ENOMEM;
 	}
 	idr_ret = idr_get_new(&allocated_ptys, NULL, &index);
 	if (idr_ret < 0) {
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
 		if (idr_ret == -EAGAIN)
 			return -ENOMEM;

drivers/char/tty_io.c static int ptmx_open(struct inode * inode, struct file * filp) · line 2659

Quoted 10 lines

drivers/char/tty_io.c +2 -2

 		return -EIO;
 	}
 	if (index >= pty_limit) {
 		idr_remove(&allocated_ptys, index);
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
 		return -EIO;
 	}
-	up(&allocated_ptys_lock);
+	mutex_unlock(&allocated_ptys_lock);

And idr_get_new is another quick, non-blocking operation.

Matthias Kaehlcke

author

2007-04-25 19:43 UTC #2 lore raw

El Wed, Apr 25, 2007 at 08:13:59PM +0100 Christoph Hellwig ha dit:

Quoted 39 lines · Matthias Kaehlcke

Quoted 2 lines
drivers/char/tty_io.c uses a semaphore as mutex. use the mutex API instead of the (binary) semaphore
This looks like it should be a spinlock:
Review
Quoted 5 lines
(quoted diff) +2 -2
-		down(&allocated_ptys_lock);
+		mutex_lock(&allocated_ptys_lock);
 		idr_remove(&allocated_ptys, idx);
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
idr_remove is a quick operation that doesn't sleep.
Review static int ptmx_open(struct inode * inode, struct file * filp) · line 2644
Quoted 8 lines
(quoted diff) +1 -2
@@ -2639,24 +2639,24 @@ static int ptmx_open(struct inode * inode, struct file * filp)
 	nonseekable_open(inode, filp);
 
 	/* find a device that is not in use. */
-	down(&allocated_ptys_lock);
+	mutex_lock(&allocated_ptys_lock);
 	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL)) {
-		up(&allocated_ptys_lock);
The idr_pre_get should be moved out of the lock, that's the whole point for it's existance..
Quoted 13 lines
(quoted diff) +2 -1
+		mutex_unlock(&allocated_ptys_lock);
 		return -ENOMEM;
 	}
 	idr_ret = idr_get_new(&allocated_ptys, NULL, &index);
 	if (idr_ret < 0) {
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
 		if (idr_ret == -EAGAIN)
 			return -ENOMEM;
 		return -EIO;
 	}
 	if (index >= pty_limit) {
 		idr_remove(&allocated_ptys, index);

Quoted 8 lines

Review static int ptmx_open(struct inode * inode, struct file * filp) · line 2659
Quoted 6 lines
(quoted diff) +2 -2
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
 		return -EIO;
 	}
-	up(&allocated_ptys_lock);
+	mutex_unlock(&allocated_ptys_lock);
And idr_get_new is another quick, non-blocking operation.

thanks for your remarks. is the following patch what you are proposing?

drivers/char/tty_io.c +12 ~~-12~~

index 7a32df5..ff27587 100644
@@ -137,7 +137,7 @@ EXPORT_SYMBOL(tty_mutex);
 extern struct tty_driver *ptm_driver;	/* Unix98 pty masters; for /dev/ptmx */
 extern int pty_limit;		/* Config limit on Unix98 ptys */
 static DEFINE_IDR(allocated_ptys);
-static DECLARE_MUTEX(allocated_ptys_lock);
+static DEFINE_SPINLOCK(allocated_ptys_lock);
 static int ptmx_open(struct inode *, struct file *);
 #endif

@@ -2471,9 +2471,9 @@ static void release_dev(struct file * filp)
 #ifdef CONFIG_UNIX98_PTYS
 	/* Make this pty number available for reallocation */
 	if (devpts) {
-		down(&allocated_ptys_lock);
+		spin_lock(&allocated_ptys_lock);
 		idr_remove(&allocated_ptys, idx);
-		up(&allocated_ptys_lock);
+		spin_unlock(&allocated_ptys_lock);
 	}
 #endif

@@ -2639,24 +2639,24 @@ static int ptmx_open(struct inode * inode, struct file * filp)
 	nonseekable_open(inode, filp);
 
 	/* find a device that is not in use. */
-	down(&allocated_ptys_lock);
-	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL)) {
-		up(&allocated_ptys_lock);
+	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL))
 		return -ENOMEM;
-	}
+
+	spin_lock(&allocated_ptys_lock);
+
 	idr_ret = idr_get_new(&allocated_ptys, NULL, &index);
 	if (idr_ret < 0) {
-		up(&allocated_ptys_lock);
+		spin_unlock(&allocated_ptys_lock);
 		if (idr_ret == -EAGAIN)
 			return -ENOMEM;
 		return -EIO;
 	}
 	if (index >= pty_limit) {
 		idr_remove(&allocated_ptys, index);
-		up(&allocated_ptys_lock);
+		spin_unlock(&allocated_ptys_lock);
 		return -EIO;
 	}
-	up(&allocated_ptys_lock);
+	spin_unlock(&allocated_ptys_lock);
 
 	mutex_lock(&tty_mutex);
 	retval = init_dev(ptm_driver, index, &tty);

@@ -2681,9 +2681,9 @@ out1:
 	release_dev(filp);
 	return retval;
 out:
-	down(&allocated_ptys_lock);
+	spin_lock(&allocated_ptys_lock);
 	idr_remove(&allocated_ptys, index);
-	up(&allocated_ptys_lock);
+	spin_unlock(&allocated_ptys_lock);
 	return retval;
 }
 #endif

Signature · 10 lines

Matthias Kaehlcke
Linux Application Developer
Barcelona

             You must have a plan. If you don't have a plan,
               you'll become part of somebody else's plan
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Christoph Hellwig

2007-04-25 19:45 UTC #3 lore raw

Quoted 2 lines · Matthias Kaehlcke

thanks for your remarks. is the following patch what you are proposing?

Yes, exactly. Looks good to me this way.:w

Matthias Kaehlcke

author

2007-05-31 13:40 UTC #4 lore raw

drivers/char/tty_io.c: Use spinlock instead of a (binary) semaphore

Signed-off-by Matthias Kaehlcke <[email protected]>

drivers/char/tty_io.c +12 ~~-12~~

index 7a32df5..ff27587 100644
@@ -137,7 +137,7 @@ EXPORT_SYMBOL(tty_mutex);
 extern struct tty_driver *ptm_driver;	/* Unix98 pty masters; for /dev/ptmx */
 extern int pty_limit;		/* Config limit on Unix98 ptys */
 static DEFINE_IDR(allocated_ptys);
-static DECLARE_MUTEX(allocated_ptys_lock);
+static DEFINE_SPINLOCK(allocated_ptys_lock);
 static int ptmx_open(struct inode *, struct file *);
 #endif

@@ -2471,9 +2471,9 @@ static void release_dev(struct file * filp)
 #ifdef CONFIG_UNIX98_PTYS
 	/* Make this pty number available for reallocation */
 	if (devpts) {
-		down(&allocated_ptys_lock);
+		spin_lock(&allocated_ptys_lock);
 		idr_remove(&allocated_ptys, idx);
-		up(&allocated_ptys_lock);
+		spin_unlock(&allocated_ptys_lock);
 	}
 #endif

@@ -2639,24 +2639,24 @@ static int ptmx_open(struct inode * inode, struct file * filp)
 	nonseekable_open(inode, filp);
 
 	/* find a device that is not in use. */
-	down(&allocated_ptys_lock);
-	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL)) {
-		up(&allocated_ptys_lock);
+	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL))
 		return -ENOMEM;
-	}
+
+	spin_lock(&allocated_ptys_lock);
+
 	idr_ret = idr_get_new(&allocated_ptys, NULL, &index);
 	if (idr_ret < 0) {
-		up(&allocated_ptys_lock);
+		spin_unlock(&allocated_ptys_lock);
 		if (idr_ret == -EAGAIN)
 			return -ENOMEM;
 		return -EIO;
 	}
 	if (index >= pty_limit) {
 		idr_remove(&allocated_ptys, index);
-		up(&allocated_ptys_lock);
+		spin_unlock(&allocated_ptys_lock);
 		return -EIO;
 	}
-	up(&allocated_ptys_lock);
+	spin_unlock(&allocated_ptys_lock);
 
 	mutex_lock(&tty_mutex);
 	retval = init_dev(ptm_driver, index, &tty);

@@ -2681,9 +2681,9 @@ out1:
 	release_dev(filp);
 	return retval;
 out:
-	down(&allocated_ptys_lock);
+	spin_lock(&allocated_ptys_lock);
 	idr_remove(&allocated_ptys, index);
-	up(&allocated_ptys_lock);
+	spin_unlock(&allocated_ptys_lock);
 	return retval;
 }
 #endif

Signature · 10 lines

Matthias Kaehlcke
Linux Application Developer
Barcelona

     The assumption that what currently exists must necessarily
      exist is the acid that corrodes all visionary thinking
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Christoph Hellwig

2007-05-31 13:59 UTC #5 lore raw

Quoted 1 line · Matthias Kaehlcke

drivers/char/tty_io.c: Use spinlock instead of a (binary) semaphore

Looks good.

Andrew Morton

2007-05-31 22:37 UTC #6 lore raw

In reply to Matthias Kaehlcke

Quoted 2 lines · On Thu, 31 May 2007 15:42:26 +0200 Matthias Kaehlcke…

drivers/char/tty_io.c: Use spinlock instead of a (binary) semaphore

hm.

drivers/char/tty_io.c

Quoted 7 lines

--
drivers/char/tty_io.c +0 -0
index 7a32df5..ff27587 100644

We end up with this:

	/* find a device that is not in use. */
	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL))
		return -ENOMEM;

	spin_lock(&allocated_ptys_lock);

	idr_ret = idr_get_new(&allocated_ptys, NULL, &index);
	if (idr_ret < 0) {
		spin_unlock(&allocated_ptys_lock);
		if (idr_ret == -EAGAIN)
			return -ENOMEM;
		return -EIO;
	}
	if (index >= pty_limit) {
		idr_remove(&allocated_ptys, index);
		spin_unlock(&allocated_ptys_lock);
		return -EIO;
	}
	spin_unlock(&allocated_ptys_lock);

this leaves a small window in which another thread can come in and steal away the idr tree's reserves, causing the idr_get_new() to fail. It's highly improbable, but it's real.

Hence I think a straight semaphore->mutex conversion would be better.

The IDR API absolutely blows chunks: it should require caller-provided locking, like radix-tree. But then it'd need gunk like radix_tree_preload to be reliable. Fact is, storage librares which need to allocate memory at insert-time are always going to be problematic in-kernel.

Matthias Kaehlcke

author

2007-06-01 07:46 UTC #7 lore raw

El Thu, May 31, 2007 at 03:37:12PM -0700 Andrew Morton ha dit:

Quoted 32 lines

Quoted 2 lines · On Thu, 31 May 2007 15:42:26 +0200 Matthias Kaehlcke…

drivers/char/tty_io.c: Use spinlock instead of a (binary) semaphore

hm.

Quoted 1 line

We end up with this:

	/* find a device that is not in use. */
	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL))
		return -ENOMEM;

	spin_lock(&allocated_ptys_lock);

	idr_ret = idr_get_new(&allocated_ptys, NULL, &index);
	if (idr_ret < 0) {
		spin_unlock(&allocated_ptys_lock);
		if (idr_ret == -EAGAIN)
			return -ENOMEM;
		return -EIO;
	}
	if (index >= pty_limit) {
		idr_remove(&allocated_ptys, index);
		spin_unlock(&allocated_ptys_lock);
		return -EIO;
	}
	spin_unlock(&allocated_ptys_lock);

Quoted 3 lines

this leaves a small window in which another thread can come in and steal away the idr tree's reserves, causing the idr_get_new() to fail. It's highly improbable, but it's real.

i agree, thanks for pointing it out

Quoted 1 line

Hence I think a straight semaphore->mutex conversion would be better.

that leads us back to the initial patch. christoph: is that ok for you or do you have another proposal?

Signature · 10 lines

Matthias Kaehlcke
Linux Application Developer
Barcelona

     The assumption that what currently exists must necessarily
      exist is the acid that corrodes all visionary thinking
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Andrew Morton

2007-04-26 02:20 UTC #8 lore raw

In reply to Christoph Hellwig

Quoted 17 lines · On Wed, 25 Apr 2007 20:13:59 +0100 Christoph Hellwig <[email protected]> wrote:

Quoted 2 lines · Matthias Kaehlcke
drivers/char/tty_io.c uses a semaphore as mutex. use the mutex API instead of the (binary) semaphore
This looks like it should be a spinlock:
drivers/char/tty_io.c static void release_dev(struct file * filp) · line 2476
Quoted 5 lines
drivers/char/tty_io.c +2 -2
-		down(&allocated_ptys_lock);
+		mutex_lock(&allocated_ptys_lock);
 		idr_remove(&allocated_ptys, idx);
-		up(&allocated_ptys_lock);
+		mutex_unlock(&allocated_ptys_lock);
idr_remove is a quick operation that doesn't sleep.
Quoted 3 lines
drivers/char/tty_io.c +0 -0
@@ -2639,24 +2639,24 @@ static int ptmx_open(struct inode * inode, struct file * filp)
 	nonseekable_open(inode, filp);
 

Quoted 9 lines

drivers/char/tty_io.c static int ptmx_open(struct inode * inode, struct file * filp) · line 2644
Quoted 5 lines
drivers/char/tty_io.c +1 -2
 	/* find a device that is not in use. */
-	down(&allocated_ptys_lock);
+	mutex_lock(&allocated_ptys_lock);
 	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL)) {
-		up(&allocated_ptys_lock);
The idr_pre_get should be moved out of the lock, that's the whole point for it's existance..

I think having it inside the lock makes sense:

	mutex_lock()
	idr_pre_get()
	idr_get_new()
	mutex_unlock()

here, if idr_pre_get() succeeded, we know that idr_get_new() will succeed.

otoh:

try_again:

	idr_pre_get()
	mutex_lock()
	if (idr_get_new() == failed) {
		mutex_unlock()
		goto try_again;
	}
	mutex_unlock()

is not nice.

the IDR api is awful. A little project is to rip out all its internal locking and to implement caller-provided locking.

Unfortunately the fact that the library allocates memory means that we might need to do awkward things like radix_tree_preload() to make it reliable for callers who use spinlocking.