Date: Fri, 13 Jan 2017 14:51:04 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mark Johnston <markj@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r311952 - head/sys/ddb Message-ID: <20170113131948.P1465@besplex.bde.org> In-Reply-To: <201701120022.v0C0MaHq053076@repo.freebsd.org> References: <201701120022.v0C0MaHq053076@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 12 Jan 2017, Mark Johnston wrote:
> Log:
> Enable the use of ^C and ^S/^Q in DDB.
>
> This lets one interrupt DDB's output, which is useful if paging is
> disabled and the output device is slow.
This is quite broken. It removes the code that was necessary for avoiding
loss of input.
> Modified: head/sys/ddb/db_input.c
> ==============================================================================
> --- head/sys/ddb/db_input.c Thu Jan 12 00:09:31 2017 (r311951)
> +++ head/sys/ddb/db_input.c Thu Jan 12 00:22:36 2017 (r311952)
> @@ -63,7 +63,6 @@ static int db_lhist_nlines;
> #define BLANK ' '
> #define BACKUP '\b'
>
> -static int cnmaygetc(void);
> static void db_delete(int n, int bwd);
> static int db_inputchar(int c);
> static void db_putnchars(int c, int count);
> @@ -291,12 +290,6 @@ db_inputchar(c)
> return (0);
> }
>
> -static int
> -cnmaygetc()
> -{
> - return (-1);
> -}
> -
BSD never had a usable console API (function) for checking for input.
cncheckc() is the correct name for such a function. The actual
cncheckc() returns any input that it finds. This is not the main
problem here. The input will have to be read here anyway to determine
what it is. The problems are that there is no console API at all for
ungetting characters in the input stream, and this function doesn't
even use a stub cnungetc(), but drops the input on the floor. It does
document this behaviour in a comment, but doesn't say that it is wrong.
> int
> db_readline(lstart, lsize)
> char * lstart;
> @@ -350,7 +343,7 @@ db_check_interrupt(void)
> {
> int c;
>
> - c = cnmaygetc();
> + c = cncheckc();
> switch (c) {
> case -1: /* no character */
> return;
The stub function always returns -1, so db_check_interrupt() is turned into
a no-op.
db_check_interrupt() now eats the first byte of any input on every call.
> @@ -361,7 +354,7 @@ db_check_interrupt(void)
>
> case CTRL('s'):
> do {
> - c = cnmaygetc();
> + c = cncheckc();
> if (c == CTRL('c'))
> db_error((char *)0);
> } while (c != CTRL('q'));
This is now a bad implementation of xon/xoff. It doesn't support ixany,
but busy-waits for ^Q or ^C using cncheckc(). It should at least
busy-wait using cngetc(), since that might be do a smarter busy wait.
cngetc() is actually only slightly smarter -- it uses busy-waiting too,
but with cpu_spinwait() in the loop. This cpu_spinwait() makes little
difference since cncheckc() tends to be a heavyweight function. Only
cpu_spinwait()s in the innermost loops of cncheckc(), if any, are likely
to have much effect.
Multiple consoles complicate this a bit.
db_check_interrupt() is called after every db_putc() of a newline. So
the breakage is mainly for type-ahead while writing many lines.
Control processing belongs in the lowest level of console drivers, and
at least the xon/xoff part is very easy to do. This makes it work for
contexts like boot messages -- this can only be controlled now by booting
with -p, which gives something like an automatic ^S after every line,
but the control characters for this mode are unusual and there is no
way to get back to -p mode after turning it off or not starting with it.
-p mode also crashes if you don't turn it off before the (syscons)
keyboard is attached, since attachment temporarily breaks the input needed
to control the mode. ddb control should worry about this even more.
Multiple consoles complicate this more than a bit.
For console driver development/debugging, I needed xon-xoff type control
and more (discard) at the driver level, especially when i/o to one of
the multiple consoles is unreachable to avoid deadlock, or too active.
It was easy to write primitive xon/xoff but not to avoid deadlocks in
this. Hackish code looks like this:
diff -u2 syscons.c~ syscons.c
X --- syscons.c~ 2016-09-01 19:20:38.263745000 +0000
X +++ syscons.c 2016-10-14 07:12:18.947461000 +0000
X ...
X +static void
X +ec_wait(void)
ec_wait() is called to give time to read transient output about certain
errors (mainly in near-deadlock conditions). The error message is written
directly to the frame buffer and will be overwritten if the deadlock
condition clears enough to allow normal output.
X +{
X + int c;
X + int timeout;
X +
X + if (ec_kill_ec_wait)
X + return;
X + c = inb(0xe060);
0xe060 is an otherwise-unused serial port which I can write control characters
too.
X + if (ec_wait_verbose)
X + ec_puts("\r\nec: hit any key to abort wait of 10 seconds...");
X + for (timeout = 0; timeout < ec_wait_delay; timeout++) {
X + // if (cncheckc() != -1)
X + // break;
Early versions used simply cncheckc(). This checks all consoles, so can
deadlock too easily, and it doesn't keep the input streams separate enough
for easy control of the per-console output streams.
X + if (inb(0xe060) != c)
X + break;
X + if (sc_consptr != NULL)
X + DELAY(1000);
X + cn_progress++;
X + }
X + if (ec_wait_verbose)
X + ec_puts("done\r\n");
X +}
X ...
X @@ -1896,5 +2025,8 @@
X sizeof(sc_cnputc_log))
X continue;
X + if (inb(0xe060) == 'O' - '@')
X + continue;
X sc_puts(scp, buf, 1, 1);
X + cn_progress++;
X }
X
This uses ^O (default termios VDISCARD) to enter a mode in which all
output is dropped (input is sticky in the serial port, so this mode is
sticky and is left by typing any other character which should be chosen
to be non-control to avoid controlling other operations). Another
driver uses ^P instead of ^Q.
xon/xoff is normally per-char in termios, but buffering tends to make
it per-buffer. In ddb, it is per-line, and this is better than per-char
for kernel uses. I have used and implemented a weird ^S mode for scrolling
system messages, where ^S works like the "any" character in -p mode --
^S stops output, and subsequent ^S's restart output and then stop it after
1 more line. This is convenient for scrolling by holding down the ^S key --
much easier than typing ^S/^Q. This was on a 1980's 8-bit system, and is
even more needed now -- you cannot type ^S/^Q fast enough to prevent a
few thousand lines from scrolling past between the characters, except
with slow output devices or pessimized console drivers.
Simple xon/xoff at the cnputc() level would work well enough ones the
deadlock problems are fixed. It allows the sysadmin to stop all printf()s
on the system for arbitrarily long just by typing ^S (^S might also be
in line noise). This looks as bad as deadlock to threads trying to do
the printf()s, though it is only actual deadlock for certain i/o in
console drivers. The cn_progress++ lines in the above are to tell my
version of printf() to not time out and blow open its locks when
console output is making progress or just stopped.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170113131948.P1465>