Date: Sat, 7 Nov 2020 16:57:54 +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: r367453 - in head/sys: kern sys Message-ID: <202011071657.0A7Gvsef091054@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: mjg Date: Sat Nov 7 16:57:53 2020 New Revision: 367453 URL: https://svnweb.freebsd.org/changeset/base/367453 Log: rms: several cleanups + debug read lockers handling This adds a dedicated counter updated with atomics when INVARIANTS is used. As a side effect one can reliably determine the lock is held for reading by at least one thread, but it's still not possible to find out whether curthread has the lock in said mode. This should be good enough in practice. Problem spotted by avg. Modified: head/sys/kern/kern_rmlock.c head/sys/sys/_rmlock.h head/sys/sys/rmlock.h Modified: head/sys/kern/kern_rmlock.c ============================================================================== --- head/sys/kern/kern_rmlock.c Sat Nov 7 16:41:59 2020 (r367452) +++ head/sys/kern/kern_rmlock.c Sat Nov 7 16:57:53 2020 (r367453) @@ -878,10 +878,105 @@ db_show_rm(const struct lock_object *lock) * problem at some point. The easiest way to lessen it is to provide a bitmap. */ +#define rms_int_membar() __compiler_membar() + #define RMS_NOOWNER ((void *)0x1) #define RMS_TRANSIENT ((void *)0x2) #define RMS_FLAGMASK 0xf +struct rmslock_pcpu { + int influx; + int readers; +}; + +_Static_assert(sizeof(struct rmslock_pcpu) == 8, "bad size"); + +/* + * Internal routines + */ +static struct rmslock_pcpu * +rms_int_pcpu(struct rmslock *rms) +{ + + CRITICAL_ASSERT(curthread); + return (zpcpu_get(rms->pcpu)); +} + +static struct rmslock_pcpu * +rms_int_remote_pcpu(struct rmslock *rms, int cpu) +{ + + return (zpcpu_get_cpu(rms->pcpu, cpu)); +} + +static void +rms_int_influx_enter(struct rmslock *rms, struct rmslock_pcpu *pcpu) +{ + + CRITICAL_ASSERT(curthread); + MPASS(pcpu->influx == 0); + pcpu->influx = 1; +} + +static void +rms_int_influx_exit(struct rmslock *rms, struct rmslock_pcpu *pcpu) +{ + + CRITICAL_ASSERT(curthread); + MPASS(pcpu->influx == 1); + pcpu->influx = 0; +} + +#ifdef INVARIANTS +static void +rms_int_debug_readers_inc(struct rmslock *rms) +{ + int old; + old = atomic_fetchadd_int(&rms->debug_readers, 1); + KASSERT(old >= 0, ("%s: bad readers count %d\n", __func__, old)); +} + +static void +rms_int_debug_readers_dec(struct rmslock *rms) +{ + int old; + + old = atomic_fetchadd_int(&rms->debug_readers, -1); + KASSERT(old > 0, ("%s: bad readers count %d\n", __func__, old)); +} +#else +static void +rms_int_debug_readers_inc(struct rmslock *rms) +{ +} + +static void +rms_int_debug_readers_dec(struct rmslock *rms) +{ +} +#endif + +static void +rms_int_readers_inc(struct rmslock *rms, struct rmslock_pcpu *pcpu) +{ + + CRITICAL_ASSERT(curthread); + rms_int_debug_readers_inc(rms); + pcpu->readers++; +} + +static void +rms_int_readers_dec(struct rmslock *rms, struct rmslock_pcpu *pcpu) +{ + + CRITICAL_ASSERT(curthread); + rms_int_debug_readers_dec(rms); + pcpu->readers--; +} + +/* + * Public API + */ void rms_init(struct rmslock *rms, const char *name) { @@ -889,9 +984,9 @@ rms_init(struct rmslock *rms, const char *name) rms->owner = RMS_NOOWNER; rms->writers = 0; rms->readers = 0; + rms->debug_readers = 0; mtx_init(&rms->mtx, name, NULL, MTX_DEF | MTX_NEW); - rms->readers_pcpu = uma_zalloc_pcpu(pcpu_zone_4, M_WAITOK | M_ZERO); - rms->readers_influx = uma_zalloc_pcpu(pcpu_zone_4, M_WAITOK | M_ZERO); + rms->pcpu = uma_zalloc_pcpu(pcpu_zone_8, M_WAITOK | M_ZERO); } void @@ -901,23 +996,21 @@ rms_destroy(struct rmslock *rms) MPASS(rms->writers == 0); MPASS(rms->readers == 0); mtx_destroy(&rms->mtx); - uma_zfree_pcpu(pcpu_zone_4, rms->readers_pcpu); - uma_zfree_pcpu(pcpu_zone_4, rms->readers_influx); + uma_zfree_pcpu(pcpu_zone_8, rms->pcpu); } static void __noinline rms_rlock_fallback(struct rmslock *rms) { - zpcpu_set_protected(rms->readers_influx, 0); + rms_int_influx_exit(rms, rms_int_pcpu(rms)); critical_exit(); mtx_lock(&rms->mtx); - MPASS(*zpcpu_get(rms->readers_pcpu) == 0); while (rms->writers > 0) msleep(&rms->readers, &rms->mtx, PUSER - 1, mtx_name(&rms->mtx), 0); critical_enter(); - zpcpu_add_protected(rms->readers_pcpu, 1); + rms_int_readers_inc(rms, rms_int_pcpu(rms)); mtx_unlock(&rms->mtx); critical_exit(); } @@ -925,43 +1018,46 @@ rms_rlock_fallback(struct rmslock *rms) void rms_rlock(struct rmslock *rms) { + struct rmslock_pcpu *pcpu; WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL, __func__); MPASS(atomic_load_ptr(&rms->owner) != curthread); critical_enter(); - zpcpu_set_protected(rms->readers_influx, 1); - __compiler_membar(); + pcpu = rms_int_pcpu(rms); + rms_int_influx_enter(rms, pcpu); + rms_int_membar(); if (__predict_false(rms->writers > 0)) { rms_rlock_fallback(rms); return; } - __compiler_membar(); - zpcpu_add_protected(rms->readers_pcpu, 1); - __compiler_membar(); - zpcpu_set_protected(rms->readers_influx, 0); + rms_int_membar(); + rms_int_readers_inc(rms, pcpu); + rms_int_membar(); + rms_int_influx_exit(rms, pcpu); critical_exit(); } int rms_try_rlock(struct rmslock *rms) { + struct rmslock_pcpu *pcpu; MPASS(atomic_load_ptr(&rms->owner) != curthread); critical_enter(); - zpcpu_set_protected(rms->readers_influx, 1); - __compiler_membar(); + pcpu = rms_int_pcpu(rms); + rms_int_influx_enter(rms, pcpu); + rms_int_membar(); if (__predict_false(rms->writers > 0)) { - __compiler_membar(); - zpcpu_set_protected(rms->readers_influx, 0); + rms_int_influx_exit(rms, pcpu); critical_exit(); return (0); } - __compiler_membar(); - zpcpu_add_protected(rms->readers_pcpu, 1); - __compiler_membar(); - zpcpu_set_protected(rms->readers_influx, 0); + rms_int_membar(); + rms_int_readers_inc(rms, pcpu); + rms_int_membar(); + rms_int_influx_exit(rms, pcpu); critical_exit(); return (1); } @@ -970,13 +1066,14 @@ static void __noinline rms_runlock_fallback(struct rmslock *rms) { - zpcpu_set_protected(rms->readers_influx, 0); + rms_int_influx_exit(rms, rms_int_pcpu(rms)); critical_exit(); mtx_lock(&rms->mtx); - MPASS(*zpcpu_get(rms->readers_pcpu) == 0); MPASS(rms->writers > 0); MPASS(rms->readers > 0); + MPASS(rms->debug_readers == rms->readers); + rms_int_debug_readers_dec(rms); rms->readers--; if (rms->readers == 0) wakeup_one(&rms->writers); @@ -986,18 +1083,20 @@ rms_runlock_fallback(struct rmslock *rms) void rms_runlock(struct rmslock *rms) { + struct rmslock_pcpu *pcpu; critical_enter(); - zpcpu_set_protected(rms->readers_influx, 1); - __compiler_membar(); + pcpu = rms_int_pcpu(rms); + rms_int_influx_enter(rms, pcpu); + rms_int_membar(); if (__predict_false(rms->writers > 0)) { rms_runlock_fallback(rms); return; } - __compiler_membar(); - zpcpu_sub_protected(rms->readers_pcpu, 1); - __compiler_membar(); - zpcpu_set_protected(rms->readers_influx, 0); + rms_int_membar(); + rms_int_readers_dec(rms, pcpu); + rms_int_membar(); + rms_int_influx_exit(rms, pcpu); critical_exit(); } @@ -1010,17 +1109,19 @@ static void rms_action_func(void *arg) { struct rmslock_ipi *rmsipi; + struct rmslock_pcpu *pcpu; struct rmslock *rms; - int readers; rmsipi = __containerof(arg, struct rmslock_ipi, srcra); rms = rmsipi->rms; + pcpu = rms_int_pcpu(rms); - if (*zpcpu_get(rms->readers_influx)) + if (pcpu->influx) return; - readers = zpcpu_replace(rms->readers_pcpu, 0); - if (readers != 0) - atomic_add_int(&rms->readers, readers); + if (pcpu->readers != 0) { + atomic_add_int(&rms->readers, pcpu->readers); + pcpu->readers = 0; + } smp_rendezvous_cpus_done(arg); } @@ -1028,18 +1129,40 @@ static void rms_wait_func(void *arg, int cpu) { struct rmslock_ipi *rmsipi; + struct rmslock_pcpu *pcpu; struct rmslock *rms; - int *in_op; rmsipi = __containerof(arg, struct rmslock_ipi, srcra); rms = rmsipi->rms; + pcpu = rms_int_remote_pcpu(rms, cpu); - in_op = zpcpu_get_cpu(rms->readers_influx, cpu); - while (atomic_load_int(in_op)) + while (atomic_load_int(&pcpu->influx)) cpu_spinwait(); } +#ifdef INVARIANTS static void +rms_assert_no_pcpu_readers(struct rmslock *rms) +{ + struct rmslock_pcpu *pcpu; + int cpu; + + CPU_FOREACH(cpu) { + pcpu = rms_int_remote_pcpu(rms, cpu); + if (pcpu->readers != 0) { + panic("%s: got %d readers on cpu %d\n", __func__, + pcpu->readers, cpu); + } + } +} +#else +static void +rms_assert_no_pcpu_readers(struct rmslock *rms) +{ +} +#endif + +static void rms_wlock_switch(struct rmslock *rms) { struct rmslock_ipi rmsipi; @@ -1080,6 +1203,7 @@ rms_wlock(struct rmslock *rms) ("%s: unexpected owner value %p\n", __func__, rms->owner)); rms_wlock_switch(rms); + rms_assert_no_pcpu_readers(rms); if (rms->readers > 0) { msleep(&rms->writers, &rms->mtx, (PUSER - 1), @@ -1088,6 +1212,7 @@ rms_wlock(struct rmslock *rms) out_grab: rms->owner = curthread; + rms_assert_no_pcpu_readers(rms); mtx_unlock(&rms->mtx); MPASS(rms->readers == 0); } Modified: head/sys/sys/_rmlock.h ============================================================================== --- head/sys/sys/_rmlock.h Sat Nov 7 16:41:59 2020 (r367452) +++ head/sys/sys/_rmlock.h Sat Nov 7 16:57:53 2020 (r367453) @@ -70,13 +70,15 @@ struct rm_priotracker { #include <sys/_mutex.h> +struct rmslock_pcpu; + struct rmslock { struct mtx mtx; struct thread *owner; + struct rmslock_pcpu *pcpu; int writers; int readers; - int *readers_pcpu; - int *readers_influx; + int debug_readers; }; #endif /* !_SYS__RMLOCK_H_ */ Modified: head/sys/sys/rmlock.h ============================================================================== --- head/sys/sys/rmlock.h Sat Nov 7 16:41:59 2020 (r367452) +++ head/sys/sys/rmlock.h Sat Nov 7 16:57:53 2020 (r367453) @@ -149,14 +149,18 @@ rms_wowned(struct rmslock *rms) return (rms->owner == curthread); } +#ifdef INVARIANTS /* - * Only valid to call if you hold the lock in some manner. + * For assertion purposes. + * + * Main limitation is that we at best can tell there are readers, but not + * whether curthread is one of them. */ static inline int rms_rowned(struct rmslock *rms) { - return (rms->readers > 0); + return (rms->debug_readers > 0); } static inline int @@ -168,6 +172,7 @@ rms_owned_any(struct rmslock *rms) return (rms_rowned(rms)); } +#endif #endif /* _KERNEL */ #endif /* !_SYS_RMLOCK_H_ */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202011071657.0A7Gvsef091054>