From: Peter Hurley <hidden> Date: 2014-06-16 13:22:07
tty_wait_until_sent_from_close() drops the tty lock while waiting
for the tty driver to finish sending previously accepted data (ie.,
data remaining in its write buffer and transmit fifo).
However, dropping the tty lock is a hold-over from when the tty
lock was system-wide; ie., one lock for all ttys.
Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
'tty: localise the lock', dropping the tty lock has not been necessary.
CC: Karsten Keil <redacted>
CC: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Peter Hurley <redacted>
---
drivers/isdn/i4l/isdn_tty.c | 2 +-
drivers/tty/hvc/hvc_console.c | 2 +-
drivers/tty/hvc/hvcs.c | 2 +-
drivers/tty/tty_port.c | 11 ++---------
include/linux/tty.h | 18 ------------------
5 files changed, 5 insertions(+), 30 deletions(-)
@@ -644,24 +644,6 @@ extern void __lockfunc tty_unlock_pair(struct tty_struct *tty,structtty_struct*tty2);/*-*thisshallbecalledonlyfromwhereBTMisheld(likeclose)-*-*Weneedthistoensurenobodywaitsforustofinishwhilewearewaiting.-*Withoutthiswewereencounteringsystemstalls.-*-*ThisshouldbeindeedremovedwithBTMremovallater.-*-*Locking:BTMrequired.Nobodyisallowedtoholdport->mutex.-*/-staticinlinevoidtty_wait_until_sent_from_close(structtty_struct*tty,-longtimeout)-{-tty_unlock(tty);/* tty->ops->close holds the BTM, drop it while waiting */-tty_wait_until_sent(tty,timeout);-tty_lock(tty);-}--/**wait_event_interruptible_tty--waitforaconditionwiththettylockheld**Theconditionwearewaitingformighttakealongtimeto
On Monday 16 June 2014 09:17:11 Peter Hurley wrote:
tty_wait_until_sent_from_close() drops the tty lock while waiting
for the tty driver to finish sending previously accepted data (ie.,
data remaining in its write buffer and transmit fifo).
However, dropping the tty lock is a hold-over from when the tty
lock was system-wide; ie., one lock for all ttys.
Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
'tty: localise the lock', dropping the tty lock has not been necessary.
CC: Karsten Keil <redacted>
CC: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Peter Hurley <redacted>
I don't understand the second half of the changelog, it doesn't seem
to fit here: there deadlock that we are trying to avoid here happens
when the *same* tty needs the lock to complete the function that
sends the pending data. I don't think we do still do that any more,
but it doesn't seem related to the tty lock being system-wide or not.
Arnd
From: Peter Hurley <hidden> Date: 2014-06-17 10:57:15
On 06/17/2014 04:00 AM, Arnd Bergmann wrote:
On Monday 16 June 2014 09:17:11 Peter Hurley wrote:
quoted
tty_wait_until_sent_from_close() drops the tty lock while waiting
for the tty driver to finish sending previously accepted data (ie.,
data remaining in its write buffer and transmit fifo).
However, dropping the tty lock is a hold-over from when the tty
lock was system-wide; ie., one lock for all ttys.
Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
'tty: localise the lock', dropping the tty lock has not been necessary.
CC: Karsten Keil <redacted>
CC: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Peter Hurley <redacted>
I don't understand the second half of the changelog, it doesn't seem
to fit here: there deadlock that we are trying to avoid here happens
when the *same* tty needs the lock to complete the function that
sends the pending data. I don't think we do still do that any more,
but it doesn't seem related to the tty lock being system-wide or not.
The tty lock is not used in the i/o path; it's purpose is to
mutually exclude state changes in open(), close() and hangup().
The commit that added this [1] comments that _other_ ttys may wait
for this tty to complete, and comments in the code note that this
function should be removed when the system-wide tty mutex was removed
(which happened with the commit noted in the changelog).
Regards,
Peter Hurley
[1]
commit a57a7bf3fc7eff00f07eb9c805774d911a3f2472
Author: Jiri Slaby [off-list ref]
Date: Thu Aug 25 15:12:06 2011 +0200
TTY: define tty_wait_until_sent_from_close
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 [off-list ref]
Acked-by: Arnd Bergmann [off-list ref]
Signed-off-by: Greg Kroah-Hartman [off-list ref]
On Tuesday 17 June 2014 11:03:50 David Laight wrote:
From: Peter Hurley
...
quoted
quoted
I don't understand the second half of the changelog, it doesn't seem
to fit here: there deadlock that we are trying to avoid here happens
when the *same* tty needs the lock to complete the function that
sends the pending data. I don't think we do still do that any more,
but it doesn't seem related to the tty lock being system-wide or not.
The tty lock is not used in the i/o path; it's purpose is to
mutually exclude state changes in open(), close() and hangup().
The commit that added this [1] comments that _other_ ttys may wait
for this tty to complete, and comments in the code note that this
function should be removed when the system-wide tty mutex was removed
(which happened with the commit noted in the changelog).
What happens if another process tries to do a non-blocking open
while you are sleeping in close waiting for output to drain?
Hopefully this returns before that data has drained.
Before the patch, I believe tty_reopen() would return -EIO because
the TTY_CLOSING flag is set. After the patch, tty_open() blocks
on tty_lock() before calling tty_reopen(). AFAICT, this is independent
of O_NONBLOCK.
Arnd
From: Peter Hurley <hidden> Date: 2014-06-17 11:32:20
On 06/17/2014 07:03 AM, David Laight wrote:
From: Peter Hurley
...
quoted
quoted
I don't understand the second half of the changelog, it doesn't seem
to fit here: there deadlock that we are trying to avoid here happens
when the *same* tty needs the lock to complete the function that
sends the pending data. I don't think we do still do that any more,
but it doesn't seem related to the tty lock being system-wide or not.
The tty lock is not used in the i/o path; it's purpose is to
mutually exclude state changes in open(), close() and hangup().
The commit that added this [1] comments that _other_ ttys may wait
for this tty to complete, and comments in the code note that this
function should be removed when the system-wide tty mutex was removed
(which happened with the commit noted in the changelog).
What happens if another process tries to do a non-blocking open
while you are sleeping in close waiting for output to drain?
Hopefully this returns before that data has drained.
Good point.
tty_open() should be trylocking both mutexes anyway in O_NONBLOCK.
Regards,
Peter Hurley
From: One Thousand Gnomes <hidden> Date: 2014-06-17 11:55:39
Before the patch, I believe tty_reopen() would return -EIO because
the TTY_CLOSING flag is set. After the patch, tty_open() blocks
on tty_lock() before calling tty_reopen(). AFAICT, this is independent
of O_NONBLOCK.
That would be a bug then. Returning -EIO is fine (if unfriendly). The
O_NONBLOCK can't block in this case though because the port could take a
long time to give up trying to dribble its bits (up to 30 seconds or so)
Alan
From: Peter Hurley <hidden> Date: 2014-07-09 13:57:57
On 06/17/2014 07:32 AM, Peter Hurley wrote:
On 06/17/2014 07:03 AM, David Laight wrote:
quoted
From: Peter Hurley
...
quoted
quoted
I don't understand the second half of the changelog, it doesn't seem
to fit here: there deadlock that we are trying to avoid here happens
when the *same* tty needs the lock to complete the function that
sends the pending data. I don't think we do still do that any more,
but it doesn't seem related to the tty lock being system-wide or not.
The tty lock is not used in the i/o path; it's purpose is to
mutually exclude state changes in open(), close() and hangup().
The commit that added this [1] comments that _other_ ttys may wait
for this tty to complete, and comments in the code note that this
function should be removed when the system-wide tty mutex was removed
(which happened with the commit noted in the changelog).
What happens if another process tries to do a non-blocking open
while you are sleeping in close waiting for output to drain?
Hopefully this returns before that data has drained.
Good point.
tty_open() should be trylocking both mutexes anyway in O_NONBLOCK.
Further, the tty lock should not be nested within the tty_mutex lock
in a reopen, regardless of O_NONBLOCK.
AFAICT, the tty_mutex in the reopen scenario is only protecting the
tty count bump of the linked tty (if the tty is a pty).
I think with some refactoring and returning with a tty reference held
from both tty_open_current_tty() and tty_driver_lookup_tty(), the tty
lock in tty_open() can be attempted without nesting in the tty_mutex.
Regardless, I'll be splitting this series and I'll be sure to cc
you all when I resubmit these changes (after testing).
Regards,
Peter Hurley
On Mon, Jun 16, 2014 at 09:17:11AM -0400, Peter Hurley wrote:
tty_wait_until_sent_from_close() drops the tty lock while waiting
for the tty driver to finish sending previously accepted data (ie.,
data remaining in its write buffer and transmit fifo).
However, dropping the tty lock is a hold-over from when the tty
lock was system-wide; ie., one lock for all ttys.
Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
'tty: localise the lock', dropping the tty lock has not been necessary.
CC: Karsten Keil <redacted>
CC: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Peter Hurley <redacted>
---
drivers/isdn/i4l/isdn_tty.c | 2 +-
drivers/tty/hvc/hvc_console.c | 2 +-
drivers/tty/hvc/hvcs.c | 2 +-
drivers/tty/tty_port.c | 11 ++---------
include/linux/tty.h | 18 ------------------
5 files changed, 5 insertions(+), 30 deletions(-)
I've applied the first 13 patches in this series, as it looks like you
were going to split things up from here, right? Can you refresh these
and resend when you have that done?
thanks,
greg k-h
From: Peter Hurley <hidden> Date: 2014-07-11 15:03:24
On 07/10/2014 07:09 PM, Greg Kroah-Hartman wrote:
On Mon, Jun 16, 2014 at 09:17:11AM -0400, Peter Hurley wrote:
quoted
tty_wait_until_sent_from_close() drops the tty lock while waiting
for the tty driver to finish sending previously accepted data (ie.,
data remaining in its write buffer and transmit fifo).
However, dropping the tty lock is a hold-over from when the tty
lock was system-wide; ie., one lock for all ttys.
Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
'tty: localise the lock', dropping the tty lock has not been necessary.
CC: Karsten Keil <redacted>
CC: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Peter Hurley <redacted>
---
drivers/isdn/i4l/isdn_tty.c | 2 +-
drivers/tty/hvc/hvc_console.c | 2 +-
drivers/tty/hvc/hvcs.c | 2 +-
drivers/tty/tty_port.c | 11 ++---------
include/linux/tty.h | 18 ------------------
5 files changed, 5 insertions(+), 30 deletions(-)
I've applied the first 13 patches in this series, as it looks like you
were going to split things up from here, right?
Yes, thanks for doing that.
Can you refresh these and resend when you have that done?
Unfortunately, that probably won't be until after the 3.17 merge window,
for 3.18. The tty_open() rework is not trivial and there is an issue
with the ldisc flush removal patch.
I'm hoping to include the tty flow control fixes with that stuff as well.
Regards,
Peter Hurley
From: Peter Hurley <hidden> Date: 2014-10-08 03:56:12
On 06/17/2014 07:03 AM, David Laight wrote:
From: Peter Hurley
...
quoted
quoted
I don't understand the second half of the changelog, it doesn't seem
to fit here: there deadlock that we are trying to avoid here happens
when the *same* tty needs the lock to complete the function that
sends the pending data. I don't think we do still do that any more,
but it doesn't seem related to the tty lock being system-wide or not.
The tty lock is not used in the i/o path; it's purpose is to
mutually exclude state changes in open(), close() and hangup().
The commit that added this [1] comments that _other_ ttys may wait
for this tty to complete, and comments in the code note that this
function should be removed when the system-wide tty mutex was removed
(which happened with the commit noted in the changelog).
I just wanted to revisit this discussion briefly so I can clarify the
situation regarding holding the tty lock while closing, and how that
affects parallel opens.
I've unnested the tty lock from the tty mutex (which I'm still testing)
but will be submitting after the merge window re-opens for 3.19. So this
is more relevant now.
The original patch that led to this thread is here:
https://lkml.org/lkml/2014/6/16/306
What happens if another process tries to do a non-blocking open
while you are sleeping in close waiting for output to drain?
Hopefully this returns before that data has drained.
Current mainline blocks on _any_ racing re-open while this lock is
dropped in tty_wait_until_sent_from_close(); blocking while
ASYNC_CLOSING has been in mainline since at least 2.6.29 and that just
merged existing code together. See tty_port_block_til_ready(); note
the test for O_NONBLOCK is after the wait while ASYNC_CLOSING.
IOW, currently a non-blocking open will sleep for the _entire_ duration
of a parallel hardware shutdown, and when it wakes, the error return will
cause a release of its tty, and it will restart with a fresh attempt
to open. Same with a blocking open that is already waiting; when its
woken the hardware shutdown has already completed so ASYNC_INITIALIZED
is cleared, which forces a release and restart too.
The point being that holding the tty lock across the _entire_ close
is equivalent to the current outcome, regardless of O_NONBLOCK.
I'm reluctant to start returning EGAIN for non-blocking tty opens
because no tty driver does that now, and I don't think userspace will
deal well with new return codes from tty opens.
Regards,
Peter Hurley
From: One Thousand Gnomes <hidden> Date: 2014-10-10 08:59:25
The point being that holding the tty lock across the _entire_ close
is equivalent to the current outcome, regardless of O_NONBLOCK.
I'm reluctant to start returning EGAIN for non-blocking tty opens
because no tty driver does that now, and I don't think userspace will
deal well with new return codes from tty opens.
I do not know about the non blocking case mattering. The blocking open
does need to wait, when I broke that case before I broke the console
login drivers (mingetty).
Returning EAGAIN would also only work if poll/select did the right thing.
Currently Linux can't support a System5 style ttymon process because of
this limitation, which means, for example, that systemd can't implement a
single thread to manage all console prompts/setup
Alan