Re: [PATCH v5 26/44] tty: Add read-recursive, writer-prioritized rw semaphore

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Mar 18, 2013 at 09:01:19PM -0400, Peter Hurley wrote:
> On Mon, 2013-03-18 at 16:58 -0700, Greg Kroah-Hartman wrote:
> > On Mon, Mar 11, 2013 at 04:44:46PM -0400, Peter Hurley wrote:
> > > 2) TIOCSETD ioctl (change line discipline) expects to return an
> > >    error if the line discipline cannot be exclusively locked within
> > >    5 secs. Lock wait timeouts are not supported by rwsem.
> > 
> > Don't we have some other lock that can timeout?
> 
> Not that behaves like a r/w semaphore.

Can't we just add it?  Or is that too much work?

> > > 3) A tty hangup is expected to halt and scrap pending i/o, so
> > >    exclusive locking must be prioritized without precluding
> > >    existing reference holders from obtaining recursive read locks.
> > >    Writer priority is not supported by rwsem.
> > 
> > But how bad is it really if we have to wait a bit for that write lock to
> > get through all of the existing readers?  Either way, we are supposed to
> > be dropping i/o, so it shouldn't be a big deal, right?
> 
> The rwsem behavior is in the process of changing. Write lock stealing
> has already been added and refinements there will likely allow some
> readers in front of writers.
> 
> With slow serial i/o, I'd rather have hangups occur promptly than let a
> bunch more i/o through.

So all we are now lacking, with the changes to rwsem, is the timeout
problem?

> > > Add ld_semaphore which implements these requirements in a
> > > semantically and operationally similar way to rw_semaphore.
> > 
> > I _really_ don't want to add a new lock to the kernel, especially one
> > that is only used by one "driver".  You are going to have to convince
> > the current lock authors that this really is needed, before I can take
> > it, sorry.
> 
> That's fine. I can understand the reluctance to take on a new lock
> [although you might be interested to read my analysis of rwsem here
> https://lkml.org/lkml/2013/3/11/533 which outlines an existing flaw].
> 
> That said, part of the reason why the current ldisc implementation is
> broken is the lack of appropriate locks. As I recently explained
> (actually in this patchset's thread),
> 
>         a lack of existing options has spawned a DIY approach without
>         higher-order locks that is rarely correct, but which goes largely
>         unnoticed exactly because it's not a new lock. A brief review of the
>         hangs, races, and deadlocks fixed by this patchset should be convincing
>         enough of that fact. In my opinion, this is the overriding concern.
>         
>         The two main problems with a one-size-fits-all lock policy is that,
>         1) lock experts can't realistically foresee the consequences of policy
>         changes without already being experts in the subsystems in which that
>         lock is used. Even domain experts may miss potential consequences, and
>         2) domain experts typically wouldn't even consider writing a new lock.
>         So they make do with atomic bit states, spinlocks, reference counts,
>         mutexes, and waitqueues, making a mostly-functional, higher-order lock. 

I read that, however rolling your own lock is almost never the solution.

> From whom would you like me to get an ack for this?

The people who wrote the rwsem code?

> > What is wrong with the existing ldisc code that the creation of this
> > lock is needed?  Is our current code that broken?
> 
> Yes. Even just the acquistion of the ldisc reference is wrong [the
> analysis is in the patch 21 changelog].

Yes, very nice work, I'm not saying that this isn't a messed up area at
all, it's just that such deep flaws that require a new type of a lock
don't usually come up all that often.

> If you'd like, I can send you 6 or so short user test programs that
> hang, crash, or deadlock inside 60 seconds on mainline and next, but not
> with this patchset.

That would be interesting to have, please send them.

And I hope that they only lock up when run as root, but I'm afraid to
ask that question...

thanks,

greg k-h