Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 4 Mar 2018 21:38:31 +0000 (UTC)
From:      Mateusz Guzik <mjg@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r330414 - head/sys/kern
Message-ID:  <201803042138.w24LcVAa088120@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: mjg
Date: Sun Mar  4 21:38:30 2018
New Revision: 330414
URL: https://svnweb.freebsd.org/changeset/base/330414

Log:
  locks: fix a corner case in r327399
  
  If there were exactly rowner_retries/asx_retries (by default: 10) transitions
  between read and write state and the waiters still did not get the lock, the
  next owner -> reader transition would result in the code correctly falling
  back to turnstile/sleepq where it would incorrectly think it was waiting
  for a writer and decide to leave turnstile/sleepq to loop back. From this
  point it would take ts/sq trips until the lock gets released.
  
  The bug sometimes manifested itself in stalls during -j 128 package builds.
  
  Refactor the code to fix the bug, while here remove some of the gratituous
  differences between rw and sx locks.

Modified:
  head/sys/kern/kern_rwlock.c
  head/sys/kern/kern_sx.c

Modified: head/sys/kern/kern_rwlock.c
==============================================================================
--- head/sys/kern/kern_rwlock.c	Sun Mar  4 21:15:31 2018	(r330413)
+++ head/sys/kern/kern_rwlock.c	Sun Mar  4 21:38:30 2018	(r330414)
@@ -875,7 +875,7 @@ __rw_wlock_hard(volatile uintptr_t *c, uintptr_t v LOC
 #ifdef ADAPTIVE_RWLOCKS
 	int spintries = 0;
 	int i, n;
-	int sleep_reason = 0;
+	enum { READERS, WRITER } sleep_reason;
 #endif
 	uintptr_t x;
 #ifdef LOCK_PROFILING
@@ -956,9 +956,11 @@ __rw_wlock_hard(volatile uintptr_t *c, uintptr_t v LOC
 		 * running on another CPU, spin until the owner stops
 		 * running or the state of the lock changes.
 		 */
-		sleep_reason = 1;
-		owner = lv_rw_wowner(v);
-		if (!(v & RW_LOCK_READ) && TD_IS_RUNNING(owner)) {
+		if (!(v & RW_LOCK_READ)) {
+			sleep_reason = WRITER;
+			owner = lv_rw_wowner(v);
+			if (!TD_IS_RUNNING(owner))
+				goto ts;
 			if (LOCK_LOG_TEST(&rw->lock_object, 0))
 				CTR3(KTR_LOCK, "%s: spinning on %p held by %p",
 				    __func__, rw, owner);
@@ -973,9 +975,10 @@ __rw_wlock_hard(volatile uintptr_t *c, uintptr_t v LOC
 			KTR_STATE0(KTR_SCHED, "thread", sched_tdname(curthread),
 			    "running");
 			continue;
-		}
-		if ((v & RW_LOCK_READ) && RW_READERS(v) &&
-		    spintries < rowner_retries) {
+		} else if (RW_READERS(v) > 0) {
+			sleep_reason = READERS;
+			if (spintries == rowner_retries)
+				goto ts;
 			if (!(v & RW_LOCK_WRITE_SPINNER)) {
 				if (!atomic_fcmpset_ptr(&rw->rw_lock, &v,
 				    v | RW_LOCK_WRITE_SPINNER)) {
@@ -993,15 +996,15 @@ __rw_wlock_hard(volatile uintptr_t *c, uintptr_t v LOC
 				if ((v & RW_LOCK_WRITE_SPINNER) == 0)
 					break;
 			}
-			KTR_STATE0(KTR_SCHED, "thread", sched_tdname(curthread),
-			    "running");
 #ifdef KDTRACE_HOOKS
-			lda.spin_cnt += rowner_loops - i;
+			lda.spin_cnt += i;
 #endif
+			KTR_STATE0(KTR_SCHED, "thread", sched_tdname(curthread),
+			    "running");
 			if (i < rowner_loops)
 				continue;
-			sleep_reason = 2;
 		}
+ts:
 #endif
 		ts = turnstile_trywait(&rw->lock_object);
 		v = RW_READ_VALUE(rw);
@@ -1021,7 +1024,7 @@ retry_ts:
 				turnstile_cancel(ts);
 				continue;
 			}
-		} else if (RW_READERS(v) > 0 && sleep_reason == 1) {
+		} else if (RW_READERS(v) > 0 && sleep_reason == WRITER) {
 			turnstile_cancel(ts);
 			continue;
 		}

Modified: head/sys/kern/kern_sx.c
==============================================================================
--- head/sys/kern/kern_sx.c	Sun Mar  4 21:15:31 2018	(r330413)
+++ head/sys/kern/kern_sx.c	Sun Mar  4 21:38:30 2018	(r330414)
@@ -533,8 +533,8 @@ _sx_xlock_hard(struct sx *sx, uintptr_t x, int opts LO
 #ifdef ADAPTIVE_SX
 	volatile struct thread *owner;
 	u_int i, n, spintries = 0;
+	enum { READERS, WRITER } sleep_reason;
 	bool adaptive;
-	int sleep_reason = 0;
 #endif
 #ifdef LOCK_PROFILING
 	uint64_t waittime = 0;
@@ -628,37 +628,33 @@ _sx_xlock_hard(struct sx *sx, uintptr_t x, int opts LO
 		 * running or the state of the lock changes.
 		 */
 		if ((x & SX_LOCK_SHARED) == 0) {
+			sleep_reason = WRITER;
 			owner = lv_sx_owner(x);
-			if (TD_IS_RUNNING(owner)) {
-				if (LOCK_LOG_TEST(&sx->lock_object, 0))
-					CTR3(KTR_LOCK,
-				    "%s: spinning on %p held by %p",
-					    __func__, sx, owner);
-				KTR_STATE1(KTR_SCHED, "thread",
-				    sched_tdname(curthread), "spinning",
-				    "lockname:\"%s\"",
-				    sx->lock_object.lo_name);
-				do {
-					lock_delay(&lda);
-					x = SX_READ_VALUE(sx);
-					owner = lv_sx_owner(x);
-				} while (owner != NULL &&
-					    TD_IS_RUNNING(owner));
-				KTR_STATE0(KTR_SCHED, "thread",
-				    sched_tdname(curthread), "running");
-				continue;
-			}
-			sleep_reason = 1;
-		} else if (SX_SHARERS(x) && spintries < asx_retries) {
-			KTR_STATE1(KTR_SCHED, "thread",
-			    sched_tdname(curthread), "spinning",
-			    "lockname:\"%s\"", sx->lock_object.lo_name);
+			if (!TD_IS_RUNNING(owner))
+				goto sleepq;
+			if (LOCK_LOG_TEST(&sx->lock_object, 0))
+				CTR3(KTR_LOCK, "%s: spinning on %p held by %p",
+				    __func__, sx, owner);
+			KTR_STATE1(KTR_SCHED, "thread", sched_tdname(curthread),
+			    "spinning", "lockname:\"%s\"",
+			    sx->lock_object.lo_name);
+			do {
+				lock_delay(&lda);
+				x = SX_READ_VALUE(sx);
+				owner = lv_sx_owner(x);
+			} while (owner != NULL && TD_IS_RUNNING(owner));
+			KTR_STATE0(KTR_SCHED, "thread", sched_tdname(curthread),
+			    "running");
+			continue;
+		} else if (SX_SHARERS(x) > 0) {
+			sleep_reason = READERS;
+			if (spintries == asx_retries)
+				goto sleepq;
 			spintries++;
+			KTR_STATE1(KTR_SCHED, "thread", sched_tdname(curthread),
+			    "spinning", "lockname:\"%s\"",
+			    sx->lock_object.lo_name);
 			for (i = 0; i < asx_loops; i += n) {
-				if (LOCK_LOG_TEST(&sx->lock_object, 0))
-					CTR4(KTR_LOCK,
-			    "%s: shared spinning on %p with %u and %u",
-					    __func__, sx, spintries, i);
 				n = SX_SHARERS(x);
 				lock_delay_spin(n);
 				x = SX_READ_VALUE(sx);
@@ -669,11 +665,10 @@ _sx_xlock_hard(struct sx *sx, uintptr_t x, int opts LO
 #ifdef KDTRACE_HOOKS
 			lda.spin_cnt += i;
 #endif
-			KTR_STATE0(KTR_SCHED, "thread",
-			    sched_tdname(curthread), "running");
+			KTR_STATE0(KTR_SCHED, "thread", sched_tdname(curthread),
+			    "running");
 			if (i < asx_loops)
 				continue;
-			sleep_reason = 2;
 		}
 sleepq:
 #endif
@@ -705,7 +700,7 @@ retry_sleepq:
 					sleepq_release(&sx->lock_object);
 					continue;
 				}
-			} else if (SX_SHARERS(x) > 0 && sleep_reason == 1) {
+			} else if (SX_SHARERS(x) > 0 && sleep_reason == WRITER) {
 				sleepq_release(&sx->lock_object);
 				continue;
 			}



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