[PATCH] tty_ioctl: locking for tty_wait_until_sent

[PATCH] tty_ioctl: locking for tty_wait_until_sent

[Alan Cox]

This function still depends on the big kernel lock in some cases. Push
locking into the function ready for removal of the BKL from ioctl call
paths.

Signed-off-by: Alan Cox <alan@redhat.com>

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c
--- linux.vanilla-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c	2008-03-10 12:57:53.000000000 +0000
+++ linux-2.6.25-rc3-mm1/drivers/char/tty_ioctl.c	2008-03-10 13:27:09.000000000 +0000
@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/bitops.h>
 #include <linux/mutex.h>
+#include <linux/smp_lock.h>
 
 #include <asm/io.h>
 #include <asm/uaccess.h>
@@ -61,11 +62,13 @@
 		return;
 	if (!timeout)
 		timeout = MAX_SCHEDULE_TIMEOUT;
+	lock_kernel();
 	if (wait_event_interruptible_timeout(tty->write_wait,
-			!tty->driver->chars_in_buffer(tty), timeout) < 0)
-		return;
-	if (tty->driver->wait_until_sent)
-		tty->driver->wait_until_sent(tty, timeout);
+			!tty->driver->chars_in_buffer(tty), timeout) >= 0) {
+		if (tty->driver->wait_until_sent)
+			tty->driver->wait_until_sent(tty, timeout);
+	}
+	unlock_kernel();
 }
 EXPORT_SYMBOL(tty_wait_until_sent);
 

Re: [PATCH] tty_ioctl: locking for tty_wait_until_sent

[Andi Kleen]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> This function still depends on the big kernel lock in some cases. Push
> locking into the function ready for removal of the BKL from ioctl call
> paths.

Didn't you forget the .ioctl -> .unlocked_ioctl change?

-Andi

Re: [PATCH] tty_ioctl: locking for tty_wait_until_sent

[Alan Cox]

On 10 Mar 2008 23:12:51 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> 
> > This function still depends on the big kernel lock in some cases. Push
> > locking into the function ready for removal of the BKL from ioctl call
> > paths.
> 
> Didn't you forget the .ioctl -> .unlocked_ioctl change?

We are not yet ready to unlock the device ioctl paths for tty. We still
explicitly take the BKL in the ioctl paths when calling the following
methods

driver:
	->wait_until_sent()
	->break_ctl()
	->tiocmget
	->tiocmset
	->ioctl
ldisc:
	->ioctl

As well as all the open/close/hangup/ldisc change logic

I'm pretty close to removing it from the modem , ioctl and break methods
and its working for me but needs a few drivers tweaking further.

Re: [PATCH] tty_ioctl: locking for tty_wait_until_sent

[Andi Kleen]

On Mon, Mar 10, 2008 at 10:06:43PM +0000, Alan Cox wrote:
> On 10 Mar 2008 23:12:51 +0100
> Andi Kleen <andi@firstfloor.org> wrote:
> 
> > Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> > 
> > > This function still depends on the big kernel lock in some cases. Push
> > > locking into the function ready for removal of the BKL from ioctl call
> > > paths.
> > 
> > Didn't you forget the .ioctl -> .unlocked_ioctl change?
> 
> We are not yet ready to unlock the device ioctl paths for tty. We still

Traditionally the usual is to first convert .ioctl to .unlocked_ioctl
and just slap lock_kernel around the whole ioctl handler and then 
later move it down step by step.

I didn't read the code completely but I assume tty_ioctl would be that
handler. I guess i was wrong?

-Andi

Re: [PATCH] tty_ioctl: locking for tty_wait_until_sent

[Jiri Slaby]

On 03/10/2008 11:28 PM, Andi Kleen wrote:
> On Mon, Mar 10, 2008 at 10:06:43PM +0000, Alan Cox wrote:
>> On 10 Mar 2008 23:12:51 +0100
>> Andi Kleen <andi@firstfloor.org> wrote:
>>
>>> Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
>>>
>>>> This function still depends on the big kernel lock in some cases. Push
>>>> locking into the function ready for removal of the BKL from ioctl call
>>>> paths.
>>> Didn't you forget the .ioctl -> .unlocked_ioctl change?
>> We are not yet ready to unlock the device ioctl paths for tty. We still
> 
> Traditionally the usual is to first convert .ioctl to .unlocked_ioctl
> and just slap lock_kernel around the whole ioctl handler and then 
> later move it down step by step.
> 
> I didn't read the code completely but I assume tty_ioctl would be that
> handler. I guess i was wrong?

tty_ioctl is already unlocked_ioctl (in -mm) ;). These patches are those next 
steps to propagate the bkl down.

regards,
--js