svn commit: r273966 - in head: share/man/man9 sys/kern sys/sys

[Konstantin Belousov]

On Sun, Nov 02, 2014 at 05:42:55PM +0100, Attilio Rao wrote:
> On Sun, Nov 2, 2014 at 5:37 PM, Konstantin Belousov <kostikbel at gmail.com> wrote:
> > On Sun, Nov 02, 2014 at 04:54:46PM +0100, Attilio Rao wrote:
> >> On Sun, Nov 2, 2014 at 2:10 PM, Konstantin Belousov <kib at freebsd.org> wrote:
> >> > Author: kib
> >> > Date: Sun Nov  2 13:10:31 2014
> >> > New Revision: 273966
> >> > URL: https://svnweb.freebsd.org/changeset/base/273966
> >> >
> >> > Log:
> >> >   Fix two issues with lockmgr(9) LK_CAN_SHARE() test, which determines
> >> >   whether the shared request for already shared-locked lock could be
> >> >   granted.  Both problems result in the exclusive locker starvation.
> >> >
> >> >   The concurrent exclusive request is indicated by either
> >> >   LK_EXCLUSIVE_WAITERS or LK_EXCLUSIVE_SPINNERS flags.  The reverse
> >> >   condition, i.e. no exclusive waiters, must check that both flags are
> >> >   cleared.
> >> >
> >> >   Add a flag LK_NODDLKTREAT for shared lock request to indicate that
> >> >   current thread guarantees that it does not own the lock in shared
> >> >   mode.  This turns back the exclusive lock starvation avoidance code;
> >> >   see man page update for detailed description.
> >> >
> >> >   Use LK_NODDLKTREAT when doing lookup(9).
> >>
> >> The right way to implement this (selectively disabling writer
> >> starvation avoidance) must be on a lock-basis.
> >> So you need a new flag to pass to lockinit(). This is to support it "globaly".
> > Any vnode (except some very special) does participate in lookup.
> > So the proposed new flag has to be passed to every lockmgr init call.
> > Disabling exclusive starvation support for all vnode lock calls is
> > also wrong.
> >
> >> Then, you can pass it on a lockmgr() instance basis (like FreeBSD also
> >> passes, for example, LK_NOWITNESS).
> >> You can use it during lookup() calls. Maybe you will need to propagate
> >> it via the vn_lock() interface.
> > Well, if you indeed looked at the patch, you would note that this is
> > exactly what is done.  The flag is passed only to vn_lock() calls
> > which are coming from lookup (VOP_LOOKUP as well).
> >
> > The flag use is safe since deadlock prevention by td_lk_slocks only
> > matters when the same lock was previously taken by the same thread
> > which does recursive shared request [*].  td_lk_slocks is the poor and
> > very indiscriminative way to express this, but might be unavoidable
> > since shared owners are not recorded.
> 
> If you have a better way to fix this into a "rich and discriminative"
> way I'm all ears.
It is easy and cheap to record the set of the owned lockmgr locks for
current thread. I do not believe that we have a situation where more
than 3 locks are held in one time. To give it some slack, we can record
8 locks shared-owned and do the check for recursion in LK_CAN_SHARE().
If the thread-locks table overflows, we could either panic() or fall
back to indiscriminative deadlock avoidance (i.e. relying on the sole
td_lk_slocks value).

> 
> > * I believe there are also situation (like vnode locking in page fault)
> > where it matters in way similar to TDP_DEADLKTREAT without flag being
> > set, but lookup() cannot expose them.
> >
> > Now, thread calling namei() cannot own shared lock on the vnode it
> > tries to lock, since such code will cause deadlock for many reason. As
> > such, td_lk_slocks counter is not relevant for the vn_lock() calls from
> > lookup. More, when vn_lock() call is performed, the counter is always >=
> > 1, due to the lookup already owning the lock for the different (parent)
> > vnode. The last fact means that the exclusive starvation happen.
> >
> >>
> >> The patch will be bigger but much cleaner and more correct than what
> >> is in head now.
> > I do not understand what would change except adding useless flag
> > to init.
> 
> I didn't look into details of the original problem, but this would
> cleanup semantics.
Oh.

> As it is now it is just confusing for people.
> I think that we should invest some time in cleaning up (or keep clean)
> interface that will be used into drivers and thirdy part code.
> Right now the new flag is just a shot in the dark.
> 
> As always, feel free to skip my suggestions.
> 
> Attilio
> 
> 
> -- 
> Peace can only be achieved by understanding - A. Einstein