[lkml]   [2013]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [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.

Peter Hurley

 \ /
  Last update: 2013-12-10 03:21    [W:0.090 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site