From: Linus Torvalds <torvalds@linux-foundation.org>
To: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>,
Sergey Senozhatsky <sergey.senozhatsky@mail.by>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Greg KH <greg@kroah.com>
Subject: [PATCH 0/2] proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c)
Date: Mon, 3 Aug 2009 10:55:34 -0700 (PDT) [thread overview]
Message-ID: <alpine.LFD.2.01.0908031047010.3208@localhost.localdomain> (raw)
In-Reply-To: <alpine.LFD.2.01.0908021654010.3352@localhost.localdomain>
On Sun, 2 Aug 2009, Linus Torvalds wrote:
>
> > You could just finish the ldisc refcounting. The last set of patches you
> > had off me split tty->ldisc from 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() and tty_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
next prev parent reply other threads:[~2009-08-03 17:55 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-08-02 12:01 WARNING at: drivers/char/tty_ldisc.c Sergey Senozhatsky
2009-08-02 16:05 ` Greg KH
2009-08-02 17:01 ` Sergey Senozhatsky
2009-08-02 17:07 ` Sergey Senozhatsky
2009-08-02 17:16 ` Linus Torvalds
2009-08-02 19:05 ` Sergey Senozhatsky
2009-08-02 20:20 ` Linus Torvalds
2009-08-02 21:17 ` OGAWA Hirofumi
2009-08-02 21:33 ` Linus Torvalds
2009-08-02 22:46 ` Sergey Senozhatsky
2009-08-02 22:48 ` Alan Cox
2009-08-03 0:40 ` Linus Torvalds
2009-08-03 1:44 ` Linus Torvalds
2009-08-03 9:37 ` Alan Cox
2009-08-03 16:26 ` OGAWA Hirofumi
2009-08-03 16:59 ` Alan Cox
2009-08-03 17:55 ` Linus Torvalds [this message]
2009-08-03 17:58 ` [PATCH 1/2] tty-ldisc: make refcount be atomic_t 'users' count Linus Torvalds
2009-08-03 18:11 ` [PATCH 2/2] tty-ldisc: turn ldisc user count into a proper refcount Linus Torvalds
2009-08-03 18:39 ` Alan Cox
2009-08-03 20:00 ` OGAWA Hirofumi
2009-08-03 18:18 ` [PATCH 0/2] proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c) Greg KH
2009-08-03 18:53 ` Linus Torvalds
2009-08-03 22:16 ` Sergey Senozhatsky
2009-08-03 22:25 ` Linus Torvalds
2009-08-03 22:58 ` [PATCH 3/2] tty-ldisc: be more careful in 'put_ldisc' locking Linus Torvalds
2009-08-03 23:00 ` [PATCH 4/2] tty-ldisc: make /proc/tty/ldiscs use ldisc_ops instead of ldiscs Linus Torvalds
2009-08-03 23:01 ` [PATCH 5/2] tty-ldisc: get rid of tty_ldisc_try_get() helper function Linus Torvalds
2009-08-04 0:30 ` proper tty-ldisc refcounting (was Re: WARNING at: drivers/char/tty_ldisc.c) Sergey Senozhatsky
2009-08-04 0:56 ` Linus Torvalds
2009-08-04 3:53 ` Greg KH
2009-08-04 4:08 ` Greg KH
2009-08-04 6:19 ` Linus Torvalds
2009-08-04 7:23 ` Greg KH
2009-08-04 9:12 ` Sergey Senozhatsky
2009-08-04 14:53 ` Greg KH
2009-08-04 15:40 ` Linus Torvalds
2009-08-04 16:00 ` Greg KH
2009-08-02 22:15 ` WARNING at: drivers/char/tty_ldisc.c Sergey Senozhatsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LFD.2.01.0908031047010.3208@localhost.localdomain \
--to=torvalds@linux-foundation.org \
--cc=alan@lxorguk.ukuu.org.uk \
--cc=greg@kroah.com \
--cc=hirofumi@mail.parknet.co.jp \
--cc=linux-kernel@vger.kernel.org \
--cc=sergey.senozhatsky@mail.by \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox