Messages in this thread | | | Date | Mon, 09 Dec 2013 19:01:33 -0500 | From | Peter Hurley <> | Subject | Re: [PATCH 3.12] Broken terminal due to echo bufferring |
| |
On 12/09/2013 05:18 PM, Mikulas Patocka wrote: > > > On Mon, 9 Dec 2013, Peter Hurley wrote: > >> On 12/08/2013 09:55 PM, Mikulas Patocka wrote: >>> Hi >>> >>> I discovered that kernel 3.12 has broken terminal handling. >>> >>> I created this program to show the problem: >>> #include <stdio.h> >>> #include <unistd.h> >>> >>> int main(void) >>> { >>> int c; >>> while ((c = getchar()) != EOF) { >>> if (c == '\n') write(1, "prompt>", 7); >>> } >>> return 0; >>> } >>> >>> Each time the user presses enter, the program prints "prompt>". Normally, >>> when you press enter, you should see: >>> >>> prompt> >>> prompt> >>> prompt> >>> prompt>_ >>> >>> However, with kernel 3.12.4, you occasionally see >>> >>> prompt> >>> prompt> >>> prompt>prompt> >>> _ >>> >>> This bug happens randomly, it is timing-dependent. I am using single-core >>> 600MHz processor with preemptible kernel, the bug may or may not happen on >>> other computers. >>> >>> This bug is caused by Peter Hurley's echo buffering patches >>> (cbfd0340ae1993378fd47179db949e050e16e697). The patches change n_tty.c so >>> that it accumulates echoed characters and sends them out in a batch. >>> Something like this happens: >>> >>> * The user presses enter >>> * n_tty.c adds '\n' to the echo buffer using echo_char_raw >>> * n_tty.c adds '\n' to the input queue using put_tty_queue >>> * A process is switched >>> * Userspace reads '\n' from the terminal input queue >>> * Userspace writes the string "prompt>" to the terminal >>> * A process is switched back >>> * The echo buffer is flushed >>> * '\n' from the echo buffer is printed. >>> >>> >>> Echo bufferring is fundamentally wrong idea - you must make sure that you >>> flush the echo buffer BEFORE you add a character to input queue and BEFORE >>> you send any signal on behalf of that character. If you delay echo, you >>> are breaking behavior of various programs because the program output will >>> be interleaved with the echoed characters. >> >> There is nothing fundamentally broken with buffering echoes; it's just that >> there is a bug wrt when to process the echoes (ie, when to force the output). >> >> In the example you provided, the write() should cause the echoes to flush >> but doesn't because the commit marker hasn't been advanced. >> >> The commit marker wasn't advanced _yet_ because there is a race window between >> the reader being woken as a result of the newline and the flush_echoes() >> which happens with every received input. >> >> Either closing the race window or advancing the commit marker prior to >> write() output will fix the problem; right now, I'm looking at which is least >> painful. >> >> Regards, >> Peter Hurley > > I still think you should drop this. > > > The user types on the keyboard so slowly, that lock contention doesn't > matter. Specialized programs that use terminal to transmit bulk data don't > use cooked mode and echo. So I don't really see any use case where > something depends on performance of echoed characters. > > > Those patches just complicate the code for no benefit. > > > When you read a variable that may be asynchronously modified, you need > ACCESS_ONCE - for example you need this in process_echoes when accessing > the variables outside the lock: > if (ACCESS_ONCE(ldata->echo_commit) == ACCESS_ONCE(ldata->echo_tail))
Not necessarily. Stale values in an SMP environment may not be a problem; in this case, a possibly stale echo_tail simply means that the output_lock will be obtained unnecessarily (which is cheaper every-so-often than contending over the echo_tail cache line every write, especially on x86 where there is no problem).
Similarly, so many fences had to be passed to get to the echo_commit load from userspace that performing a load-acquire here and store-release in commit_echoes would be ridiculously superfluous.
> Anyway accessing variables that may change without locks or barriers is > generally bad idea and it is hard to verify it. Terminal layer is not > performance-sensitive part of the kernel, so it isn't justified to use > such dirty tricks. > > > Another problem: what about this in process_echoes and flush_echoes? > if (!L_ECHO(tty) || ldata->echo_commit == ldata->echo_tail) > return; > - so if the user turns off echo, echoes are not performed. But the buffer > is not flushed. So when the user turns on echo again, previously buffered > characters will be echoed. That is wrong.
The check for !L_ECHO pre-dates my patches; it might be wrong but userspace may have come to rely on this behavior. That said, feel free to submit a fix for that, if you think it's broken.
Regards, Peter Hurley
| |