Date:	Mon, 02 Dec 2013 22:22:00 -0500
From:	Peter Hurley <peter@...leysoftware.com>
To:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH tty-next 0/4] tty: Fix ^C echo

On 12/02/2013 07:01 PM, One Thousand Gnomes wrote:
>> I cc'd you because of your recent involvement in other
>> tty patches/bug fixes and because it's your FIXME comment.
>> Feel free to ignore and/or let me know you would prefer not to
>> be bothered.
>
> It does seem horribly convoluted and likely to dig bigger long term holes
> than the one its filling. The tty layer has suffered far too much from
> "dodging one bullet by being clever and then getting shot at twice more"

Alan,

Thanks for the feedback.

I think any solution will be seriously constrained by the larger design
choice; the simplicity and performance of direct writes into a shared
buffer has trade-offs.

I chose what I felt was the simplest route; the code itself is
straightforward. Maybe I could move the locking documentation into the
changelog instead?

Setting aside the parallel flush interface complications (which in some
form is necessary even with a shared lock), there are a limited number
of solutions to concurrent pty buffer flushes:
1. Leave it broken.
2. As submitted.
3. Drop the buf->lock before flushing. This was the other solution I
    seriously considered, because a parallel flush interface would not
    be necessary then.
    The problem with this solution is that input processing would need
    to be aborted and restarted in case the current buffer data had been
    flushed in parallel while the buf->lock was unowned; this requires
    bubbling up return values from n_tty_receive_break() and
    n_tty_receive_signal_char(), plus the actual processed char count
    would need to propagate as well.
4. Do the flush in flush_to_ldisc(). This is variation on 3, because
    current input processing would still need to abort. This solution
    suffers in that the echo may still be deferred, since at the time
    of processing the signal, the other pty write buffer may still be
    full.
5. Probably a few others I haven't thought of.

 > Bigger question (and one I'm not going to try and untangle at quarter to
 > midnight). Is there any reason that the buffer locking has to be per tty
 > not a shared lock in some cases.

I'm not sure what kind of impact a partial half-duplex pty design would
really have.

> My thinking is that we never sit hogging the buffer lock for long periods
> (even though someone has now made it a mutex which seems an odd
> performance choice)

Uncontended mutex lock performance is on-par with uncontended spinlock;
both use a single bus-locked r/m/w instruction.

The buffer lock is mostly uncontended because it only excludes the
consumer-side; ie., only excludes flush_to_ldisc() from tty_buffer_flush().
The driver-side doesn't use buf->lock for access.

The use of a mutex for buf->lock allows the n_tty_receive_buf() path to
claim a read lock on termios changes with a r/w semaphore, termios_rwsem.

> and it is the deepest lock in the subsystem we take
>
> So:
>
> if the tty_buffer contained a mutex and a pointer to that mutex then for
> the pty pairs you could set them to point to the same mutex but default
> to separate mutexes.
>
> At that point you swap all the locks on the mutex to simply lock through
> the pointer, you don't need the nested hack and there are no special case
> paths or uglies in the general code.

To claim the buf->lock in any form from within the receive_buf() path
will require some construct that informs the pty driver's flush_buffer()
method that the buf->lock has already been claimed; otherwise,
double-claim deadlock.

These types of nested lock problems are common when different layers use
the same interface (the fb subsystem's use of the vt driver is another
example).

> The only special is that pty init
> paths set the points to both point at the same mutex and no kittens die.

I like the ptr-to-a-shadow-lock idiom, thanks for sharing.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists