Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 16 Mar 2002 19:08:53 +0900
From:      Seigo Tanimura <tanimura@r.dl.itc.u-tokyo.ac.jp>
To:        Alfred Perlstein <bright@mu.org>
Cc:        "Brian F. Feldman" <green@FreeBSD.org>, jhb@FreeBSD.org, current@FreeBSD.org, Seigo Tanimura <tanimura@r.dl.itc.u-tokyo.ac.jp>
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)
Message-ID:  <200203161009.g2GA8rA5092735@silver.carrots.uucp.r.dl.itc.u-tokyo.ac.jp>
In-Reply-To: <20020315185320.GJ4857@elvis.mu.org>
References:  <20020315054113.GC4857@elvis.mu.org> <200203151121.g2FBLnj36094@green.bikeshed.org> <20020315185320.GJ4857@elvis.mu.org>

next in thread | previous in thread | raw e-mail | index | archive | help
--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 <bright@mu.org> said:

Alfred> * Brian F. Feldman <green@FreeBSD.org> [020315 03:22] wrote:
>> Alfred Perlstein <bright@mu.org> 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 <tanimura@r.dl.itc.u-tokyo.ac.jp> <tanimura@FreeBSD.org>

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




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200203161009.g2GA8rA5092735>