Date: Sun, 2 Nov 2014 18:59:16 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Attilio Rao <attilio@freebsd.org> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org> Subject: Re: svn commit: r273966 - in head: share/man/man9 sys/kern sys/sys Message-ID: <20141102165916.GY53947@kib.kiev.ua> In-Reply-To: <CAJ-FndAeh_i9F98Tq-ZXgX%2Badq4mOgoadSoYJ6hmvYpBtmkvow@mail.gmail.com> References: <201411021310.sA2DAWmD003298@svn.freebsd.org> <CAJ-FndB5%2BhhHprTiV1ODJoE9RNZLtOe2eQDi3MEpgPKYbM3LAw@mail.gmail.com> <20141102163728.GX53947@kib.kiev.ua> <CAJ-FndAeh_i9F98Tq-ZXgX%2Badq4mOgoadSoYJ6hmvYpBtmkvow@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141102165916.GY53947>