From 8f165b8c6a3e6ec3d65f6cf293a9913233dbd3bb Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Sun, 29 Aug 2010 23:25:42 -0700 Subject: [PATCH 1/1] kernel - remove spin_lock_rd() and spin_unlock_rd() * Remove shared spinlocks entirely. We only have exclusive spinlocks now. * Strip out the exclusive/shared contention handling code. * Slightly rework the initial exponential backoff and test. Do the atomic swap after the exponential backoff instead of before so we do not add a superfluous backoff after actually acquiring the spinlock. --- sys/kern/kern_ktr.c | 6 -- sys/kern/kern_spinlock.c | 134 +++------------------------------- sys/kern/lwkt_thread.c | 4 - sys/kern/usched_bsd4.c | 1 - sys/platform/pc32/isa/clock.c | 3 +- sys/platform/pc64/isa/clock.c | 3 +- sys/sys/globaldata.h | 2 +- sys/sys/spinlock2.h | 85 ++------------------- 8 files changed, 20 insertions(+), 218 deletions(-) diff --git a/sys/kern/kern_ktr.c b/sys/kern/kern_ktr.c index 7b65515caa..d0748f3a77 100644 --- a/sys/kern/kern_ktr.c +++ b/sys/kern/kern_ktr.c @@ -288,12 +288,6 @@ ktr_resync_callback(void *dummy __unused) spin_unlock_wr(&spin); } logtest_noargs(spin_end); - logtest_noargs(spin_beg); - for (count = ktr_testspincnt; count; --count) { - spin_lock_rd(&spin); - spin_unlock_rd(&spin); - } - logtest_noargs(spin_end); ktr_testspincnt = 0; } #endif diff --git a/sys/kern/kern_spinlock.c b/sys/kern/kern_spinlock.c index c44a850d7b..ece6e042d5 100644 --- a/sys/kern/kern_spinlock.c +++ b/sys/kern/kern_spinlock.c @@ -108,41 +108,16 @@ exponential_init(struct exponential_backoff *bo, struct spinlock *mtx) bo->backoff = BACKOFF_INITIAL; bo->nsec = 0; bo->mtx = mtx; + bo->base = 0; /* silence gcc */ } /* - * We were either contested due to another exclusive lock holder, - * or due to the presence of shared locks. We have to undo the mess - * we created by returning the shared locks. - * - * If there was another exclusive lock holder only the exclusive bit - * in value will be the only bit set. We don't have to do anything since - * restoration does not involve any work. - * - * Otherwise we successfully obtained the exclusive bit. Attempt to - * clear the shared bits. If we are able to clear the shared bits - * we win. Otherwise we lose and we have to restore the shared bits - * we couldn't clear (and also clear our exclusive bit). + * We contested due to another exclusive lock holder. We lose. */ int -spin_trylock_wr_contested(globaldata_t gd, struct spinlock *mtx, int value) +spin_trylock_wr_contested2(globaldata_t gd) { - int bit; - ++spinlocks_contested1; - if ((value & SPINLOCK_EXCLUSIVE) == 0) { - while (value) { - bit = bsfl(value); - if (globaldata_find(bit)->gd_spinlock_rd == mtx) { - atomic_swap_int(&mtx->lock, value); - --gd->gd_spinlocks_wr; - --gd->gd_curthread->td_critcount; - return (FALSE); - } - value &= ~(1 << bit); - } - return (TRUE); - } --gd->gd_spinlocks_wr; --gd->gd_curthread->td_critcount; return (FALSE); @@ -156,102 +131,27 @@ spin_trylock_wr_contested(globaldata_t gd, struct spinlock *mtx, int value) * would have been set and we can throw away value. */ void -spin_lock_wr_contested(struct spinlock *mtx, int value) +spin_lock_wr_contested2(struct spinlock *mtx) { struct exponential_backoff backoff; - globaldata_t gd = mycpu; - int bit; - int mask; + int value; /* * Wait until we can gain exclusive access vs another exclusive * holder. */ - exponential_init(&backoff, mtx); ++spinlocks_contested1; - logspin(beg, mtx, 'w'); + exponential_init(&backoff, mtx); - while (value & SPINLOCK_EXCLUSIVE) { - value = atomic_swap_int(&mtx->lock, SPINLOCK_EXCLUSIVE); - if (exponential_backoff(&backoff)) { - value &= ~SPINLOCK_EXCLUSIVE; + logspin(beg, mtx, 'w'); + do { + if (exponential_backoff(&backoff)) break; - } - } - - /* - * Kill the cached shared bit for our own cpu. This is the most - * common case and there's no sense wasting cpu on it. Since - * spinlocks aren't recursive, we can't own a shared ref on the - * spinlock while trying to get an exclusive one. - * - * If multiple bits are set do not stall on any single cpu. Check - * all cpus that have the cache bit set, then loop and check again, - * until we've cleaned all the bits. - */ - value &= ~gd->gd_cpumask; - - while ((mask = value) != 0) { - while (mask) { - bit = bsfl(value); - if (globaldata_find(bit)->gd_spinlock_rd != mtx) { - value &= ~(1 << bit); - } else if (exponential_backoff(&backoff)) { - value = 0; - break; - } - mask &= ~(1 << bit); - } - } + value = atomic_swap_int(&mtx->lock, SPINLOCK_EXCLUSIVE); + } while (value & SPINLOCK_EXCLUSIVE); logspin(end, mtx, 'w'); } -/* - * The cache bit wasn't set for our cpu. Loop until we can set the bit. - * As with the spin_lock_rd() inline we need a memory fence after setting - * gd_spinlock_rd to interlock against exclusive spinlocks waiting for - * that field to clear. - */ -void -spin_lock_rd_contested(struct spinlock *mtx) -{ - struct exponential_backoff backoff; - globaldata_t gd = mycpu; - int value = mtx->lock; - - /* - * Shortcut the op if we can just set the cache bit. This case - * occurs when the last lock was an exclusive lock. - */ - while ((value & SPINLOCK_EXCLUSIVE) == 0) { - if (atomic_cmpset_int(&mtx->lock, value, value|gd->gd_cpumask)) - return; - value = mtx->lock; - } - - exponential_init(&backoff, mtx); - ++spinlocks_contested1; - - logspin(beg, mtx, 'r'); - - while ((value & gd->gd_cpumask) == 0) { - if (value & SPINLOCK_EXCLUSIVE) { - gd->gd_spinlock_rd = NULL; - if (exponential_backoff(&backoff)) { - gd->gd_spinlock_rd = mtx; - break; - } - gd->gd_spinlock_rd = mtx; - cpu_mfence(); - } else { - if (atomic_cmpset_int(&mtx->lock, value, value|gd->gd_cpumask)) - break; - } - value = mtx->lock; - } - logspin(end, mtx, 'r'); -} - /* * Handle exponential backoff and indefinite waits. * @@ -377,18 +277,6 @@ sysctl_spin_lock_test(SYSCTL_HANDLER_ARGS) } } - /* - * Time best-case shared spinlocks - */ - if (value == 3) { - globaldata_t gd = mycpu; - - spin_init(&mtx); - for (i = spin_test_count; i > 0; --i) { - spin_lock_rd_quick(gd, &mtx); - spin_unlock_rd_quick(gd, &mtx); - } - } return (0); } diff --git a/sys/kern/lwkt_thread.c b/sys/kern/lwkt_thread.c index c8004a007c..41dd7a576c 100644 --- a/sys/kern/lwkt_thread.c +++ b/sys/kern/lwkt_thread.c @@ -563,9 +563,6 @@ lwkt_switch(void) * We had better not be holding any spin locks, but don't get into an * endless panic loop. */ - KASSERT(gd->gd_spinlock_rd == NULL || panicstr != NULL, - ("lwkt_switch: still holding a shared spinlock %p!", - gd->gd_spinlock_rd)); KASSERT(gd->gd_spinlocks_wr == 0 || panicstr != NULL, ("lwkt_switch: still holding %d exclusive spinlocks!", gd->gd_spinlocks_wr)); @@ -989,7 +986,6 @@ lwkt_preempt(thread_t ntd, int critcount) * We could try to acquire the tokens but this case is so rare there * is no need to support it. */ - KKASSERT(gd->gd_spinlock_rd == NULL); KKASSERT(gd->gd_spinlocks_wr == 0); if (TD_TOKS_HELD(ntd)) { diff --git a/sys/kern/usched_bsd4.c b/sys/kern/usched_bsd4.c index 18096f4d40..61847d534a 100644 --- a/sys/kern/usched_bsd4.c +++ b/sys/kern/usched_bsd4.c @@ -555,7 +555,6 @@ bsd4_schedulerclock(struct lwp *lp, sysclock_t period, sysclock_t cpstamp) * Spinlocks also hold a critical section so there should not be * any active. */ - KKASSERT(gd->gd_spinlock_rd == NULL); KKASSERT(gd->gd_spinlocks_wr == 0); bsd4_resetpriority(lp); diff --git a/sys/platform/pc32/isa/clock.c b/sys/platform/pc32/isa/clock.c index f404d9fbe6..c5acda6233 100644 --- a/sys/platform/pc32/isa/clock.c +++ b/sys/platform/pc32/isa/clock.c @@ -469,8 +469,7 @@ DRIVERSLEEP(int usec) { globaldata_t gd = mycpu; - if (gd->gd_intr_nesting_level || - gd->gd_spinlock_rd != NULL || gd->gd_spinlocks_wr) { + if (gd->gd_intr_nesting_level || gd->gd_spinlocks_wr) { DODELAY(usec, 0); } else { DODELAY(usec, 1); diff --git a/sys/platform/pc64/isa/clock.c b/sys/platform/pc64/isa/clock.c index 94ba26e966..ae7bbca3fa 100644 --- a/sys/platform/pc64/isa/clock.c +++ b/sys/platform/pc64/isa/clock.c @@ -472,8 +472,7 @@ DRIVERSLEEP(int usec) { globaldata_t gd = mycpu; - if (gd->gd_intr_nesting_level || - gd->gd_spinlock_rd || gd->gd_spinlocks_wr) { + if (gd->gd_intr_nesting_level || gd->gd_spinlocks_wr) { DODELAY(usec, 0); } else { DODELAY(usec, 1); diff --git a/sys/sys/globaldata.h b/sys/sys/globaldata.h index 2c5878d99d..d78d5d0169 100644 --- a/sys/sys/globaldata.h +++ b/sys/sys/globaldata.h @@ -159,7 +159,7 @@ struct globaldata { sysid_t gd_sysid_alloc; /* allocate unique sysid */ struct tslpque *gd_tsleep_hash; /* tsleep/wakeup support */ - struct spinlock *gd_spinlock_rd; /* Shared spinlock held */ + void *gd_unused08; int gd_spinlocks_wr; /* Exclusive spinlocks held */ struct systimer *gd_systimer_inprog; /* in-progress systimer */ int gd_timer_running; diff --git a/sys/sys/spinlock2.h b/sys/sys/spinlock2.h index 7fb1e8be33..4d0af1a88f 100644 --- a/sys/sys/spinlock2.h +++ b/sys/sys/spinlock2.h @@ -57,20 +57,10 @@ #include #endif -/* - * SPECIAL NOTE! Obtaining a spinlock does not enter a critical section - * or protect against FAST interrupts but it will prevent thread preemption. - * Because the spinlock code path is ultra critical, we do not check for - * LWKT reschedule requests (due to an interrupt thread not being able to - * preempt). - */ - #ifdef SMP -extern int spin_trylock_wr_contested(globaldata_t gd, struct spinlock *mtx, - int value); -extern void spin_lock_wr_contested(struct spinlock *mtx, int value); -extern void spin_lock_rd_contested(struct spinlock *mtx); +extern int spin_trylock_wr_contested2(globaldata_t gd); +extern void spin_lock_wr_contested2(struct spinlock *mtx); #endif @@ -90,7 +80,7 @@ spin_trylock_wr(struct spinlock *mtx) cpu_ccfence(); ++gd->gd_spinlocks_wr; if ((value = atomic_swap_int(&mtx->lock, SPINLOCK_EXCLUSIVE)) != 0) - return (spin_trylock_wr_contested(gd, mtx, value)); + return (spin_trylock_wr_contested2(gd)); return (TRUE); } @@ -110,8 +100,7 @@ spin_trylock_wr(struct spinlock *mtx) #endif /* - * Obtain an exclusive spinlock and return. Shortcut the case where the only - * cached read lock was from our own cpu (it can just be cleared). + * Obtain an exclusive spinlock and return. */ static __inline void spin_lock_wr_quick(globaldata_t gd, struct spinlock *mtx) @@ -124,11 +113,8 @@ spin_lock_wr_quick(globaldata_t gd, struct spinlock *mtx) cpu_ccfence(); ++gd->gd_spinlocks_wr; #ifdef SMP - if ((value = atomic_swap_int(&mtx->lock, SPINLOCK_EXCLUSIVE)) != 0) { - value &= ~gd->gd_cpumask; - if (value) - spin_lock_wr_contested(mtx, value); - } + if ((value = atomic_swap_int(&mtx->lock, SPINLOCK_EXCLUSIVE)) != 0) + spin_lock_wr_contested2(mtx); #endif } @@ -138,43 +124,6 @@ spin_lock_wr(struct spinlock *mtx) spin_lock_wr_quick(mycpu, mtx); } -/* - * Obtain a shared spinlock and return. This is a critical code path. - * - * The vast majority of the overhead is in the cpu_mfence() (5ns vs 1ns for - * the entire rest of the procedure). Unfortunately we have to ensure that - * spinlock pointer is written out before we check the cpumask to interlock - * against an exclusive spinlock that clears the cpumask and then checks - * the spinlock pointer. - * - * But what is EXTREMELY important here is that we do not have to perform - * a locked bus cycle on the spinlock itself if the shared bit for our cpu - * is already found to be set. We only need the mfence, and the mfence is - * local to the cpu and never conflicts with other cpu's. - * - * This means that multiple parallel shared acessors (e.g. filedescriptor - * table lookups, namecache lookups) run at full speed and incur NO cache - * contention at all. It is the difference between 10ns and 40-100ns. - */ -static __inline void -spin_lock_rd_quick(globaldata_t gd, struct spinlock *mtx) -{ - ++gd->gd_curthread->td_critcount; - cpu_ccfence(); - gd->gd_spinlock_rd = mtx; -#ifdef SMP - cpu_mfence(); - if ((mtx->lock & gd->gd_cpumask) == 0) - spin_lock_rd_contested(mtx); -#endif -} - -static __inline void -spin_lock_rd(struct spinlock *mtx) -{ - spin_lock_rd_quick(mycpu,mtx); -} - /* * Release an exclusive spinlock. We can just do this passively, only * ensuring that our spinlock count is left intact until the mutex is @@ -198,28 +147,6 @@ spin_unlock_wr(struct spinlock *mtx) spin_unlock_wr_quick(mycpu, mtx); } -/* - * Release a shared spinlock. We leave the shared bit set in the spinlock - * as a cache and simply clear the spinlock pointer for the cpu. This - * fast-paths another shared lock later at the cost of an exclusive lock - * having to check per-cpu spinlock pointers to determine when there are no - * shared holders remaining. - */ -static __inline void -spin_unlock_rd_quick(globaldata_t gd, struct spinlock *mtx) -{ - KKASSERT(gd->gd_spinlock_rd == mtx); - gd->gd_spinlock_rd = NULL; - cpu_ccfence(); - --gd->gd_curthread->td_critcount; -} - -static __inline void -spin_unlock_rd(struct spinlock *mtx) -{ - spin_unlock_rd_quick(mycpu, mtx); -} - static __inline void spin_init(struct spinlock *mtx) { -- 2.41.0