proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest2 patches 19 repliesIn review
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/2 | tty-ldisc: make refcount be atomic_t 'users' countdrivers/ include/ | +9 | 0 replies | |
| 2/2 | tty-ldisc: turn ldisc user count into a proper refcountdrivers/ | +46 | 2 replies |
Reviews trailers per patch
1 of 2 patches has a Reviewed-by or Acked-by.
| Patch | Alan Cox | Sergey Senozhatsky |
|---|---|---|
| Series | T-b | |
| 1/2 | ||
| 2/2 | A-b |
Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.
Cover letter [PATCH 0/2] proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)
Quoted 8 lines · Linus Torvalds
Quoted 5 lines
You could just finish the ldisc refcounting. The last set of patches you had off me split
tty->ldiscfrom struct tty ready to do exactly that and I don't think there is anything left that stops it happening now (It was just not ready in time)I considered it, and it didn't look horrible (the thing really is pretty self-contained in
tty_ldisc_try()andtty_ldisc_deref()).
Here we are.
It wasn't a straight conversion, because the old code really didn't think of the refcounts as lifetimes, but it wasn't too bad either. And doing the proper refcounting makes all the stupid "wait for idle" go away, so it actually removes code:
drivers/char/tty_ldisc.c | 145 ++++++++++++++------------------------------ include/linux/tty_ldisc.h | 2 +- 2 files changed, 47 insertions(+), 100 deletions(-)
and generally simplifies the logic.
That said, looking through the code as I did this, I consciously avoided doing some other cleanups that really should be done some day. The code is chock-full of crazy stuff, where we just do
o_ldisc = tty->ldisc;
with dubious locking. None of that is _new_ though, and most of it is in the "replace one ldisc with another" code. And for all I know, maybe it's all fine, it's just very much not _obviously_ correct.
As far as I can tell, this short series should not introduce any new problems, but hey, maybe it leaks ldisc references like mad because I made some silly mistake. It's a _fairly_ straightforward cleanup, but it's a big cnnceptual change to go from a model with a "wait until idle and then free" to a model of "count users and free on last use", and I could easily have screwed up something.
"It works for me"(tm), including a shutdown/reboot cycle.
Sergey, mind testing? You seem to be very good at consistently triggering odd things in the tty layer that few other people seem to ever hit.
Greg - I've signed off on these, but I wasn't planning on committing them to my master branch. So perhaps you could do these as the new tty maintainer, assuming we get an ack from Alan and testing by Sergey.
Linus