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>