Skip site navigation (1)Skip section navigation (2) 
Date:      Tue, 3 Jan 2017 21:36:15 +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: r311172 - in head/sys: kern sys
Message-ID:  <201701032136.v03LaFCe010314@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: mjg
Date: Tue Jan  3 21:36:15 2017
New Revision: 311172
URL: https://svnweb.freebsd.org/changeset/base/311172

Log:
  mtx: reduce lock accesses
  
  Instead of spuriously re-reading the lock value, read it once.
  
  This change also has a side effect of fixing a performance bug:
  on failed _mtx_obtain_lock, it was possible that re-read would find
  the lock is unowned, but in this case the primitive would make a trip
  through turnstile code.
  
  This is diff reduction to a variant which uses atomic_fcmpset.
  
  Discussed with:	jhb (previous version)
  Tested by:	pho (previous version)

Modified:
  head/sys/kern/kern_mutex.c
  head/sys/sys/mutex.h

Modified: head/sys/kern/kern_mutex.c
==============================================================================
--- head/sys/kern/kern_mutex.c	Tue Jan  3 21:11:30 2017	(r311171)
+++ head/sys/kern/kern_mutex.c	Tue Jan  3 21:36:15 2017	(r311172)
@@ -94,8 +94,6 @@ PMC_SOFT_DEFINE( , , lock, failed);
 
 #define	mtx_destroyed(m) ((m)->mtx_lock == MTX_DESTROYED)
 
-#define	mtx_owner(m)	((struct thread *)((m)->mtx_lock & ~MTX_FLAGMASK))
-
 static void	assert_mtx(const struct lock_object *lock, int what);
 #ifdef DDB
 static void	db_show_mtx(const struct lock_object *lock);
@@ -491,8 +489,9 @@ __mtx_lock_sleep(volatile uintptr_t *c, 
 	lock_delay_arg_init(&lda, NULL);
 #endif
 	m = mtxlock2mtx(c);
+	v = MTX_READ_VALUE(m);
 
-	if (mtx_owned(m)) {
+	if (__predict_false(lv_mtx_owner(v) == (struct thread *)tid)) {
 		KASSERT((m->lock_object.lo_flags & LO_RECURSABLE) != 0 ||
 		    (opts & MTX_RECURSE) != 0,
 	    ("_mtx_lock_sleep: recursed on non-recursive mutex %s @ %s:%d\n",
@@ -520,8 +519,12 @@ __mtx_lock_sleep(volatile uintptr_t *c, 
 #endif
 
 	for (;;) {
-		if (m->mtx_lock == MTX_UNOWNED && _mtx_obtain_lock(m, tid))
-			break;
+		if (v == MTX_UNOWNED) {
+			if (_mtx_obtain_lock(m, tid))
+				break;
+			v = MTX_READ_VALUE(m);
+			continue;
+		}
 #ifdef KDTRACE_HOOKS
 		lda.spin_cnt++;
 #endif
@@ -530,31 +533,30 @@ __mtx_lock_sleep(volatile uintptr_t *c, 
 		 * If the owner is running on another CPU, spin until the
 		 * owner stops running or the state of the lock changes.
 		 */
-		v = m->mtx_lock;
-		if (v != MTX_UNOWNED) {
-			owner = (struct thread *)(v & ~MTX_FLAGMASK);
-			if (TD_IS_RUNNING(owner)) {
-				if (LOCK_LOG_TEST(&m->lock_object, 0))
-					CTR3(KTR_LOCK,
-					    "%s: spinning on %p held by %p",
-					    __func__, m, owner);
-				KTR_STATE1(KTR_SCHED, "thread",
-				    sched_tdname((struct thread *)tid),
-				    "spinning", "lockname:\"%s\"",
-				    m->lock_object.lo_name);
-				while (mtx_owner(m) == owner &&
-				    TD_IS_RUNNING(owner))
-					lock_delay(&lda);
-				KTR_STATE0(KTR_SCHED, "thread",
-				    sched_tdname((struct thread *)tid),
-				    "running");
-				continue;
-			}
+		owner = lv_mtx_owner(v);
+		if (TD_IS_RUNNING(owner)) {
+			if (LOCK_LOG_TEST(&m->lock_object, 0))
+				CTR3(KTR_LOCK,
+				    "%s: spinning on %p held by %p",
+				    __func__, m, owner);
+			KTR_STATE1(KTR_SCHED, "thread",
+			    sched_tdname((struct thread *)tid),
+			    "spinning", "lockname:\"%s\"",
+			    m->lock_object.lo_name);
+			do {
+				lock_delay(&lda);
+				v = m->mtx_lock;
+				owner = lv_mtx_owner(v);
+			} while (v != MTX_UNOWNED && TD_IS_RUNNING(owner));
+			KTR_STATE0(KTR_SCHED, "thread",
+			    sched_tdname((struct thread *)tid),
+			    "running");
+			continue;
 		}
 #endif
 
 		ts = turnstile_trywait(&m->lock_object);
-		v = m->mtx_lock;
+		v = MTX_READ_VALUE(m);
 
 		/*
 		 * Check if the lock has been released while spinning for
@@ -573,7 +575,7 @@ __mtx_lock_sleep(volatile uintptr_t *c, 
 		 * chain lock.  If so, drop the turnstile lock and try
 		 * again.
 		 */
-		owner = (struct thread *)(v & ~MTX_FLAGMASK);
+		owner = lv_mtx_owner(v);
 		if (TD_IS_RUNNING(owner)) {
 			turnstile_cancel(ts);
 			continue;
@@ -588,6 +590,7 @@ __mtx_lock_sleep(volatile uintptr_t *c, 
 		if ((v & MTX_CONTESTED) == 0 &&
 		    !atomic_cmpset_ptr(&m->mtx_lock, v, v | MTX_CONTESTED)) {
 			turnstile_cancel(ts);
+			v = MTX_READ_VALUE(m);
 			continue;
 		}
 
@@ -618,6 +621,7 @@ __mtx_lock_sleep(volatile uintptr_t *c, 
 		sleep_time += lockstat_nsecs(&m->lock_object);
 		sleep_cnt++;
 #endif
+		v = MTX_READ_VALUE(m);
 	}
 #ifdef KDTRACE_HOOKS
 	all_time += lockstat_nsecs(&m->lock_object);
@@ -675,6 +679,7 @@ _mtx_lock_spin_cookie(volatile uintptr_t
 {
 	struct mtx *m;
 	struct lock_delay_arg lda;
+	uintptr_t v;
 #ifdef LOCK_PROFILING
 	int contested = 0;
 	uint64_t waittime = 0;
@@ -701,24 +706,30 @@ _mtx_lock_spin_cookie(volatile uintptr_t
 #ifdef KDTRACE_HOOKS
 	spin_time -= lockstat_nsecs(&m->lock_object);
 #endif
+	v = MTX_READ_VALUE(m);
 	for (;;) {
-		if (m->mtx_lock == MTX_UNOWNED && _mtx_obtain_lock(m, tid))
-			break;
+		if (v == MTX_UNOWNED) {
+			if (_mtx_obtain_lock(m, tid))
+				break;
+			v = MTX_READ_VALUE(m);
+			continue;
+		}
 		/* Give interrupts a chance while we spin. */
 		spinlock_exit();
-		while (m->mtx_lock != MTX_UNOWNED) {
+		do {
 			if (lda.spin_cnt < 10000000) {
 				lock_delay(&lda);
-				continue;
+			} else {
+				lda.spin_cnt++;
+				if (lda.spin_cnt < 60000000 || kdb_active ||
+				    panicstr != NULL)
+					DELAY(1);
+				else
+					_mtx_lock_spin_failed(m);
+				cpu_spinwait();
 			}
-			lda.spin_cnt++;
-			if (lda.spin_cnt < 60000000 || kdb_active ||
-			    panicstr != NULL)
-				DELAY(1);
-			else
-				_mtx_lock_spin_failed(m);
-			cpu_spinwait();
-		}
+			v = MTX_READ_VALUE(m);
+		} while (v != MTX_UNOWNED);
 		spinlock_enter();
 	}
 #ifdef KDTRACE_HOOKS

Modified: head/sys/sys/mutex.h
==============================================================================
--- head/sys/sys/mutex.h	Tue Jan  3 21:11:30 2017	(r311171)
+++ head/sys/sys/mutex.h	Tue Jan  3 21:36:15 2017	(r311172)
@@ -420,9 +420,15 @@ extern struct mtx_pool *mtxpool_sleep;
 	_sleep((chan), &(mtx)->lock_object, (pri), (wmesg),		\
 	    tick_sbt * (timo), 0, C_HARDCLOCK)
 
+#define	MTX_READ_VALUE(m)	((m)->mtx_lock)
+
 #define	mtx_initialized(m)	lock_initialized(&(m)->lock_object)
 
-#define mtx_owned(m)	(((m)->mtx_lock & ~MTX_FLAGMASK) == (uintptr_t)curthread)
+#define lv_mtx_owner(v)	((struct thread *)((v) & ~MTX_FLAGMASK))
+
+#define mtx_owner(m)	lv_mtx_owner(MTX_READ_VALUE(m))
+
+#define mtx_owned(m)	(mtx_owner(m) == curthread)
 
 #define mtx_recursed(m)	((m)->mtx_recurse != 0)

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