Page MenuHomeFreeBSD

(WIP) tty: split the tty lock up, make the primary tty lock sleepable
Needs ReviewPublic

Authored by kevans on Apr 17 2020, 4:07 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 9 2024, 4:59 PM
Unknown Object (File)
Dec 23 2023, 12:58 AM
Unknown Object (File)
Dec 18 2023, 3:36 AM
Unknown Object (File)
Oct 28 2023, 11:12 PM
Unknown Object (File)
Jul 7 2023, 11:06 AM
Unknown Object (File)
May 20 2023, 11:40 PM
Unknown Object (File)
Jan 4 2023, 10:26 PM
Unknown Object (File)
Dec 29 2022, 7:30 AM
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

The primary motivation behind this patch is to allow TTY drivers to sleep while holding the TTY lock. This is particularly necessary in order to allow TTY drivers to operate asynchronously if they wish, e.g. ucom, without breaking one of the most basic assumptions of the TTY layer that operations should generally have complete or at least been submitted to hardware before we've returned to userland.

The changes in this patch are, for the most part, mechanical. TTY drivers are almost universally converted to using the new ttydisc lock, which is still a mutex. The TTY sx is still held across many calls into the driver, but for the most part they no longer need to be overtly aware of it since it's sleepable.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 30578

Event Timeline

markj added inline comments.
sys/dev/altera/jtag_uart/altera_jtag_uart_tty.c
269

This is a cosmetic note, but in other places we've changed the pattern of lock assertions to embed the assertion in the name rather than using a mtx(9)-specific parameter. In this case it'd become ttydisc_assert_locked(tp). That makes it easier to change the underlying implementation, like you had to do with tty_lock_assert().

560

callout_drain() is allowed to sleep, so you can't be holding a mtx here.

sys/dev/altera/jtag_uart/altera_jtag_uart_tty.c
560

I suspect the nomenclature for the tty lock here might be confusing, as that one's now an sx and the callout got converted to using the ttydisc lock, so that one must be held (IIRC)

sys/dev/altera/jtag_uart/altera_jtag_uart_tty.c
560

callout_stop() must be called with the associated lock held, but callout_drain() must not. If the callout thread is blocked on the ttydisc lock and you call callout_drain() with that lock held, you'll get a deadlock. I think it is sufficient to just defer acquiring the ttydisc lock until after the callout has been drained.

sys/dev/altera/jtag_uart/altera_jtag_uart_tty.c
560

Got it, thanks! Will fix

kevans added inline comments.
sys/dev/nmdm/nmdm.c
140

This is also fixed in my local draft... unsure why I did this.

sys/kern/tty.c
509

This is also fixed... this patch grew organically as I worked out the intricacies of tty bits, and I suspect at some point I was dropping the ttydisc lock coming into this loop then picking it back up here. It no longer makes any sense, though.

Rebase after rS360051; removed some whitespace-only hunks and fixed the callout handling on nmdm/altera.

tty_wait_background() should look a little better, no more attempting to recurse on the lock or dropping it...

Could you explain the new locking model, please ? Pref. in the code comment.

Take a stab at an explanation of locking above struct tty's definition; the short version is that most conventional usage outside of tty internals gets converted to using the ttydisc lock, but drivers must still grab the tty lock for tty_rel_gone().

Ok, can you explain please, which new sleeps under the tty lock you allow with this patch ?

In D24459#544931, @kib wrote:

Ok, can you explain please, which new sleeps under the tty lock you allow with this patch ?

I will re-evaluate the problem I'm trying to solve... right now the ucom situation is pretty bad, ucom_param (for instance) will not return an entire class of errors pertaining to what happens after we've enqueued a command for the usbproc to deal with, and callers (particularly TIOCSETA handler) will carry on as if the change was successful, which we cannot possibly know.

After talking to both kib and hps, I think there's no need to make the tty lock sleepable, and maybe there's not even a need to split the tty lock (though the split may have interesting properties for drivers like nmdm, where the two ends share the same ttydisc lock but have distinct tty locks).

I'm going to devise a mechanism for ucom callbacks to actually pass errors back to the initiator... its tasks will move from the ucom sc onto the stack of whatever's needing to schedule a callback, then they'll schedule the callback and wait (dropping the tty lock) to observe any errors. All of the callbacks I've looked at have timeouts on any usb requests they do, so this shouldn't block whatever's called into the ucom layer for too long.