On 03/12/2015 10:20 AM, George Spelvin wrote: > From peter@xxxxxxxxxxxxxxxxxx Thu Mar 12 11:30:55 2015 > On 03/10/2015 06:49 PM, George Spelvin wrote: >> By the way, there's also a performance bug in tty_insert_flip_char. >> If (tb->flags & TTYB_NORMAL), then tb->used is allowed to go up >> to 2*tb->size, as the __tty_buffer_request_room code makes clear. >> But tty_insert_flip_char() doesn't know that and will take the slow path >> for the second half of every buffer. > >> In practice, this doesn't happen because drivers that use >> tty_insert_flip_char() won't have compressed buffers (ie., TTYB_NORMAL). >> That's because tty_insert_flip_string_flags() will always get the >> uncompressed buffer flavor. > > Ah, thank you! I had missed the fact that the "flag" argument to > __tty_buffer_request_room is 0 = uncompressed, TTYB_NORMAL = 1 = compressed, > so the call chain > tty_insert_flip_char > -> tty_insert_flip_string_flags > -> tty_buffer_request_room > -> __tty_buffer_request_room(port, size, 0) > > always allocates an uncompressed buffer. > > I had it mixed up with the "flags" parameter which is 0 = TTY_NORMAL = > compressed is permissible. I should probably replace that '0' argument with a symbolic constant. >> That's to avoid buffer type hysteresis on a dirty line. > > I was thinking of tweaking it so that a compressed buffer can be converted > in-place to an uncompressed buffer as long as it's less than half full, > i.e. b->used < b->size. > > If it's done properly (barrier between filling the flag buffer and > clearing the TTYB_NORMAL flag), then receive_buf can remain lockless. I'm not seeing how that can be done without adding a complementary write barrier between reading the commit index and reading the flags on the consumer side, which would unnecessarily impact pty performance. If this space optimization is desirable, I'd rather see a simple state machine on the producer-side only that limited buffer type hysteresis; eg., starting out with tty_bufhead being TTYB_NORMAL capable until the first type switch is required because input flag was parity/break/overrun, and then requiring some fixed # of buffers input before resetting it. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
