Paths

Table of Contentst

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

Authored by kevans on Apr 17 2020, 4:07 AM.

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 30570

Event Timeline

kevans created this revision.Apr 17 2020, 4:07 AM

Herald added subscribers: melifaro, donner, jhibbits, imp. · View Herald TranscriptApr 17 2020, 4:07 AM

Harbormaster completed remote builds in B30552: Diff 70672.Apr 17 2020, 4:07 AM

mikael added a subscriber: mikael.Apr 17 2020, 12:10 PM

markj added a subscriber: markj.Apr 17 2020, 1:53 PM

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.

kevans added inline comments.Apr 17 2020, 2:06 PM

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)

markj added inline comments.Apr 17 2020, 2:19 PM

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.

kevans added inline comments.Apr 17 2020, 2:20 PM

sys/dev/altera/jtag_uart/altera_jtag_uart_tty.c
560	Got it, thanks! Will fix

emaste added a subscriber: emaste.Apr 17 2020, 4:23 PM

kevans mentioned this in rS360051: tty: convert tty_lock_assert to tty_assert_locked to hide lock type.Apr 17 2020, 6:35 PM

kevans marked 3 inline comments as done.Apr 17 2020, 7:04 PM

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...

Harbormaster completed remote builds in B30570: Diff 70715.Apr 17 2020, 8:33 PM

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().

Harbormaster completed remote builds in B30578: Diff 70725.Apr 18 2020, 4:50 AM

kevans mentioned this in rS360258: MFC r360051: tty: convert tty_lock_assert to tty_assert_locked.Apr 24 2020, 1:31 PM

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.

Resurrect the tty lock split patch from the dead; better describe the sleeps we're wanting to enable -- the general idea is that anything coming in via the tty layer to a driver should sleep as needed so that we can meet the usual termios promise that when, e.g., tcsetattr() returns, the change has actually taken effect. This is often important so that userland applications trying to configure a tty can get relative timing correct if there's some minimum requirement between operations they're doing.

In practice, this only affects USB serial drivers, where we otherwise do everything completely asynchronously. If the device goes away while we're waiting for a command to complete, the process queue should get drained and we'll promptly return + drop the lock since the device lock is separate from the now-sleepable tty lock. The usb serial layer has some error handling issues that we should probably looked at, but this isn't a new problem.

This patch has recently been independently tested to significantly improve reliability of connecting to some ESP32 gear running micropython.

Herald added subscribers: riscv, olce, glebius, ehem_freebsd_m5p.com. · View Herald TranscriptDec 4 2024, 3:16 PM

This looks good from 10k feet, but I've not thought it all through.
I suspect this review is getting close to too long to easily review and some subsetting (like the tty_assert_locked -> ttydisc_assert_locked) might be helpful.
I'll look in more detail later, so far just one question, but it seems kinda basic to my understanding of the rest.

sys/kern/kern_cons.c
596–599	I thought that ttydisc is a sx lock, so this shouldn't be needed.... Or is that a 'future thing after these patches'?

In D24459#1092380, @imp wrote:

This looks good from 10k feet, but I've not thought it all through.
I suspect this review is getting close to too long to easily review and some subsetting (like the tty_assert_locked -> ttydisc_assert_locked) might be helpful.
I'll look in more detail later, so far just one question, but it seems kinda basic to my understanding of the rest.

I have it locally split up into two commits:
1.) Split the tty lock up into tty, ttydisc locks
2.) Convert the former to an sx lock

I suspect I split it after this review was created, I can go ahead and break up the review to match if the idea of sleeping on the tty lock at least when satisfying requests from userland isn't outrigth rejected.

sys/kern/kern_cons.c
596–599	The ttydisc lock is separated out, so it's effectively the discipline + driver lock while the historical tty lock is just for the tty itself.

imp added inline comments.Dec 4 2024, 3:59 PM

sys/kern/kern_cons.c
596–599	I thought wrong, it's still mtx.

So userspace might end up waiting uninterruptibly for hw to finish something?

In D24459#1092714, @kib wrote:

So userspace might end up waiting uninterruptibly for hw to finish something?

Yes, though we could presumably implement usb_proc_mwait_sig() for an escape hatch. It would be an improvement even before this change, because open/close will both drop the tty lock and wait potentially uninterruptibly for the hardware.

In D24459#1092724, @kevans wrote:

In D24459#1092714, @kib wrote:

So userspace might end up waiting uninterruptibly for hw to finish something?

Yes, though we could presumably implement usb_proc_mwait_sig() for an escape hatch. It would be an improvement even before this change, because open/close will both drop the tty lock and wait potentially uninterruptibly for the hardware.

Well, this might help with the driver thread itself, but what about user threads that happen to wait for the same sx? Your _tty_lock() is uninterruptible. This would cause lock cascade, and user needs to know which thread to kill to unblock the dependency chain.

In D24459#1092725, @kib wrote:

In D24459#1092724, @kevans wrote:

In D24459#1092714, @kib wrote:

So userspace might end up waiting uninterruptibly for hw to finish something?

Yes, though we could presumably implement usb_proc_mwait_sig() for an escape hatch. It would be an improvement even before this change, because open/close will both drop the tty lock and wait potentially uninterruptibly for the hardware.

Well, this might help with the driver thread itself, but what about user threads that happen to wait for the same sx? Your _tty_lock() is uninterruptible. This would cause lock cascade, and user needs to know which thread to kill to unblock the dependency chain.

Ah, good point. Munching on it a bit more, I guess the sensible option here is to slap a maybe-conservative timeout on the mwait so that the worst case scenario isn't any more broken than it would be today.

Actually, it looks like our driver callbacks typically do timeouts when they actually submit to the controller: https://cgit.freebsd.org/src/tree/sys/dev/usb/serial/uftdi.c#n1688 -- so most of these should be reasonably bounded. Perhaps still good to be defensive, since I'm not about to audit every path out of usb_serial.c

In D24459#1092727, @kevans wrote:

Actually, it looks like our driver callbacks typically do timeouts when they actually submit to the controller: https://cgit.freebsd.org/src/tree/sys/dev/usb/serial/uftdi.c#n1688 -- so most of these should be reasonably bounded. Perhaps still good to be defensive, since I'm not about to audit every path out of usb_serial.c

I believe this is palliative. If you want to hold the lock around driver calls, then _tty_lock should do sx_sig_lock() and propagate the locking error back.

In D24459#1092986, @kib wrote:

In D24459#1092727, @kevans wrote:

Actually, it looks like our driver callbacks typically do timeouts when they actually submit to the controller: https://cgit.freebsd.org/src/tree/sys/dev/usb/serial/uftdi.c#n1688 -- so most of these should be reasonably bounded. Perhaps still good to be defensive, since I'm not about to audit every path out of usb_serial.c

I believe this is palliative. If you want to hold the lock around driver calls, then _tty_lock should do sx_sig_lock() and propagate the locking error back.

The ramifications of doing so are rather annoying. :-) I'll do some soul searching and decide if I actually need it to be sleepable to hit the minimum guarantees that I need, and come back with a more palatable review (re: size). If I don't end up with something sleepable, I might still do some profiling to see if there's any interesting property in splitting out the ttydisc lock to make it worth it on its own.

This has been split into:

https://reviews.freebsd.org/D47950 usb: serial: propagate errors from ucom_queue_command()
https://reviews.freebsd.org/D47951 usb: serial: allow the open/close sleep to be interruptible
https://reviews.freebsd.org/D47952 usb: serial: make more commands execute synchronously

I don't think I actually care about sleeping at all.