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

[Konstantin Belousov]

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.

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