* [PATCH 2/5] TTY: ldisc, move wait idle to caller
2011-11-16 15:27 [PATCH 1/5] TTY: ldisc, allow waiting for ldisc arbitrarily long Jiri Slaby
@ 2011-11-16 15:27 ` Jiri Slaby
2011-12-01 23:01 ` Tony Luck
2011-11-16 15:27 ` [PATCH 3/5] TTY: ldisc, wait for ldisc infinitely in hangup Jiri Slaby
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Jiri Slaby @ 2011-11-16 15:27 UTC (permalink / raw)
To: gregkh
Cc: linux-kernel, jirislaby, Dave Young, Dave Jones, Ben Hutchings,
Dmitriy Matrosov, Alan Cox, stable
It is the only place where reinit is called from. And we really need
to wait for the old ldisc to go once. Actually this is the place where
the waiting originally was (before removed and re-added later).
This will make the fix in the following patch easier to implement.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Dave Young <hidave.darkstar@gmail.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Dmitriy Matrosov <sgf.dma@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: stable <stable@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
drivers/tty/tty_ldisc.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 534d176..a69a755 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -763,8 +763,6 @@ static int tty_ldisc_reinit(struct tty_struct *tty, int ldisc)
if (IS_ERR(ld))
return -1;
- WARN_ON_ONCE(tty_ldisc_wait_idle(tty, 5 * HZ));
-
tty_ldisc_close(tty, tty->ldisc);
tty_ldisc_put(tty->ldisc);
tty->ldisc = NULL;
@@ -848,6 +846,8 @@ void tty_ldisc_hangup(struct tty_struct *tty)
it means auditing a lot of other paths so this is
a FIXME */
if (tty->ldisc) { /* Not yet closed */
+ WARN_ON_ONCE(tty_ldisc_wait_idle(tty, 5 * HZ));
+
if (reset == 0) {
if (!tty_ldisc_reinit(tty, tty->termios->c_line))
--
1.7.7.3
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/5] TTY: ldisc, move wait idle to caller
2011-11-16 15:27 ` [PATCH 2/5] TTY: ldisc, move wait idle to caller Jiri Slaby
@ 2011-12-01 23:01 ` Tony Luck
2011-12-02 0:40 ` Tony Luck
2011-12-02 8:21 ` Jiri Slaby
0 siblings, 2 replies; 9+ messages in thread
From: Tony Luck @ 2011-12-01 23:01 UTC (permalink / raw)
To: Jiri Slaby
Cc: gregkh, linux-kernel, jirislaby, Dave Young, Dave Jones,
Ben Hutchings, Dmitriy Matrosov, Alan Cox, stable
One of my systems (HP rx2620 - ia64) has started hanging during shutdown.
Last messages on the serial console are:
Shutting down service (localfs) network . . . . . . . . . done
Shutting down D-Bus daemon done
Running /etc/ini
and there it hangs forever in mid word (totally repeatable stops at "ini" every
time).
git bisect blames:
commit 300420722e0734a4254f3b634e0f82664495d210
Author: Jiri Slaby <jslaby@suse.cz>
Date: Wed Nov 16 16:27:08 2011 +0100
TTY: ldisc, move wait idle to caller
[Though the following commit "TTY: ldisc, wait for ldisc infinitely in hangup"
sounds like it fits the symptoms]
Ideas?
-Tony
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/5] TTY: ldisc, move wait idle to caller
2011-12-01 23:01 ` Tony Luck
@ 2011-12-02 0:40 ` Tony Luck
2011-12-02 8:21 ` Jiri Slaby
1 sibling, 0 replies; 9+ messages in thread
From: Tony Luck @ 2011-12-02 0:40 UTC (permalink / raw)
To: Jiri Slaby
Cc: gregkh, linux-kernel, jirislaby, Dave Young, Dave Jones,
Ben Hutchings, Dmitriy Matrosov, Alan Cox, stable
On Thu, Dec 1, 2011 at 3:01 PM, Tony Luck <tony.luck@intel.com> wrote:
> [Though the following commit "TTY: ldisc, wait for ldisc infinitely in hangup"
> sounds like it fits the symptoms]
Confirmed - reverting the "wait for ldisc infinitely" cures the hang problem (I
do seem to be hitting the 5 second delay, and there is a pause that feels
about 5 seconds long)
-Tony
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/5] TTY: ldisc, move wait idle to caller
2011-12-01 23:01 ` Tony Luck
2011-12-02 0:40 ` Tony Luck
@ 2011-12-02 8:21 ` Jiri Slaby
2011-12-02 18:24 ` Tony Luck
1 sibling, 1 reply; 9+ messages in thread
From: Jiri Slaby @ 2011-12-02 8:21 UTC (permalink / raw)
To: Tony Luck
Cc: gregkh, linux-kernel, jirislaby, Dave Young, Dave Jones,
Ben Hutchings, Dmitriy Matrosov, Alan Cox, stable
[-- Attachment #1: Type: text/plain, Size: 853 bytes --]
On 12/02/2011 12:01 AM, Tony Luck wrote:
> One of my systems (HP rx2620 - ia64) has started hanging during shutdown.
> Last messages on the serial console are:
>
> Shutting down service (localfs) network . . . . . . . . . done
> Shutting down D-Bus daemon done
> Running /etc/ini
>
> and there it hangs forever in mid word (totally repeatable stops at "ini" every
> time).
>
> git bisect blames:
>
> commit 300420722e0734a4254f3b634e0f82664495d210
> Author: Jiri Slaby <jslaby@suse.cz>
> Date: Wed Nov 16 16:27:08 2011 +0100
>
> TTY: ldisc, move wait idle to caller
>
> [Though the following commit "TTY: ldisc, wait for ldisc infinitely in hangup"
> sounds like it fits the symptoms]
>
> Ideas?
Thanks. What distro and init do you use? Does the attached patch help?
--
js
suse labs
[-- Attachment #2: 0001-TTY-ldisc-don-t-wait-with-ldisc_mutex.patch --]
[-- Type: text/x-patch, Size: 1954 bytes --]
>From 6f778e80388f8b1ad65f0c8bfbc874704d77b3eb Mon Sep 17 00:00:00 2001
From: Jiri Slaby <jslaby@suse.cz>
Date: Fri, 2 Dec 2011 09:19:05 +0100
Subject: [PATCH 1/1] TTY: ldisc, don't wait with ldisc_mutex
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
drivers/tty/tty_ldisc.c | 16 +++++++++++++---
1 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 24b95db..c93e113 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -527,6 +527,14 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
flush_work_sync(&tty->buf.work);
}
+static bool tty_ldisc_is_idle(struct tty_struct *tty)
+{
+ bool idle;
+ mutex_lock(&tty->ldisc_mutex);
+ idle = !tty->ldisc || atomic_read(&tty->ldisc->users) == 1;
+ mutex_unlock(&tty->ldisc_mutex);
+ return idle;
+}
/**
* tty_ldisc_wait_idle - wait for the ldisc to become idle
* @tty: tty to wait for
@@ -534,12 +542,14 @@ static void tty_ldisc_flush_works(struct tty_struct *tty)
*
* Wait for the line discipline to become idle. The discipline must
* have been halted for this to guarantee it remains idle.
+ *
+ * Locking: neither BTM, nor tty->ldisc_mutex must be held
*/
static int tty_ldisc_wait_idle(struct tty_struct *tty, long timeout)
{
long ret;
- ret = wait_event_timeout(tty_ldisc_idle,
- atomic_read(&tty->ldisc->users) == 1, timeout);
+ ret = wait_event_timeout(tty_ldisc_idle, tty_ldisc_is_idle(tty),
+ timeout);
return ret > 0 ? 0 : -EBUSY;
}
@@ -830,6 +840,7 @@ retry:
if (atomic_read(&tty->ldisc->users) != 1) {
char cur_n[TASK_COMM_LEN], tty_n[64];
long timeout = 3 * HZ;
+ mutex_unlock(&tty->ldisc_mutex);
tty_unlock();
while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
@@ -839,7 +850,6 @@ retry:
__func__, get_task_comm(cur_n, current),
tty_name(tty, tty_n));
}
- mutex_unlock(&tty->ldisc_mutex);
goto retry;
}
--
1.7.7.3
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/5] TTY: ldisc, move wait idle to caller
2011-12-02 8:21 ` Jiri Slaby
@ 2011-12-02 18:24 ` Tony Luck
0 siblings, 0 replies; 9+ messages in thread
From: Tony Luck @ 2011-12-02 18:24 UTC (permalink / raw)
To: Jiri Slaby
Cc: gregkh, linux-kernel, jirislaby, Dave Young, Dave Jones,
Ben Hutchings, Dmitriy Matrosov, Alan Cox, stable
On Fri, Dec 2, 2011 at 12:21 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> Thanks. What distro and init do you use? Does the attached patch help?
Patch didn't apply cleanly ... it looks like you have another patch that
deleted these lines:
if (ret < 0)
return ret;
from tty_ldisc_wait_idle() ... so I think I hand applied around this by dropping
those lines too - but I might have goofed.
Anyway - the patch as modified by me didn't work. I still see the hang.
Base distro is SLES11-SP1
Init version: sysvinit-2.86-198.20
System is headless (no graphic card) so serial console is all I have.
ia64 early serial console is enabled with boot argument:
console=uart,mmio,0xff5e0000
-Tony
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/5] TTY: ldisc, wait for ldisc infinitely in hangup
2011-11-16 15:27 [PATCH 1/5] TTY: ldisc, allow waiting for ldisc arbitrarily long Jiri Slaby
2011-11-16 15:27 ` [PATCH 2/5] TTY: ldisc, move wait idle to caller Jiri Slaby
@ 2011-11-16 15:27 ` Jiri Slaby
2011-11-16 15:27 ` [PATCH 4/5] TTY: ldisc, remove some unneeded includes Jiri Slaby
2011-11-16 15:27 ` [PATCH 5/5] TTY: pty, cleanup the pty counting Jiri Slaby
3 siblings, 0 replies; 9+ messages in thread
From: Jiri Slaby @ 2011-11-16 15:27 UTC (permalink / raw)
To: gregkh
Cc: linux-kernel, jirislaby, Dave Young, Dave Jones, Ben Hutchings,
Dmitriy Matrosov, Alan Cox, stable
For /dev/console case, we do not kill all ldisc users. It's due to
redirected_tty_write test in __tty_hangup. In that case there still
might be a process waiting e.g. in n_tty_read for input.
We wait for such processes to disappear. The problem is that we use a
timeout. After this timeout, we continue closing the ldisc and start
freeing tty resources. It obviously leads to crashes when the other
process is woken.
So to fix this, we wait infinitely before reiniting the ldisc. (The
tiocsetd remains untouched -- times out after 5s.)
This is nicely reproducible with this run from shell:
exec 0<>/dev/console 1<>/dev/console 2<>/dev/console
and stopping a getty like:
systemctl stop serial-getty@ttyS0.service
The crash proper may be produced only under load or with constified
timing the same as for 92f6fa09b.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Dave Young <hidave.darkstar@gmail.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Dmitriy Matrosov <sgf.dma@gmail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: stable <stable@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
drivers/tty/tty_ldisc.c | 19 +++++++++++++++++--
1 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index a69a755..8e0924f 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -36,6 +36,7 @@
#include <linux/kmod.h>
#include <linux/nsproxy.h>
+#include <linux/ratelimit.h>
/*
* This guards the refcounted line discipline lists. The lock
@@ -837,7 +838,7 @@ void tty_ldisc_hangup(struct tty_struct *tty)
tty_unlock();
cancel_work_sync(&tty->buf.work);
mutex_unlock(&tty->ldisc_mutex);
-
+retry:
tty_lock();
mutex_lock(&tty->ldisc_mutex);
@@ -846,7 +847,21 @@ void tty_ldisc_hangup(struct tty_struct *tty)
it means auditing a lot of other paths so this is
a FIXME */
if (tty->ldisc) { /* Not yet closed */
- WARN_ON_ONCE(tty_ldisc_wait_idle(tty, 5 * HZ));
+ if (atomic_read(&tty->ldisc->users) != 1) {
+ char cur_n[TASK_COMM_LEN], tty_n[64];
+ long timeout = 3 * HZ;
+ tty_unlock();
+
+ while (tty_ldisc_wait_idle(tty, timeout) == -EBUSY) {
+ timeout = MAX_SCHEDULE_TIMEOUT;
+ printk_ratelimited(KERN_WARNING
+ "%s: waiting (%s) for %s took too long, but we keep waiting...\n",
+ __func__, get_task_comm(cur_n, current),
+ tty_name(tty, tty_n));
+ }
+ mutex_unlock(&tty->ldisc_mutex);
+ goto retry;
+ }
if (reset == 0) {
--
1.7.7.3
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 4/5] TTY: ldisc, remove some unneeded includes
2011-11-16 15:27 [PATCH 1/5] TTY: ldisc, allow waiting for ldisc arbitrarily long Jiri Slaby
2011-11-16 15:27 ` [PATCH 2/5] TTY: ldisc, move wait idle to caller Jiri Slaby
2011-11-16 15:27 ` [PATCH 3/5] TTY: ldisc, wait for ldisc infinitely in hangup Jiri Slaby
@ 2011-11-16 15:27 ` Jiri Slaby
2011-11-16 15:27 ` [PATCH 5/5] TTY: pty, cleanup the pty counting Jiri Slaby
3 siblings, 0 replies; 9+ messages in thread
From: Jiri Slaby @ 2011-11-16 15:27 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, jirislaby, Alan Cox
They were cut&pasted from tty_io. Many of them are not needed in
tty_ldisc.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@linux.intel.com>
---
drivers/tty/tty_ldisc.c | 22 ++--------------------
1 files changed, 2 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 8e0924f..6b344ae 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -1,19 +1,11 @@
#include <linux/types.h>
-#include <linux/major.h>
#include <linux/errno.h>
-#include <linux/signal.h>
-#include <linux/fcntl.h>
+#include <linux/kmod.h>
#include <linux/sched.h>
#include <linux/interrupt.h>
#include <linux/tty.h>
#include <linux/tty_driver.h>
-#include <linux/tty_flip.h>
-#include <linux/devpts_fs.h>
#include <linux/file.h>
-#include <linux/console.h>
-#include <linux/timer.h>
-#include <linux/ctype.h>
-#include <linux/kd.h>
#include <linux/mm.h>
#include <linux/string.h>
#include <linux/slab.h>
@@ -24,19 +16,9 @@
#include <linux/device.h>
#include <linux/wait.h>
#include <linux/bitops.h>
-#include <linux/delay.h>
#include <linux/seq_file.h>
-
-#include <linux/uaccess.h>
-#include <asm/system.h>
-
-#include <linux/kbd_kern.h>
-#include <linux/vt_kern.h>
-#include <linux/selection.h>
-
-#include <linux/kmod.h>
-#include <linux/nsproxy.h>
#include <linux/ratelimit.h>
+#include <linux/uaccess.h>
/*
* This guards the refcounted line discipline lists. The lock
--
1.7.7.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 5/5] TTY: pty, cleanup the pty counting
2011-11-16 15:27 [PATCH 1/5] TTY: ldisc, allow waiting for ldisc arbitrarily long Jiri Slaby
` (2 preceding siblings ...)
2011-11-16 15:27 ` [PATCH 4/5] TTY: ldisc, remove some unneeded includes Jiri Slaby
@ 2011-11-16 15:27 ` Jiri Slaby
3 siblings, 0 replies; 9+ messages in thread
From: Jiri Slaby @ 2011-11-16 15:27 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, jirislaby, Alan Cox
Instead of the hackish way of counting ptys, let's define a specific
->remove hook both from slave and master. And decrease the count only
for master.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@linux.intel.com>
---
drivers/tty/pty.c | 26 +++++++++-----------------
1 files changed, 9 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index e18604b..d8653ab 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -446,19 +446,8 @@ static inline void legacy_pty_init(void) { }
int pty_limit = NR_UNIX98_PTY_DEFAULT;
static int pty_limit_min;
static int pty_limit_max = NR_UNIX98_PTY_MAX;
-static int tty_count;
static int pty_count;
-static inline void pty_inc_count(void)
-{
- pty_count = (++tty_count) / 2;
-}
-
-static inline void pty_dec_count(void)
-{
- pty_count = (--tty_count) / 2;
-}
-
static struct cdev ptmx_cdev;
static struct ctl_table pty_table[] = {
@@ -600,8 +589,7 @@ static int pty_unix98_install(struct tty_driver *driver, struct tty_struct *tty)
*/
tty_driver_kref_get(driver);
tty->count++;
- pty_inc_count(); /* tty */
- pty_inc_count(); /* tty->link */
+ pty_count++;
return 0;
err_free_mem:
deinitialize_tty_struct(o_tty);
@@ -613,15 +601,19 @@ err_free_tty:
return -ENOMEM;
}
-static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
+static void ptm_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
+{
+ pty_count--;
+}
+
+static void pts_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
{
- pty_dec_count();
}
static const struct tty_operations ptm_unix98_ops = {
.lookup = ptm_unix98_lookup,
.install = pty_unix98_install,
- .remove = pty_unix98_remove,
+ .remove = ptm_unix98_remove,
.open = pty_open,
.close = pty_close,
.write = pty_write,
@@ -638,7 +630,7 @@ static const struct tty_operations ptm_unix98_ops = {
static const struct tty_operations pty_unix98_ops = {
.lookup = pts_unix98_lookup,
.install = pty_unix98_install,
- .remove = pty_unix98_remove,
+ .remove = pts_unix98_remove,
.open = pty_open,
.close = pty_close,
.write = pty_write,
--
1.7.7.3
^ permalink raw reply [flat|nested] 9+ messages in thread