From owner-freebsd-current Sat Mar 16 2: 9:50 2002 Delivered-To: freebsd-current@freebsd.org Received: from rina.r.dl.itc.u-tokyo.ac.jp (cvsup2.r.dl.itc.u-tokyo.ac.jp [133.11.199.247]) by hub.freebsd.org (Postfix) with ESMTP id AA85037B417; Sat, 16 Mar 2002 02:09:40 -0800 (PST) Received: from silver.carrots.uucp.r.dl.itc.u-tokyo.ac.jp (silver.carrots.uucp.r.dl.itc.u-tokyo.ac.jp [IPv6:3ffe:b80:5b0:3:280:c8ff:fe6b:6d73]) by rina.r.dl.itc.u-tokyo.ac.jp (8.12.2/3.7W-rina.r-Nankai-Koya) with ESMTP id g2GA97Sp056863 ; Sat, 16 Mar 2002 19:09:08 +0900 (JST) Received: from silver.carrots.uucp.r.dl.itc.u-tokyo.ac.jp (localhost [127.0.0.1]) by silver.carrots.uucp.r.dl.itc.u-tokyo.ac.jp (8.12.2/3.7W-carrots-Keikyu-Kurihama) with ESMTP id g2GA8rA5092735 ; Sat, 16 Mar 2002 19:09:06 +0900 (JST) Message-Id: <200203161009.g2GA8rA5092735@silver.carrots.uucp.r.dl.itc.u-tokyo.ac.jp> Date: Sat, 16 Mar 2002 19:08:53 +0900 From: Seigo Tanimura To: Alfred Perlstein Cc: "Brian F. Feldman" , jhb@FreeBSD.org, current@FreeBSD.org, Seigo Tanimura Subject: sx_upgrade() (was: Re: cvs commit: src/sys/kern kern_mtxpool.c src/sys/sys kernel.h src/sys/vm vm_fault.c vm_glue.c vm_map.c vm_map.h vm_pageout.c vm_zone.c) In-Reply-To: <20020315185320.GJ4857@elvis.mu.org> References: <20020315054113.GC4857@elvis.mu.org> <200203151121.g2FBLnj36094@green.bikeshed.org> <20020315185320.GJ4857@elvis.mu.org> User-Agent: Wanderlust/2.8.1 (Something) SEMI/1.14.3 (Ushinoya) FLIM/1.14.3 (=?ISO-8859-1?Q?Unebigory=F2mae?=) APEL/10.3 MULE XEmacs/21.1 (patch 14) (Cuyahoga Valley) (i386--freebsd) Organization: Digital Library Research Division, Information Techinology Centre, The University of Tokyo MIME-Version: 1.0 (generated by SEMI 1.14.3 - "Ushinoya") Content-Type: multipart/mixed; boundary="Multipart_Sat_Mar_16_19:08:53_2002-1" Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG --Multipart_Sat_Mar_16_19:08:53_2002-1 Content-Type: text/plain; charset=US-ASCII [Add jhb and move to -current] On Fri, 15 Mar 2002 10:53:20 -0800, Alfred Perlstein said: Alfred> * Brian F. Feldman [020315 03:22] wrote: >> Alfred Perlstein wrote: >> > >> > What is the problem? >> >> Damn good question. Are the tracebacks related? If not, what are you >> supposed to be telling me it's deadlocking on? I don't see the system being >> deadlocked. What is it actually supposed to be blocked on? >> >> > Well basically you changed: >> > >> > ! static __inline__ int >> > ! _vm_map_lock_upgrade(vm_map_t map, struct thread *td) { >> > ! int error; >> > ! >> > ! vm_map_printf("locking map LK_EXCLUPGRADE: %p\n", map); >> > ! error = lockmgr(&map->lock, LK_EXCLUPGRADE, NULL, td); >> > ! if (error == 0) >> > ! map->timestamp++; >> > ! return error; >> > } >> > >> > into: >> > >> > ! _vm_map_lock_upgrade(vm_map_t map, const char *file, int line) >> > { >> > ! vm_map_printf("locking map LK_EXCLUPGRADE: %p\n", map); >> > ! if (_sx_try_upgrade(&map->lock, file, line)) { >> > ! map->timestamp++; >> > ! return (0); >> > ! } >> > ! return (EWOULDBLOCK); >> > } >> >> It doesn't need LK_EXCLUPGRADE semantics, only LK_UPGRADE, if it's not >> blocking. It backs out completely and unlocks the shared reference and >> tries for an exclusive lock. Alfred> Sigh, you're making me do all the work here... :( Alfred> lockmgr(&map->lock, LK_EXCLUPGRADE, NULL, td); Alfred> means: Alfred> Turn my shared lock into an exclusive lock, Alfred> if it's shared then wait for all shared locks to drain, Alfred> however if someone else is requesting an exclusive lock, then fail. Alfred> _sx_try_upgrade(&map->lock, file, line) Alfred> means: Alfred> Give me an exclusive lock Alfred> if anyone else has a shared lock then fail immediately. Alfred> What happens in your case is that you get into a busy loop Alfred> because you never yeild the processor. Alfred> This happens in vm_map_lookup() because of this: Alfred> if (fault_type & VM_PROT_WRITE) { Alfred> /* Alfred> * Make a new object, and place it in the object Alfred> * chain. Note that no new references have appeared Alfred> * -- one just moved from the map to the new Alfred> * object. Alfred> */ Alfred> if (vm_map_lock_upgrade(map)) Alfred> goto RetryLookup; Alfred> So, you fail your sx_lock upgrade and cause the cpu to spin. Alfred> You basically need to add logic to the sxlock subsystem to do what Alfred> lockmgr does, actually wait for the others to go away or fail if Alfred> someone else wants an upgrade. Attached patch implements sx_upgrade() which should work as you said above. This compiles fine, but is not tested yet. sx_upgrader records the thread that wants to upgrade. If sx_upgrader is non-NULL, sx_sunlock() wakes up the upgrader rather than other exclusive lock waiters. --Multipart_Sat_Mar_16_19:08:53_2002-1 Content-Type: application/octet-stream; type=patch Content-Disposition: attachment; filename="sx_upgrade.diff" Content-Transfer-Encoding: 7bit ==== //depot/user/tanimura/inodezone_uma/kern/kern_sx.c#1 - /home/carrots/tanimura/silver4/p4-freefall/inodezone_uma/kern/kern_sx.c ==== --- /tmp/tmp.89710.0 Sat Mar 16 18:50:40 2002 +++ /home/carrots/tanimura/silver4/p4-freefall/inodezone_uma/kern/kern_sx.c Sat Mar 16 17:56:00 2002 @@ -72,6 +72,7 @@ cv_init(&sx->sx_excl_cv, description); sx->sx_excl_wcnt = 0; sx->sx_xholder = NULL; + sx->sx_upgrader = NULL; LOCK_LOG_INIT(lock, 0); @@ -215,7 +216,15 @@ * there are exclusive lock waiters. */ if (sx->sx_excl_wcnt > 0) { - if (sx->sx_cnt == 0) + /* + * The upgrader beats any other exclusive lockers. + * Note that the upgrader holds the last shared lock. + */ + if (sx->sx_upgrader != NULL && sx->sx_cnt == 1) { + KASSERT(td->td_flags & TDF_CVWAITQ != 0, + ("_sx_sunlock: the upgrader is not waiting for sx_excl_cv")); + cv_waitq_remove(sx->sx_upgrader); + } else if (sx->sx_cnt == 0) cv_signal(&sx->sx_excl_cv); } else if (sx->sx_shrd_wcnt > 0) cv_broadcast(&sx->sx_shrd_cv); @@ -250,6 +259,45 @@ LOCK_LOG_LOCK("XUNLOCK", &sx->sx_object, 0, 0, file, line); mtx_unlock(sx->sx_lock); +} + +int +_sx_upgrade(struct sx *sx, const char *file, int line) +{ + + _sx_assert(sx, SX_SLOCKED, file, line); + mtx_lock(sx->sx_lock); + + /* + * If another thread is waiting for lock upgrade, + * the curtherad must unlock this sx. + */ + if (sx->sx_upgrader != NULL) { + mtx_unlock(sx->sx_lock); + return (0); + } + sx->sx_upgrader = curthread; + + /* Loop in case we lose the race for lock acquisition. */ + while (sx->sx_cnt != 1) { + sx->sx_excl_wcnt++; + cv_wait(&sx->sx_excl_cv, sx->sx_lock); + sx->sx_excl_wcnt--; + } + + /* We must be the sole thread slocking this sx. */ + MPASS(sx->sx_cnt == 1); + + /* Acquire an exclusive lock. */ + sx->sx_cnt = -1; + sx->sx_xholder = curthread; + sx->sx_upgrader = NULL; + + LOCK_LOG_LOCK("XUPGRADE", &sx->sx_object, 0, 1, file, line); + WITNESS_UPGRADE(&sx->sx_object, LOP_EXCLUSIVE, file, line); + + mtx_unlock(sx->sx_lock); + return (1); } int ==== //depot/user/tanimura/inodezone_uma/kern/subr_witness.c#1 - /home/carrots/tanimura/silver4/p4-freefall/inodezone_uma/kern/subr_witness.c ==== --- /tmp/tmp.89710.1 Sat Mar 16 18:50:41 2002 +++ /home/carrots/tanimura/silver4/p4-freefall/inodezone_uma/kern/subr_witness.c Sat Mar 16 18:02:38 2002 @@ -717,9 +717,9 @@ if ((lock->lo_flags & LO_UPGRADABLE) == 0) panic("upgrade of non-upgradable lock (%s) %s @ %s:%d", class->lc_name, lock->lo_name, file, line); - if ((flags & LOP_TRYLOCK) == 0) - panic("non-try upgrade of lock (%s) %s @ %s:%d", class->lc_name, - lock->lo_name, file, line); + if ((flags & LOP_TRYLOCK) == 0) { + /* TODO: check for lock order reversal. */ + } if ((lock->lo_class->lc_flags & LC_SLEEPLOCK) == 0) panic("upgrade of non-sleep lock (%s) %s @ %s:%d", class->lc_name, lock->lo_name, file, line); ==== //depot/user/tanimura/inodezone_uma/sys/sx.h#1 - /home/carrots/tanimura/silver4/p4-freefall/inodezone_uma/sys/sx.h ==== --- /tmp/tmp.89710.2 Sat Mar 16 18:50:41 2002 +++ /home/carrots/tanimura/silver4/p4-freefall/inodezone_uma/sys/sx.h Sat Mar 16 17:33:09 2002 @@ -44,6 +44,7 @@ struct cv sx_excl_cv; /* xlock waiters. */ int sx_excl_wcnt; /* Number of xlock waiters. */ struct thread *sx_xholder; /* Thread presently holding xlock. */ + struct thread *sx_upgrader; /* Thread presently waiting for lock upgrade. */ }; #ifdef _KERNEL @@ -55,6 +56,7 @@ int _sx_try_xlock(struct sx *sx, const char *file, int line); void _sx_sunlock(struct sx *sx, const char *file, int line); void _sx_xunlock(struct sx *sx, const char *file, int line); +int _sx_upgrade(struct sx *sx, const char *file, int line); int _sx_try_upgrade(struct sx *sx, const char *file, int line); void _sx_downgrade(struct sx *sx, const char *file, int line); #ifdef INVARIANT_SUPPORT @@ -67,6 +69,7 @@ #define sx_try_xlock(sx) _sx_try_xlock((sx), LOCK_FILE, LOCK_LINE) #define sx_sunlock(sx) _sx_sunlock((sx), LOCK_FILE, LOCK_LINE) #define sx_xunlock(sx) _sx_xunlock((sx), LOCK_FILE, LOCK_LINE) +#define sx_upgrade(sx) _sx_upgrade((sx), LOCK_FILE, LOCK_LINE) #define sx_try_upgrade(sx) _sx_try_upgrade((sx), LOCK_FILE, LOCK_LINE) #define sx_downgrade(sx) _sx_downgrade((sx), LOCK_FILE, LOCK_LINE) --Multipart_Sat_Mar_16_19:08:53_2002-1 Content-Type: text/plain; charset=US-ASCII -- Seigo Tanimura --Multipart_Sat_Mar_16_19:08:53_2002-1-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message