From owner-svn-src-all@freebsd.org Sat Nov 7 16:57:54 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id D777F464FA3; Sat, 7 Nov 2020 16:57:54 +0000 (UTC) (envelope-from mjg@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CT3NL5qG0z3Jff; Sat, 7 Nov 2020 16:57:54 +0000 (UTC) (envelope-from mjg@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id BAA1F11FB3; Sat, 7 Nov 2020 16:57:54 +0000 (UTC) (envelope-from mjg@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 0A7Gvsqw091058; Sat, 7 Nov 2020 16:57:54 GMT (envelope-from mjg@FreeBSD.org) Received: (from mjg@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 0A7Gvsef091054; Sat, 7 Nov 2020 16:57:54 GMT (envelope-from mjg@FreeBSD.org) Message-Id: <202011071657.0A7Gvsef091054@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: mjg set sender to mjg@FreeBSD.org using -f From: Mateusz Guzik Date: Sat, 7 Nov 2020 16:57:54 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r367453 - in head/sys: kern sys X-SVN-Group: head X-SVN-Commit-Author: mjg X-SVN-Commit-Paths: in head/sys: kern sys X-SVN-Commit-Revision: 367453 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 07 Nov 2020 16:57:54 -0000 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 +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_ */