From owner-svn-src-all@freebsd.org Sat Nov 7 03:29:04 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 F037A2DF4B6; Sat, 7 Nov 2020 03:29:04 +0000 (UTC) (envelope-from kevans@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 4CSjR46TtYz3jNl; Sat, 7 Nov 2020 03:29:04 +0000 (UTC) (envelope-from kevans@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 D1AA12FF84; Sat, 7 Nov 2020 03:29:04 +0000 (UTC) (envelope-from kevans@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 0A73T4DS099076; Sat, 7 Nov 2020 03:29:04 GMT (envelope-from kevans@FreeBSD.org) Received: (from kevans@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 0A73T4Oe099075; Sat, 7 Nov 2020 03:29:04 GMT (envelope-from kevans@FreeBSD.org) Message-Id: <202011070329.0A73T4Oe099075@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: kevans set sender to kevans@FreeBSD.org using -f From: Kyle Evans Date: Sat, 7 Nov 2020 03:29:04 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r367440 - head/sys/kern X-SVN-Group: head X-SVN-Commit-Author: kevans X-SVN-Commit-Paths: head/sys/kern X-SVN-Commit-Revision: 367440 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 03:29:05 -0000 Author: kevans Date: Sat Nov 7 03:29:04 2020 New Revision: 367440 URL: https://svnweb.freebsd.org/changeset/base/367440 Log: epoch: support non-preemptible epochs checking in_epoch() Previously, non-preemptible epochs could not check; in_epoch() would always fail, usually because non-preemptible epochs don't imply THREAD_NO_SLEEPING. For default epochs, it's easy enough to verify that we're in the given epoch: if we're in a critical section and our record for the given epoch is active, then we're in it. This patch also adds some additional INVARIANTS bookkeeping. Notably, we set and check the recorded thread in epoch_enter/epoch_exit to try and catch some edge-cases for the caller. It also checks upon freeing that none of the records had a thread in the epoch, which may make it a little easier to diagnose some improper use if epoch_free() took place while some other thread was inside. This version differs slightly from what was just previously reviewed by the below-listed, in that in_epoch() will assert that no CPU has this thread recorded even if it *is* currently in a critical section. This is intended to catch cases where the caller might have somehow messed up critical section nesting, we can catch both if they exited the critical section or if they exited, migrated, then re-entered (on the wrong CPU). Reviewed by: kib, markj (both previous version) MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D27098 Modified: head/sys/kern/subr_epoch.c Modified: head/sys/kern/subr_epoch.c ============================================================================== --- head/sys/kern/subr_epoch.c Sat Nov 7 03:28:32 2020 (r367439) +++ head/sys/kern/subr_epoch.c Sat Nov 7 03:29:04 2020 (r367440) @@ -72,6 +72,10 @@ typedef struct epoch_record { volatile struct epoch_tdlist er_tdlist; volatile uint32_t er_gen; uint32_t er_cpuid; +#ifdef INVARIANTS + /* Used to verify record ownership for non-preemptible epochs. */ + struct thread *er_td; +#endif } __aligned(EPOCH_ALIGN) *epoch_record_t; struct epoch { @@ -377,6 +381,9 @@ done: void epoch_free(epoch_t epoch) { +#ifdef INVARIANTS + int cpu; +#endif EPOCH_LOCK(); @@ -390,6 +397,21 @@ epoch_free(epoch_t epoch) * to zero, by calling epoch_wait() on the global_epoch: */ epoch_wait(global_epoch); +#ifdef INVARIANTS + CPU_FOREACH(cpu) { + epoch_record_t er; + + er = zpcpu_get_cpu(epoch->e_pcpu_record, cpu); + + /* + * Sanity check: none of the records should be in use anymore. + * We drained callbacks above and freeing the pcpu records is + * imminent. + */ + MPASS(er->er_td == NULL); + MPASS(TAILQ_EMPTY(&er->er_tdlist)); + } +#endif uma_zfree_pcpu(pcpu_zone_record, epoch->e_pcpu_record); mtx_destroy(&epoch->e_drain_mtx); sx_destroy(&epoch->e_drain_sx); @@ -434,6 +456,8 @@ _epoch_enter_preempt(epoch_t epoch, epoch_tracker_t et sched_pin(); td->td_pre_epoch_prio = td->td_priority; er = epoch_currecord(epoch); + /* Record-level tracking is reserved for non-preemptible epochs. */ + MPASS(er->er_td == NULL); TAILQ_INSERT_TAIL(&er->er_tdlist, et, et_link); ck_epoch_begin(&er->er_record, &et->et_section); critical_exit(); @@ -448,6 +472,15 @@ epoch_enter(epoch_t epoch) INIT_CHECK(epoch); critical_enter(); er = epoch_currecord(epoch); +#ifdef INVARIANTS + if (er->er_record.active == 0) { + MPASS(er->er_td == NULL); + er->er_td = curthread; + } else { + /* We've recursed, just make sure our accounting isn't wrong. */ + MPASS(er->er_td == curthread); + } +#endif ck_epoch_begin(&er->er_record, NULL); } @@ -468,6 +501,8 @@ _epoch_exit_preempt(epoch_t epoch, epoch_tracker_t et MPASS(et->et_td == td); #ifdef INVARIANTS et->et_td = (void*)0xDEADBEEF; + /* Record-level tracking is reserved for non-preemptible epochs. */ + MPASS(er->er_td == NULL); #endif ck_epoch_end(&er->er_record, &et->et_section); TAILQ_REMOVE(&er->er_tdlist, et, et_link); @@ -488,6 +523,11 @@ epoch_exit(epoch_t epoch) INIT_CHECK(epoch); er = epoch_currecord(epoch); ck_epoch_end(&er->er_record, NULL); +#ifdef INVARIANTS + MPASS(er->er_td == curthread); + if (er->er_record.active == 0) + er->er_td = NULL; +#endif critical_exit(); } @@ -777,18 +817,18 @@ epoch_call_task(void *arg __unused) } } -int -in_epoch_verbose(epoch_t epoch, int dump_onfail) +static int +in_epoch_verbose_preempt(epoch_t epoch, int dump_onfail) { + epoch_record_t er; struct epoch_tracker *tdwait; struct thread *td; - epoch_record_t er; + MPASS(epoch != NULL); + MPASS((epoch->e_flags & EPOCH_PREEMPT) != 0); td = curthread; if (THREAD_CAN_SLEEP()) return (0); - if (__predict_false((epoch) == NULL)) - return (0); critical_enter(); er = epoch_currecord(epoch); TAILQ_FOREACH(tdwait, &er->er_tdlist, et_link) @@ -807,6 +847,66 @@ in_epoch_verbose(epoch_t epoch, int dump_onfail) #endif critical_exit(); return (0); +} + +#ifdef INVARIANTS +static void +epoch_assert_nocpu(epoch_t epoch, struct thread *td) +{ + epoch_record_t er; + int cpu; + bool crit; + + crit = td->td_critnest > 0; + + /* Check for a critical section mishap. */ + CPU_FOREACH(cpu) { + er = zpcpu_get_cpu(epoch->e_pcpu_record, cpu); + KASSERT(er->er_td != td, + ("%s critical section in epoch '%s', from cpu %d", + (crit ? "exited" : "re-entered"), epoch->e_name, cpu)); + } +} +#else +#define epoch_assert_nocpu(e, td) +#endif + +int +in_epoch_verbose(epoch_t epoch, int dump_onfail) +{ + epoch_record_t er; + struct thread *td; + + if (__predict_false((epoch) == NULL)) + return (0); + if ((epoch->e_flags & EPOCH_PREEMPT) != 0) + return (in_epoch_verbose_preempt(epoch, dump_onfail)); + + /* + * The thread being in a critical section is a necessary + * condition to be correctly inside a non-preemptible epoch, + * so it's definitely not in this epoch. + */ + td = curthread; + if (td->td_critnest == 0) { + epoch_assert_nocpu(epoch, td); + return (0); + } + + /* + * The current cpu is in a critical section, so the epoch record will be + * stable for the rest of this function. Knowing that the record is not + * active is sufficient for knowing whether we're in this epoch or not, + * since it's a pcpu record. + */ + er = epoch_currecord(epoch); + if (er->er_record.active == 0) { + epoch_assert_nocpu(epoch, td); + return (0); + } + + MPASS(er->er_td == td); + return (1); } int