From owner-freebsd-bugs@FreeBSD.ORG Thu Oct 25 09:00:02 2012 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 2EA91775 for ; Thu, 25 Oct 2012 09:00:02 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.FreeBSD.org [8.8.178.135]) by mx1.freebsd.org (Postfix) with ESMTP id 15B058FC0C for ; Thu, 25 Oct 2012 09:00:02 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.5/8.14.5) with ESMTP id q9P901jd042808 for ; Thu, 25 Oct 2012 09:00:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.5/8.14.5/Submit) id q9P901AG042806; Thu, 25 Oct 2012 09:00:01 GMT (envelope-from gnats) Date: Thu, 25 Oct 2012 09:00:01 GMT Message-Id: <201210250900.q9P901AG042806@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org Cc: From: Jeremy Chadwick Subject: Re: kern/173010: Backgrounded processes remain in ttyin state / SIGCONT does not remove sleeping processes from sleep queue X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: Jeremy Chadwick List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Oct 2012 09:00:02 -0000 The following reply was made to PR kern/173010; it has been noted by GNATS. From: Jeremy Chadwick To: FreeBSD-gnats-submit@FreeBSD.org, freebsd-bugs@FreeBSD.org Cc: Subject: Re: kern/173010: Backgrounded processes remain in ttyin state / SIGCONT does not remove sleeping processes from sleep queue Date: Thu, 25 Oct 2012 01:51:45 -0700 --ReaqsoxgOBHFXBhH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Mail thread on -stable at this point, including a patch from ed@ From: Ed Schouten To: Konstantin Belousov Date: Thu, 25 Oct 2012 09:45:11 +0200 Cc: Jeremy Chadwick , freebsd-stable@freebsd.org, jhb@freebsd.org Subject: Re: pty/tty or signal strangeness, or grep/bsdgrep bug? Hi all, 2012/10/23 Ed Schouten : > Will try to come up with a decent patch tomorrow evening. Ahem; the day after tomorrow. Jeremy, could you please try the following patch? http://80386.nl/pub/tty-bg-read.txt I decomposed the TTY read routine into four separate functions to improve clarity. While this was initially true, I think it's a pity the four functions are constantly becoming a bit more complex. The same issue is also present on the output path, but I have no idea how realistic/hard it is to fix this issue. Also, it might not really be an issue in practice. If you do a large write and become a non-foreground process group, you might be able to circumvent TOSTOP while the write() is in transit. Fixing this might be tedious, because we currently enforce that writes to a TTY are serialized. Blocking inside the write() might then cause a deadlock. But in my opinion, I would prefer the serialization over the enforcing of TOSTOP. Thanks again for reporting the issue! -- Ed Schouten From: Jeremy Chadwick To: Ed Schouten Date: Thu, 25 Oct 2012 01:46:03 -0700 Cc: Konstantin Belousov , freebsd-stable@freebsd.org, jhb@freebsd.org Subject: Re: pty/tty or signal strangeness, or grep/bsdgrep bug? On Thu, Oct 25, 2012 at 09:45:11AM +0200, Ed Schouten wrote: > 2012/10/23 Ed Schouten : > > Will try to come up with a decent patch tomorrow evening. > > Ahem; the day after tomorrow. Jeremy, could you please try the following patch? > > http://80386.nl/pub/tty-bg-read.txt > > I decomposed the TTY read routine into four separate functions to > improve clarity. While this was initially true, I think it's a pity > the four functions are constantly becoming a bit more complex. > > The same issue is also present on the output path, but I have no idea > how realistic/hard it is to fix this issue. Also, it might not really > be an issue in practice. If you do a large write and become a > non-foreground process group, you might be able to circumvent TOSTOP > while the write() is in transit. > > Fixing this might be tedious, because we currently enforce that writes > to a TTY are serialized. Blocking inside the write() might then cause > a deadlock. But in my opinion, I would prefer the serialization over > the enforcing of TOSTOP. After the patch, testing with grep and cat, and checking cv/state for the latter case: (01:30:39 jdc@icarus) ~ $ csh % grep -r "-2011" . & [1] 1964 % jobs [1] + Suspended (tty input) grep -r -2011 . % fg grep -r -2011 . ^C % jobs % grep -r "-2011" . ^Z Suspended % bg [1] grep -r -2011 . & [1] + Suspended (tty input) grep -r -2011 . % fg grep -r -2011 . ^C % cat & [1] 2042 % jobs [1] + Suspended (tty input) cat % ps -auxwU jdc | grep cat jdc 2042 0.0 0.0 9908 1496 1 T 1:34AM 0:00.00 cat jdc 2044 0.0 0.0 16268 1864 1 S+ 1:34AM 0:00.00 grep cat % top -b -U jdc | grep cat 2042 jdc 1 20 0 9908K 1496K STOP 0 0:00 0.00% cat 2047 jdc 1 20 0 16268K 1864K piperd 0 0:00 0.00% grep cat % fg cat ^C % exit I do not get dropped characters or witness any other anomalies. I tested behaviour with /bin/sh, as well as bash. All seems good. > Thanks again for reporting the issue! No, thank *you* and others for looking + fixing it! :-) I assume a commit to HEAD + MFC in 2 weeks is in order? I'll update the PR with this part of our mail thread. -- | Jeremy Chadwick jdc@koitsu.org | | UNIX Systems Administrator http://jdc.koitsu.org/ | | Mountain View, CA, US | | Making life hard for others since 1977. PGP 4BD6C0CB | --ReaqsoxgOBHFXBhH Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="tty-bg-read.txt" Index: sys/kern/tty.c =================================================================== --- sys/kern/tty.c (Revision 241962) +++ sys/kern/tty.c (Arbeitskopie) @@ -361,7 +361,7 @@ return (p->p_session == tp->t_session && p->p_flag & P_CONTROLT); } -static int +int tty_wait_background(struct tty *tp, struct thread *td, int sig) { struct proc *p = td->td_proc; @@ -433,13 +433,6 @@ error = ttydev_enter(tp); if (error) goto done; - - error = tty_wait_background(tp, curthread, SIGTTIN); - if (error) { - tty_unlock(tp); - goto done; - } - error = ttydisc_read(tp, uio, ioflag); tty_unlock(tp); Index: sys/kern/tty_ttydisc.c =================================================================== --- sys/kern/tty_ttydisc.c (Revision 241962) +++ sys/kern/tty_ttydisc.c (Arbeitskopie) @@ -126,6 +126,10 @@ breakc[n] = '\0'; do { + error = tty_wait_background(tp, curthread, SIGTTIN); + if (error) + return (error); + /* * Quite a tricky case: unlike the old TTY * implementation, this implementation copies data back @@ -192,6 +196,10 @@ */ for (;;) { + error = tty_wait_background(tp, curthread, SIGTTIN); + if (error) + return (error); + error = ttyinq_read_uio(&tp->t_inq, tp, uio, uio->uio_resid, 0); if (error) @@ -229,6 +237,10 @@ timevaladd(&end, &now); for (;;) { + error = tty_wait_background(tp, curthread, SIGTTIN); + if (error) + return (error); + error = ttyinq_read_uio(&tp->t_inq, tp, uio, uio->uio_resid, 0); if (error) @@ -278,6 +290,10 @@ */ for (;;) { + error = tty_wait_background(tp, curthread, SIGTTIN); + if (error) + return (error); + error = ttyinq_read_uio(&tp->t_inq, tp, uio, uio->uio_resid, 0); if (error) Index: sys/sys/tty.h =================================================================== --- sys/sys/tty.h (Revision 241962) +++ sys/sys/tty.h (Arbeitskopie) @@ -180,6 +180,7 @@ void tty_signal_pgrp(struct tty *tp, int signal); /* Waking up readers/writers. */ int tty_wait(struct tty *tp, struct cv *cv); +int tty_wait_background(struct tty *tp, struct thread *td, int sig); int tty_timedwait(struct tty *tp, struct cv *cv, int timo); void tty_wakeup(struct tty *tp, int flags); --ReaqsoxgOBHFXBhH--