Skip site navigation (1)Skip section navigation (2)
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>