Re: patch "TTY: remove tty_locked" added to tty tree

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

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

On 08/23/2011 07:36 PM, gregkh@suse.de wrote:
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 44bc0c5..6d5eceb 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -600,8 +600,6 @@ extern long vt_compat_ioctl(struct tty_struct *tty,
>  /* functions for preparation of BKL removal */
>  extern void __lockfunc tty_lock(void) __acquires(tty_lock);
>  extern void __lockfunc tty_unlock(void) __releases(tty_lock);
> -extern struct task_struct *__big_tty_mutex_owner;
> -#define tty_locked()		(current == __big_tty_mutex_owner)

It looks like I need tty_locked to fix system stalls in
tty_wait_until_sent in the end. Like with the patch attached. But if
someone finds another way to fix this in short-term, it would be great.

thanks,
-- 
js
suse labs

[-- Attachment #2: 0001-TTY-fix-stalls-on-BTM-when-waiting-until-sent.patch --]
[-- Type: text/x-patch, Size: 1375 bytes --]

>From a7c35daffe26129072aceb5d285924017820b56d Mon Sep 17 00:00:00 2001
From: Jiri Slaby <jslaby@suse.cz>
Date: Fri, 19 Aug 2011 22:16:51 +0200
Subject: [PATCH] TTY: fix stalls on BTM when waiting until sent

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Andreas Bombe <aeb@debian.org>
---
 drivers/tty/tty_ioctl.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 53f2442..3837175 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -146,6 +146,7 @@ EXPORT_SYMBOL(tty_unthrottle);
 
 void tty_wait_until_sent(struct tty_struct *tty, long timeout)
 {
+	int retval;
 #ifdef TTY_DEBUG_WAIT_UNTIL_SENT
 	char buf[64];
 
@@ -153,8 +154,14 @@ void tty_wait_until_sent(struct tty_struct *tty, long timeout)
 #endif
 	if (!timeout)
 		timeout = MAX_SCHEDULE_TIMEOUT;
-	if (wait_event_interruptible_timeout(tty->write_wait,
-			!tty_chars_in_buffer(tty), timeout) >= 0) {
+
+	if (tty_locked()) /* e.g. uart (holds) vs. tty_ioctl (does not) */
+		retval = wait_event_interruptible_timeout_tty(tty->write_wait,
+			!tty_chars_in_buffer(tty), timeout);
+	else
+		retval = wait_event_interruptible_timeout(tty->write_wait,
+			!tty_chars_in_buffer(tty), timeout);
+	if (retval >= 0) {
 		if (tty->ops->wait_until_sent)
 			tty->ops->wait_until_sent(tty, timeout);
 	}
-- 
1.7.6

Re: patch "TTY: remove tty_locked" added to tty tree

[Arnd Bergmann]

On Tuesday 23 August 2011 20:33:38 Jiri Slaby wrote:

> It looks like I need tty_locked to fix system stalls in
> tty_wait_until_sent in the end. Like with the patch attached. But if
> someone finds another way to fix this in short-term, it would be great.
>
> 
> @@ -153,8 +154,14 @@ void tty_wait_until_sent(struct tty_struct *tty, long timeout)
>  #endif
>  	if (!timeout)
>  		timeout = MAX_SCHEDULE_TIMEOUT;
> -	if (wait_event_interruptible_timeout(tty->write_wait,
> -			!tty_chars_in_buffer(tty), timeout) >= 0) {
> +
> +	if (tty_locked()) /* e.g. uart (holds) vs. tty_ioctl (does not) */
> +		retval = wait_event_interruptible_timeout_tty(tty->write_wait,
> +			!tty_chars_in_buffer(tty), timeout);
> +	else
> +		retval = wait_event_interruptible_timeout(tty->write_wait,
> +			!tty_chars_in_buffer(tty), timeout);
> 
> 

According to http://kernelnewbies.org/BigKernelLock, I concluded back then
that tty_wait_until_sent would always be called without BTM held. Has that
changed recently, or did I miss a caller that holds the BTM?

The only caller that used to call it with BKL was tty_set_ldisc, and that
got changed to release the BTM before calling it. If there is another
caller, can it do the same?

	Arnd

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

On 08/23/2011 08:46 PM, Arnd Bergmann wrote:
> According to http://kernelnewbies.org/BigKernelLock, I concluded back then
> that tty_wait_until_sent would always be called without BTM held. Has that
> changed recently, or did I miss a caller that holds the BTM?

Every tty_operations->close and ->hangup :).

-- 
js
suse labs

Re: patch "TTY: remove tty_locked" added to tty tree

[Arnd Bergmann]

On Tuesday 23 August 2011 20:54:08 Jiri Slaby wrote:
> On 08/23/2011 08:46 PM, Arnd Bergmann wrote:
> > According to http://kernelnewbies.org/BigKernelLock, I concluded back then
> > that tty_wait_until_sent would always be called without BTM held. Has that
> > changed recently, or did I miss a caller that holds the BTM?
> 
> Every tty_operations->close and ->hangup :).

Ah, right, I remember. The chart I did was only to prove that locking was
consistent (i.e. no deadlocks), it ignored that the function needs to be
called without BTM because I had incorrectly convinced myself that the
wait_event_interruptible_timeout() didn't need to release it.

I think I just saw another problem: uart_close takes port->mutex while
holding the BTM, then calls tty_wait_until_sent(). If this releases
and reaquires the BTM, you get an AB-BA deadlock with port->mutex.

	Arnd

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

On 08/24/2011 10:46 AM, Arnd Bergmann wrote:
> On Tuesday 23 August 2011 20:54:08 Jiri Slaby wrote:
>> On 08/23/2011 08:46 PM, Arnd Bergmann wrote:
>>> According to http://kernelnewbies.org/BigKernelLock, I concluded back then
>>> that tty_wait_until_sent would always be called without BTM held. Has that
>>> changed recently, or did I miss a caller that holds the BTM?
>>
>> Every tty_operations->close and ->hangup :).
> 
> Ah, right, I remember. The chart I did was only to prove that locking was
> consistent (i.e. no deadlocks), it ignored that the function needs to be
> called without BTM because I had incorrectly convinced myself that the
> wait_event_interruptible_timeout() didn't need to release it.
> 
> I think I just saw another problem: uart_close takes port->mutex while
> holding the BTM, then calls tty_wait_until_sent(). If this releases
> and reaquires the BTM, you get an AB-BA deadlock with port->mutex.

Aargh, right. The question is why uart_close takes port->mutex there? It
may take it even right before uart_shutdown. As tty_wait_until_sent (or
uart_wait_until_sent) may be called e.g. from set_termios without that
lock anyway. There are ->tx_empty and ->stop_rx that may need some
protection. But those are register accessors, so they should be
protected by some spinlock to not race with interrupts. Actually stop_rx
is. And empty_rx is only in 8250.

And I don't see anything else there which would need be protected by the
lock. Do you?

thanks,
-- 
js
suse labs

Re: patch "TTY: remove tty_locked" added to tty tree

[Arnd Bergmann]

On Wednesday 24 August 2011, Jiri Slaby wrote:
> On 08/24/2011 10:46 AM, Arnd Bergmann wrote:
> > On Tuesday 23 August 2011 20:54:08 Jiri Slaby wrote:
> >> On 08/23/2011 08:46 PM, Arnd Bergmann wrote:

> > I think I just saw another problem: uart_close takes port->mutex while
> > holding the BTM, then calls tty_wait_until_sent(). If this releases
> > and reaquires the BTM, you get an AB-BA deadlock with port->mutex.
> 
> Aargh, right. The question is why uart_close takes port->mutex there? It
> may take it even right before uart_shutdown. As tty_wait_until_sent (or
> uart_wait_until_sent) may be called e.g. from set_termios without that
> lock anyway. There are ->tx_empty and ->stop_rx that may need some
> protection. But those are register accessors, so they should be
> protected by some spinlock to not race with interrupts. Actually stop_rx
> is. And empty_rx is only in 8250.
> 
> And I don't see anything else there which would need be protected by the
> lock. Do you?

I have not looked at correctness of port->lock before, I just tried to
make sure that BTM correctly nests around it when I removed the BKL.

It's not clear to me what state->mutex protects in the serial_core, but
it has been around forever (used to be called state->sem) and is held in
all uart functions, which is at least consistent. IIRC what Alan's plan
for this was, uart_close should eventually get changed to use
tty_port_close_start or even tty_port_close. Maybe the time for that has
come now, lacking better alternatives?

A lot of other drivers call tty_port_close_start() before taking port->mutex.

	Arnd

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
> It's not clear to me what state->mutex protects in the serial_core, but
> it has been around forever (used to be called state->sem)

It was actually moved in uart_close back in 2003. Formerly (when there
was only a coarse grained port_sem) it was right before uart_shutdown.
But there were some flags to handle some races. I'm not sure whether the
flags protected any race here though.

> and is held in
> all uart functions, which is at least consistent. IIRC what Alan's plan
> for this was, uart_close should eventually get changed to use
> tty_port_close_start or even tty_port_close. Maybe the time for that has
> come now, lacking better alternatives?

Yes, I have such a patch in my queue. But it's not easy to get there.
You may take a look at:
http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel

But it's still far from ready. And yet, in the queue, I still have
port->mutex locked before tty_port_close_start like it is now.

> A lot of other drivers call tty_port_close_start() before taking port->mutex.

Yes, that's true. That's why I wrote that before. But most of the
drivers doesn't have so complex locking dependencies like uart.

thanks,
-- 
js
suse labs

Re: patch "TTY: remove tty_locked" added to tty tree

[Arnd Bergmann]

On Wednesday 24 August 2011, Jiri Slaby wrote:
> On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
> > It's not clear to me what state->mutex protects in the serial_core, but
> > it has been around forever (used to be called state->sem)
> 
> It was actually moved in uart_close back in 2003. Formerly (when there
> was only a coarse grained port_sem) it was right before uart_shutdown.
> But there were some flags to handle some races. I'm not sure whether the
> flags protected any race here though.

ok

> > and is held in
> > all uart functions, which is at least consistent. IIRC what Alan's plan
> > for this was, uart_close should eventually get changed to use
> > tty_port_close_start or even tty_port_close. Maybe the time for that has
> > come now, lacking better alternatives?
> 
> Yes, I have such a patch in my queue. But it's not easy to get there.
> You may take a look at:
> http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel
> 
> But it's still far from ready. And yet, in the queue, I still have
> port->mutex locked before tty_port_close_start like it is now.

Ah, right. I still don't see why the port->mutex is or is not needed there,
and I think that's the main issue.

By comparison, getting *_wait_until_sent to be called without BTM seems
easy -- we know that all callers from ->close() hold it, while the ones
from ->ioctl() don't. We could have a helper like

void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout)
{
	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
	tty_wait_until_sent(tty, timeout);
	tty_lock();
}

to deal with that, if only we can sort the lock ordering with port->mutex.

BTW, I saw that the three m68k serial port drivers (amiserial, 68328, 68360)
all call *_wait_until_sent with interrupts disabled, which is even more
broken.

	Arnd

Re: patch "TTY: remove tty_locked" added to tty tree

[Arnd Bergmann]

On Wednesday 24 August 2011, Jiri Slaby wrote:
> On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
> > It's not clear to me what state->mutex protects in the serial_core, but
> > it has been around forever (used to be called state->sem)
> 
> It was actually moved in uart_close back in 2003. Formerly (when there
> was only a coarse grained port_sem) it was right before uart_shutdown.
> But there were some flags to handle some races. I'm not sure whether the
> flags protected any race here though.

ok

> > and is held in
> > all uart functions, which is at least consistent. IIRC what Alan's plan
> > for this was, uart_close should eventually get changed to use
> > tty_port_close_start or even tty_port_close. Maybe the time for that has
> > come now, lacking better alternatives?
> 
> Yes, I have such a patch in my queue. But it's not easy to get there.
> You may take a look at:
> http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel
> 
> But it's still far from ready. And yet, in the queue, I still have
> port->mutex locked before tty_port_close_start like it is now.

Ah, right. I still don't see why the port->mutex is or is not needed there,
and I think that's the main issue.

By comparison, getting *_wait_until_sent to be called without BTM seems
easy -- we know that all callers from ->close() hold it, while the ones
from ->ioctl() don't. We could have a helper like

void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout)
{
	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
	tty_wait_until_sent(tty, timeout);
	tty_lock();
}

to deal with that, if only we can sort the lock ordering with port->mutex.

BTW, I saw that the three m68k serial port drivers (amiserial, 68328, 68360)
all call *_wait_until_sent with interrupts disabled, which is even more
broken.

	Arnd

Re: patch "TTY: remove tty_locked" added to tty tree

[Alan Cox]

> It's not clear to me what state->mutex protects in the serial_core,
> but it has been around forever (used to be called state->sem) and is
> held in all uart functions, which is at least consistent. IIRC what
> Alan's plan for this was, uart_close should eventually get changed to
> use tty_port_close_start or even tty_port_close. Maybe the time for
> that has come now, lacking better alternatives?

That was certainly the plan.

> A lot of other drivers call tty_port_close_start() before taking
> port->mutex.

You don't need the port->mutex for tty_port_close_start() if I got it
right, the reason for taking it across the whole of tty_port_close is
so we would end up with a simple API which could be explained as

"open, close and hangup manage locking internally, you will never get
 the events overlapping in any callback. Initialize and shutdown
 callbacks will only be called once with each initialize of the first
 open matching a shutdown of the final close"

It has no meaningful performance impact so it seemed silly not to
provide a somewhat more accident-proof API.

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

On 08/24/2011 04:35 PM, Arnd Bergmann wrote:
> On Wednesday 24 August 2011, Jiri Slaby wrote:
>> On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
>>> It's not clear to me what state->mutex protects in the serial_core, but
>>> it has been around forever (used to be called state->sem)
>>
>> It was actually moved in uart_close back in 2003. Formerly (when there
>> was only a coarse grained port_sem) it was right before uart_shutdown.
>> But there were some flags to handle some races. I'm not sure whether the
>> flags protected any race here though.
> 
> ok
> 
>>> and is held in
>>> all uart functions, which is at least consistent. IIRC what Alan's plan
>>> for this was, uart_close should eventually get changed to use
>>> tty_port_close_start or even tty_port_close. Maybe the time for that has
>>> come now, lacking better alternatives?
>>
>> Yes, I have such a patch in my queue. But it's not easy to get there.
>> You may take a look at:
>> http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel
>>
>> But it's still far from ready. And yet, in the queue, I still have
>> port->mutex locked before tty_port_close_start like it is now.
> 
> Ah, right. I still don't see why the port->mutex is or is not needed there,
> and I think that's the main issue.
> 
> By comparison, getting *_wait_until_sent to be called without BTM seems
> easy -- we know that all callers from ->close() hold it, while the ones
> from ->ioctl() don't. We could have a helper like
> 
> void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout)
> {
> 	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
> 	tty_wait_until_sent(tty, timeout);
> 	tty_lock();
> }
> 
> to deal with that, if only we can sort the lock ordering with .

Ah, it looks like I just got the reason why port->mutex is locked in the
top of uart_close. In uart, TTY_CLOSING flag is not used. So there is
nothing to protect against races between ->close (the code between the
two spinlock critical sections corresponding to port_close_start and
_end) and ->open (block_til_ready).

Other than that I see no point for the lock to be in the beginning. So
if we introduce CLOSING flag (I do that in my patches implicitly),
everything should be OK:
* port->count etc is and always was protected by the spinlock,
* ->stop_rx stands as I wrote earlier.
* uart_wait_until_sent -- that one is already called without port->mutex
from set_termios and tty_set_ldisc.

So it looks like we should:
- introduce CLOSING flag
- move the lock below, before shutdown
- introduce your magic _from_close helper
- use it

Doing this after we have all the helpers in place would be easier. There
would be no need to play with CLOSING bit. But there will be no option
to backport this to stable trees then. And I know I will have to do that
at least for 3.0.

Note that we may use the _from_close helper from tty_port_close_start
almost instantly. All users should not hold port->mutex over
tty_port_close_start. But I need to check. Tomorrow.

In the meantime, comments welcome.

> BTW, I saw that the three m68k serial port drivers (amiserial, 68328, 68360)
> all call *_wait_until_sent with interrupts disabled, which is even more
> broken.

Blah, yes.

-- 
js
suse labs

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

On 08/24/2011 04:35 PM, Arnd Bergmann wrote:
> On Wednesday 24 August 2011, Jiri Slaby wrote:
>> On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
>>> It's not clear to me what state->mutex protects in the serial_core, but
>>> it has been around forever (used to be called state->sem)
>>
>> It was actually moved in uart_close back in 2003. Formerly (when there
>> was only a coarse grained port_sem) it was right before uart_shutdown.
>> But there were some flags to handle some races. I'm not sure whether the
>> flags protected any race here though.
> 
> ok
> 
>>> and is held in
>>> all uart functions, which is at least consistent. IIRC what Alan's plan
>>> for this was, uart_close should eventually get changed to use
>>> tty_port_close_start or even tty_port_close. Maybe the time for that has
>>> come now, lacking better alternatives?
>>
>> Yes, I have such a patch in my queue. But it's not easy to get there.
>> You may take a look at:
>> http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel
>>
>> But it's still far from ready. And yet, in the queue, I still have
>> port->mutex locked before tty_port_close_start like it is now.
> 
> Ah, right. I still don't see why the port->mutex is or is not needed there,
> and I think that's the main issue.
> 
> By comparison, getting *_wait_until_sent to be called without BTM seems
> easy -- we know that all callers from ->close() hold it, while the ones
> from ->ioctl() don't. We could have a helper like
> 
> void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout)
> {
> 	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
> 	tty_wait_until_sent(tty, timeout);
> 	tty_lock();
> }
> 
> to deal with that, if only we can sort the lock ordering with .

Ah, it looks like I just got the reason why port->mutex is locked in the
top of uart_close. In uart, TTY_CLOSING flag is not used. So there is
nothing to protect against races between ->close (the code between the
two spinlock critical sections corresponding to port_close_start and
_end) and ->open (block_til_ready).

Other than that I see no point for the lock to be in the beginning. So
if we introduce CLOSING flag (I do that in my patches implicitly),
everything should be OK:
* port->count etc is and always was protected by the spinlock,
* ->stop_rx stands as I wrote earlier.
* uart_wait_until_sent -- that one is already called without port->mutex
from set_termios and tty_set_ldisc.

So it looks like we should:
- introduce CLOSING flag
- move the lock below, before shutdown
- introduce your magic _from_close helper
- use it

Doing this after we have all the helpers in place would be easier. There
would be no need to play with CLOSING bit. But there will be no option
to backport this to stable trees then. And I know I will have to do that
at least for 3.0.

Note that we may use the _from_close helper from tty_port_close_start
almost instantly. All users should not hold port->mutex over
tty_port_close_start. But I need to check. Tomorrow.

In the meantime, comments welcome.

> BTW, I saw that the three m68k serial port drivers (amiserial, 68328, 68360)
> all call *_wait_until_sent with interrupts disabled, which is even more
> broken.

Blah, yes.

-- 
js
suse labs

Re: patch "TTY: remove tty_locked" added to tty tree

[Greg KH]

On Wed, Aug 24, 2011 at 11:27:19PM +0200, Jiri Slaby wrote:
> On 08/24/2011 04:35 PM, Arnd Bergmann wrote:
> > On Wednesday 24 August 2011, Jiri Slaby wrote:
> >> On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
> >>> It's not clear to me what state->mutex protects in the serial_core, but
> >>> it has been around forever (used to be called state->sem)
> >>
> >> It was actually moved in uart_close back in 2003. Formerly (when there
> >> was only a coarse grained port_sem) it was right before uart_shutdown.
> >> But there were some flags to handle some races. I'm not sure whether the
> >> flags protected any race here though.
> > 
> > ok
> > 
> >>> and is held in
> >>> all uart functions, which is at least consistent. IIRC what Alan's plan
> >>> for this was, uart_close should eventually get changed to use
> >>> tty_port_close_start or even tty_port_close. Maybe the time for that has
> >>> come now, lacking better alternatives?
> >>
> >> Yes, I have such a patch in my queue. But it's not easy to get there.
> >> You may take a look at:
> >> http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel
> >>
> >> But it's still far from ready. And yet, in the queue, I still have
> >> port->mutex locked before tty_port_close_start like it is now.
> > 
> > Ah, right. I still don't see why the port->mutex is or is not needed there,
> > and I think that's the main issue.
> > 
> > By comparison, getting *_wait_until_sent to be called without BTM seems
> > easy -- we know that all callers from ->close() hold it, while the ones
> > from ->ioctl() don't. We could have a helper like
> > 
> > void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout)
> > {
> > 	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
> > 	tty_wait_until_sent(tty, timeout);
> > 	tty_lock();
> > }
> > 
> > to deal with that, if only we can sort the lock ordering with .
> 
> Ah, it looks like I just got the reason why port->mutex is locked in the
> top of uart_close. In uart, TTY_CLOSING flag is not used. So there is
> nothing to protect against races between ->close (the code between the
> two spinlock critical sections corresponding to port_close_start and
> _end) and ->open (block_til_ready).
> 
> Other than that I see no point for the lock to be in the beginning. So
> if we introduce CLOSING flag (I do that in my patches implicitly),
> everything should be OK:
> * port->count etc is and always was protected by the spinlock,
> * ->stop_rx stands as I wrote earlier.
> * uart_wait_until_sent -- that one is already called without port->mutex
> from set_termios and tty_set_ldisc.
> 
> So it looks like we should:
> - introduce CLOSING flag
> - move the lock below, before shutdown
> - introduce your magic _from_close helper
> - use it
> 
> Doing this after we have all the helpers in place would be easier. There
> would be no need to play with CLOSING bit. But there will be no option
> to backport this to stable trees then. And I know I will have to do that
> at least for 3.0.
> 
> Note that we may use the _from_close helper from tty_port_close_start
> almost instantly. All users should not hold port->mutex over
> tty_port_close_start. But I need to check. Tomorrow.
> 
> In the meantime, comments welcome.

So, is your original patch you sent in this thread still needed?

confused,

greg k-h

Re: patch "TTY: remove tty_locked" added to tty tree

[Greg KH]

On Wed, Aug 24, 2011 at 11:27:19PM +0200, Jiri Slaby wrote:
> On 08/24/2011 04:35 PM, Arnd Bergmann wrote:
> > On Wednesday 24 August 2011, Jiri Slaby wrote:
> >> On 08/24/2011 01:20 PM, Arnd Bergmann wrote:
> >>> It's not clear to me what state->mutex protects in the serial_core, but
> >>> it has been around forever (used to be called state->sem)
> >>
> >> It was actually moved in uart_close back in 2003. Formerly (when there
> >> was only a coarse grained port_sem) it was right before uart_shutdown.
> >> But there were some flags to handle some races. I'm not sure whether the
> >> flags protected any race here though.
> > 
> > ok
> > 
> >>> and is held in
> >>> all uart functions, which is at least consistent. IIRC what Alan's plan
> >>> for this was, uart_close should eventually get changed to use
> >>> tty_port_close_start or even tty_port_close. Maybe the time for that has
> >>> come now, lacking better alternatives?
> >>
> >> Yes, I have such a patch in my queue. But it's not easy to get there.
> >> You may take a look at:
> >> http://decibel.fi.muni.cz/gitweb/?p=linux.git;a=shortlog;h=refs/heads/devel
> >>
> >> But it's still far from ready. And yet, in the queue, I still have
> >> port->mutex locked before tty_port_close_start like it is now.
> > 
> > Ah, right. I still don't see why the port->mutex is or is not needed there,
> > and I think that's the main issue.
> > 
> > By comparison, getting *_wait_until_sent to be called without BTM seems
> > easy -- we know that all callers from ->close() hold it, while the ones
> > from ->ioctl() don't. We could have a helper like
> > 
> > void tty_wait_until_sent_from_close(struct tty_struct *tty, long timeout)
> > {
> > 	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
> > 	tty_wait_until_sent(tty, timeout);
> > 	tty_lock();
> > }
> > 
> > to deal with that, if only we can sort the lock ordering with .
> 
> Ah, it looks like I just got the reason why port->mutex is locked in the
> top of uart_close. In uart, TTY_CLOSING flag is not used. So there is
> nothing to protect against races between ->close (the code between the
> two spinlock critical sections corresponding to port_close_start and
> _end) and ->open (block_til_ready).
> 
> Other than that I see no point for the lock to be in the beginning. So
> if we introduce CLOSING flag (I do that in my patches implicitly),
> everything should be OK:
> * port->count etc is and always was protected by the spinlock,
> * ->stop_rx stands as I wrote earlier.
> * uart_wait_until_sent -- that one is already called without port->mutex
> from set_termios and tty_set_ldisc.
> 
> So it looks like we should:
> - introduce CLOSING flag
> - move the lock below, before shutdown
> - introduce your magic _from_close helper
> - use it
> 
> Doing this after we have all the helpers in place would be easier. There
> would be no need to play with CLOSING bit. But there will be no option
> to backport this to stable trees then. And I know I will have to do that
> at least for 3.0.
> 
> Note that we may use the _from_close helper from tty_port_close_start
> almost instantly. All users should not hold port->mutex over
> tty_port_close_start. But I need to check. Tomorrow.
> 
> In the meantime, comments welcome.

So, is your original patch you sent in this thread still needed?

confused,

greg k-h

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

On 08/24/2011 11:42 PM, Greg KH wrote:
> So, is your original patch you sent in this thread still needed?

If you mean "TTY: remove tty_locked", then yes, it's fine to see
tty_locked go. It looks like we won't need it to fix the stalls in the
end. If you mean TTY-fix-stalls-on-BTM-when-waiting-until-sent, that one
will be superseded by a new one.

thanks,
-- 
js
suse labs

Re: patch "TTY: remove tty_locked" added to tty tree

[Jiri Slaby]

On 08/24/2011 11:42 PM, Greg KH wrote:
> So, is your original patch you sent in this thread still needed?

If you mean "TTY: remove tty_locked", then yes, it's fine to see
tty_locked go. It looks like we won't need it to fix the stalls in the
end. If you mean TTY-fix-stalls-on-BTM-when-waiting-until-sent, that one
will be superseded by a new one.

thanks,
-- 
js
suse labs

Re: patch "TTY: remove tty_locked" added to tty tree

[Greg KH]

On Wed, Aug 24, 2011 at 11:48:22PM +0200, Jiri Slaby wrote:
> On 08/24/2011 11:42 PM, Greg KH wrote:
> > So, is your original patch you sent in this thread still needed?
> 
> If you mean "TTY: remove tty_locked", then yes, it's fine to see
> tty_locked go.

Good, that's already in my tree :)

> It looks like we won't need it to fix the stalls in the end. If you
> mean TTY-fix-stalls-on-BTM-when-waiting-until-sent, that one will be
> superseded by a new one.

Ok, I'll wait for that one then, thanks.

greg k-h

Re: patch "TTY: remove tty_locked" added to tty tree

[Greg KH]

On Wed, Aug 24, 2011 at 11:48:22PM +0200, Jiri Slaby wrote:
> On 08/24/2011 11:42 PM, Greg KH wrote:
> > So, is your original patch you sent in this thread still needed?
> 
> If you mean "TTY: remove tty_locked", then yes, it's fine to see
> tty_locked go.

Good, that's already in my tree :)

> It looks like we won't need it to fix the stalls in the end. If you
> mean TTY-fix-stalls-on-BTM-when-waiting-until-sent, that one will be
> superseded by a new one.

Ok, I'll wait for that one then, thanks.

greg k-h

[PATCH 1/5] TTY: serial, use ASYNCB_CLOSING in uart_close

[Jiri Slaby]

We need to move port->mutex locking after wait_until_sent in
uart_close (for rationale see next patches). But if we did it now, we
would introduce a race between close and open. This is exactly why
port->mutex is locked at the top of uart_close.

To avoid the race, we add ASYNCB_CLOSING to uart_close. Like every
other sane TTY driver. Thanks to tty_port_block_til_ready used in
uart_open we will have this for free. Then we can move the port->mutex
lock.

Also note that this will make the conversion to tty_port helpers
easier. They are currently handling ASYNC_CLOSING flag correctly.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/serial/serial_core.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 781d566..cd7a0bb 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1293,6 +1293,7 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	 * the line discipline to only process XON/XOFF characters by
 	 * setting tty->closing.
 	 */
+	set_bit(ASYNCB_CLOSING, &port->flags);
 	tty->closing = 1;
 	spin_unlock_irqrestore(&port->lock, flags);
 
@@ -1340,8 +1341,10 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	 * Wake up anyone trying to open this port.
 	 */
 	clear_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
+	clear_bit(ASYNCB_CLOSING, &port->flags);
 	spin_unlock_irqrestore(&port->lock, flags);
 	wake_up_interruptible(&port->open_wait);
+	wake_up_interruptible(&port->close_wait);
 
 done:
 	mutex_unlock(&port->mutex);
-- 
1.7.6

[PATCH 2/5] TTY: serial, move locking in uart_close

[Jiri Slaby]

So now, when we handle CLOSING flag, there is no point to hold
port->mutex over the start of uart_close.

Yes, there are still several things to reason about:
* port->count etc is and always was protected by a spinlock
* ->stop_rx is protected by a spinlock. Otherwise it would
  race with interrupts.
* uart_wait_until_sent -- that one is already called without
  port->mutex from set_termios and tty_set_ldisc. Should anything
  be protected there, it would be tx_empty. And by a spinlock.
  8250 does this internally...

This step is needed to fix system stalls. To not create an AB-BA lock
dependency (see next patches).

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/serial/serial_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index cd7a0bb..e1bff1b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1258,7 +1258,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 
 	pr_debug("uart_close(%d) called\n", uport->line);
 
-	mutex_lock(&port->mutex);
 	spin_lock_irqsave(&port->lock, flags);
 
 	if (tty_hung_up_p(filp)) {
@@ -1317,6 +1316,7 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 		uart_wait_until_sent(tty, uport->timeout);
 	}
 
+	mutex_lock(&port->mutex);
 	uart_shutdown(tty, state);
 	uart_flush_buffer(tty);
 
-- 
1.7.6

[PATCH 3/5] TTY: define tty_wait_until_sent_from_close

[Jiri Slaby]

We need this helper to fix system stalls. The issue is that the rest
of the system TTYs wait for us to finish waiting. This wasn't an issue
with BKL. BKL used to unlock implicitly.

This is based on the Arnd suggestion.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/tty.h |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/linux/tty.h b/include/linux/tty.h
index a7720a4..a0bf319 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -604,6 +604,24 @@ extern void __lockfunc tty_lock(void) __acquires(tty_lock);
 extern void __lockfunc tty_unlock(void) __releases(tty_lock);
 
 /*
+ * this shall be called only from where BTM is held (like close)
+ *
+ * We need this to ensure nobody waits for us to finish while we are waiting.
+ * Without this we were encountering system stalls.
+ *
+ * This should be indeed removed with BTM removal later.
+ *
+ * Locking: BTM required. Nobody is allowed to hold port->mutex.
+ */
+static inline void tty_wait_until_sent_from_close(struct tty_struct *tty,
+		long timeout)
+{
+	tty_unlock(); /* tty->ops->close holds the BTM, drop it while waiting */
+	tty_wait_until_sent(tty, timeout);
+	tty_lock();
+}
+
+/*
  * wait_event_interruptible_tty -- wait for a condition with the tty lock held
  *
  * The condition we are waiting for might take a long time to
-- 
1.7.6

[PATCH 4/5] TTY: use tty_wait_until_sent_from_close in tty_port_close_start

[Jiri Slaby]

Let's use the newly added helper to avoid stalls in drivers which are
already ported to tty_port helpers.

We have to ensure here, that there is no user of tty_port_close_start
and tty_port_close which holds port->mutex (or other) lock over them.
And sure, there is none.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/tty_port.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 33d37d2..ef9dd62 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -350,7 +350,7 @@ int tty_port_close_start(struct tty_port *port,
 		tty_driver_flush_buffer(tty);
 	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
 			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent(tty, port->closing_wait);
+		tty_wait_until_sent_from_close(tty, port->closing_wait);
 	if (port->drain_delay) {
 		unsigned int bps = tty_get_baud_rate(tty);
 		long timeout;
-- 
1.7.6

[PATCH 5/5] TTY: use tty_wait_until_sent_from_close in other drivers

[Jiri Slaby]

Let's use the newly added helper to avoid stalls in drivers which are
not yet ported to tty_port helpers.

Those which are broken (call tty_wait_until_sent with irqs disabled)
are left untouched. They are in a deeper trouble than we are trying to
solve here. This includes amiserial, 68328serial, 68360serial and
crisv10.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/isdn/i4l/isdn_tty.c      |    2 +-
 drivers/tty/hvc/hvc_console.c    |    2 +-
 drivers/tty/hvc/hvcs.c           |    2 +-
 drivers/tty/serial/serial_core.c |    3 ++-
 net/irda/ircomm/ircomm_tty.c     |    2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index d850427..e5546cb 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -1693,7 +1693,7 @@ isdn_tty_close(struct tty_struct *tty, struct file *filp)
 	 * line status register.
 	 */
 	if (info->flags & ISDN_ASYNC_INITIALIZED) {
-		tty_wait_until_sent(tty, 3000);	/* 30 seconds timeout */
+		tty_wait_until_sent_from_close(tty, 3000);	/* 30 seconds timeout */
 		/*
 		 * Before we drop DTR, make sure the UART transmitter
 		 * has completely drained; this is especially
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index e1aaf4f..b6b2d18 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -388,7 +388,7 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
 		 * there is no buffered data otherwise sleeps on a wait queue
 		 * waking periodically to check chars_in_buffer().
 		 */
-		tty_wait_until_sent(tty, HVC_CLOSE_WAIT);
+		tty_wait_until_sent_from_close(tty, HVC_CLOSE_WAIT);
 	} else {
 		if (hp->count < 0)
 			printk(KERN_ERR "hvc_close %X: oops, count is %d\n",
diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 4c8b665..e523773 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -1237,7 +1237,7 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
 		irq = hvcsd->vdev->irq;
 		spin_unlock_irqrestore(&hvcsd->lock, flags);
 
-		tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
+		tty_wait_until_sent_from_close(tty, HVCS_CLOSE_WAIT);
 
 		/*
 		 * This line is important because it tells hvcs_open that this
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index e1bff1b..5c04cb9 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1297,7 +1297,8 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	if (port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent(tty, msecs_to_jiffies(port->closing_wait));
+		tty_wait_until_sent_from_close(tty,
+				msecs_to_jiffies(port->closing_wait));
 
 	/*
 	 * At this point, we stop accepting input.  To do this, we
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index b3cc8b3..253695d 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -551,7 +551,7 @@ static void ircomm_tty_close(struct tty_struct *tty, struct file *filp)
 	 */
 	tty->closing = 1;
 	if (self->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent(tty, self->closing_wait);
+		tty_wait_until_sent_from_close(tty, self->closing_wait);
 
 	ircomm_tty_shutdown(self);
 
-- 
1.7.6

Re: [PATCH 1/5] TTY: serial, use ASYNCB_CLOSING in uart_close

[Arnd Bergmann]

On Thursday 25 August 2011, Jiri Slaby wrote:
> 
> We need to move port->mutex locking after wait_until_sent in
> uart_close (for rationale see next patches). But if we did it now, we
> would introduce a race between close and open. This is exactly why
> port->mutex is locked at the top of uart_close.
> 
> To avoid the race, we add ASYNCB_CLOSING to uart_close. Like every
> other sane TTY driver. Thanks to tty_port_block_til_ready used in
> uart_open we will have this for free. Then we can move the port->mutex
> lock.
> 
> Also note that this will make the conversion to tty_port helpers
> easier. They are currently handling ASYNC_CLOSING flag correctly.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

The series looks good to me,

Acked-by: Arnd Bergmann <arnd@arndb.de>

The only thing left to note is probably that there are a bunch
of drivers that implement their own *_wait_until_sent function
and call that from their *_close function instead of calling
tty_wait_until_sent. These drivers continue to suffer from the
same stalls that you are fixing the other ones.

	Arnd