On Sun, Nov 2, 2014 at 5:37 PM, Konstantin Belousov <kostik...@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 <k...@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. > * 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. 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 _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
