Date: Thu, 25 Oct 2012 01:51:45 -0700 From: Jeremy Chadwick <jdc@koitsu.org> To: FreeBSD-gnats-submit@FreeBSD.org, freebsd-bugs@FreeBSD.org Subject: Re: kern/173010: Backgrounded processes remain in ttyin state / SIGCONT does not remove sleeping processes from sleep queue Message-ID: <20121025085145.GA2310@icarus.home.lan> In-Reply-To: <201210240350.q9O3o0ca066126@freefall.freebsd.org> References: <20121024034553.E5D8473A1A@icarus.home.lan> <201210240350.q9O3o0ca066126@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <ed@80386.nl> To: Konstantin Belousov <kostikbel@gmail.com> Date: Thu, 25 Oct 2012 09:45:11 +0200 Cc: Jeremy Chadwick <jdc@koitsu.org>, 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 <ed@80386.nl>: > 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 <ed@80386.nl> From: Jeremy Chadwick <jdc@koitsu.org> To: Ed Schouten <ed@80386.nl> Date: Thu, 25 Oct 2012 01:46:03 -0700 Cc: Konstantin Belousov <kostikbel@gmail.com>, 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 <ed@80386.nl>: > > 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121025085145.GA2310>