From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755323Ab1G1N3p (ORCPT ); Thu, 28 Jul 2011 09:29:45 -0400 Received: from mail-fx0-f46.google.com ([209.85.161.46]:59475 "EHLO mail-fx0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753677Ab1G1N3n (ORCPT ); Thu, 28 Jul 2011 09:29:43 -0400 Message-ID: <4E316441.1080503@suse.cz> Date: Thu, 28 Jul 2011 17:29:37 +0400 From: Jiri Slaby User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20110716 Thunderbird/6.0 MIME-Version: 1.0 To: Andi Kleen CC: alan@lxorguk.ukuu.org.uk, torvalds@linux-foundation.org, gregkh@suse.de, ak@linux.intel.com, linux-kernel@vger.kernel.org, stable@kernel.org, tim.bird@am.sony.com Subject: Re: [PATCH] [69/99] TTY: ldisc, do not close until there are readers References: <20110727247.325703029@firstfloor.org> <20110727214908.895D42403FF@tassilo.jf.intel.com> In-Reply-To: <20110727214908.895D42403FF@tassilo.jf.intel.com> Content-Type: text/plain; charset=ISO-8859-2; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/28/2011 01:49 AM, Andi Kleen wrote: > 2.6.35-longterm review patch. If anyone has any objections, please let me know. > > ------------------ > From: Jiri Slaby > > commit 92f6fa09bd453ffe3351fa1f1377a1b7cfa911e6 upstream. > > We restored tty_ldisc_wait_idle in 100eeae2c5c (TTY: restore > tty_ldisc_wait_idle). We used it in the ldisc changing path to fix the > case where there are tasks in n_tty_read waiting for data and somebody > tries to change ldisc. > > Similar to the case above, there may be also tasks waiting in > n_tty_read while hangup is performed. As 65b770468e98 (tty-ldisc: turn > ldisc user count into a proper refcount) removed the wait-until-idle > from all paths, hangup path won't wait for them to disappear either > now. So add it back even to the hangup path. > > There is a difference, we need uninterruptible sleep as there is > obviously HUP signal pending. So tty_ldisc_wait_idle now sleeps > without possibility to be interrupted. This is what original > tty_ldisc_wait_idle did. After the wait idle reintroduction > (100eeae2c5c), we have had interruptible sleeps for the ldisc changing > path. But as there is a 5s timeout anyway, we don't allow it to be > interrupted from now on. It's not worth the added complexity of > deciding what kind of sleep we want. > > Before 65b770468e98 tty_ldisc_release was called also from > tty_ldisc_release. It is called from tty_release, so I don't think we > need to restore that one. > > This is nicely reproducible after constifying the timing when > drivers/tty/n_tty.c is patched as follows ("TTY: ntty, add one more > sanity check" patch is needed to actually see it explode): > %% -1548,6 +1549,7 @@ static int n_tty_open(struct tty_struct *tty) > > /* These are ugly. Currently a malloc failure here can panic */ > if (!tty->read_buf) { > + msleep(100); > tty->read_buf = kzalloc(N_TTY_BUF_SIZE, GFP_KERNEL); > if (!tty->read_buf) > return -ENOMEM; > %% -1785,6 +1788,7 @@ do_it_again: > break; > } > timeout = schedule_timeout(timeout); > + msleep(20); > continue; > } > __set_current_state(TASK_RUNNING); > ===== With a process: ===== > while (1) { > int fd = open(argv[1], O_RDWR); > read(fd, buf, sizeof(buf)); > close(fd); > } > ===== and its child: ===== > setsid(); > while (1) { > int fd = open(tty, O_RDWR|O_NOCTTY); > ioctl(fd, TIOCSCTTY, 1); > vhangup(); > close(fd); > usleep(100 * (10 + random() % 1000)); > } > ===== EOF ===== > > References: https://bugzilla.novell.com/show_bug.cgi?id=693374 > References: https://bugzilla.novell.com/show_bug.cgi?id=694509 > Signed-off-by: Jiri Slaby > Cc: Alan Cox > Cc: Linus Torvalds > Signed-off-by: Greg Kroah-Hartman > Signed-off-by: Andi Kleen > > --- > drivers/char/tty_ldisc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Index: linux-2.6.35.y/drivers/char/tty_ldisc.c > =================================================================== > --- linux-2.6.35.y.orig/drivers/char/tty_ldisc.c > +++ linux-2.6.35.y/drivers/char/tty_ldisc.c > @@ -543,7 +543,7 @@ static int tty_ldisc_halt(struct tty_str > static int tty_ldisc_wait_idle(struct tty_struct *tty) > { > int ret; > - ret = wait_event_interruptible_timeout(tty_ldisc_idle, > + ret = wait_event_timeout(tty_ldisc_idle, > atomic_read(&tty->ldisc->users) == 1, 5 * HZ); > if (ret< 0) > return ret; > @@ -750,6 +750,8 @@ static int tty_ldisc_reinit(struct tty_s > if (IS_ERR(ld)) > return -1; > > + WARN_ON_ONCE(tty_ldisc_wait_idle(tty)); Please hold on for backporting this into stable, the WARN_ON is reported to trigger in some cases. > + > tty_ldisc_close(tty, tty->ldisc); > tty_ldisc_put(tty->ldisc); > tty->ldisc = NULL; regards, -- js suse labs