From nobody Sun Jul 17 09:31:43 2022 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Lm0H538yKz4WlY1; Sun, 17 Jul 2022 09:32:01 +0000 (UTC) (envelope-from tijl@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (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 "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Lm0H52k9Wz3FhP; Sun, 17 Jul 2022 09:32:01 +0000 (UTC) (envelope-from tijl@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1658050321; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=skUMxG0rz92gFFlwOt+wS3sstNyMnUX2LAgPTE69J0M=; b=ifJwDs9PUwemfqtia+8PPbNfRN2GfochFVnBmEn1LXkfYAKF9/TrDnZr+JAgJ30cldcqn7 Stlty6i/IY/4KP86O2EuJRAhIh3RfxWVzFmIbWctY7rX8Py75CmUMGK3v5jxLszSeJyWmO oHJPicwwn5+00OpO4W22tvrtv1jNvmOACv0ZwMRzi/Fktz4SnLDuB1akYxAv1eQdYs4Bk4 mKUdkNatv/tROWJHqtxIfXnbFJIg+pnAh1AqR+1KEnVl3eg6NtOLFh/bZEktLFVg3vOP03 vJYzo86MpuXie0DRNGl7vuEUfNtEufLQWmaAHJx5BEu5X0mXb2ud8YkI3Sj9VQ== Received: from localhost (unknown [IPv6:2a02:a03f:894b:4700:289e:c43f:dbf:a065]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: tijl) by smtp.freebsd.org (Postfix) with ESMTPSA id 4Lm0H44fHqzmjM; Sun, 17 Jul 2022 09:32:00 +0000 (UTC) (envelope-from tijl@FreeBSD.org) Date: Sun, 17 Jul 2022 11:31:43 +0200 From: =?UTF-8?B?VMSzbA==?= Coosemans To: Mark Johnston Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 03f868b163ad - main - x86: Add a required store-load barrier in cpu_idle() Message-ID: <20220717113143.358f28b2@FreeBSD.org> In-Reply-To: <202207141447.26EElbAZ041128@gitrepo.freebsd.org> References: <202207141447.26EElbAZ041128@gitrepo.freebsd.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1658050321; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=skUMxG0rz92gFFlwOt+wS3sstNyMnUX2LAgPTE69J0M=; b=q7Z6IjvtLk0Mn6uo4dbf+pnhnJ+oNr4ZlV5sZ6wqM54VrRJ7rTiGF6oV6d0YH3ohWcTO3R X6mQECXvd6m30vIS/I3hCvVVKUs5lJlElcczoZWGT9nNGrYwmH0oIwhc3xaUTjCmVzZhzl B+jm5C8dtre3AGVWHFbiXDkcvmRVMMqRSgES8wWildJQ24gqsHisvKA7YiyeFQ69y8tWH2 zKy3PYchxl7DZZAUYAjsTvC/Pe7hNMufA8n3Vr1yePrK12/4uDBo/Zq0mvuzu1Nnz/SZNq h9hccUiUB5d5yjx92EVgzNgiQO/oh9j+pgbHN6pduC/KJolX1fJE9DUHIT6hBA== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1658050321; a=rsa-sha256; cv=none; b=xryrto61o/NDUxpP6u7dReuYw6MeAkXsctFm/XlR3dseaxAZRwA4Cr6H6xUstWPqFPwAvp uUCFsbq17wqGyk7VGbvpaMTdLyLBbth0nfDPdy7La7jgPsPmHGBrVuidi4rHQWa0cFqZnn bN2VHP7zW1/YmWctxb5BN3R0Clvn6zyMe9srz7jWJXN1spOkQcHB0gv4VXIXHHvFTyuwQC jm9UOoU+I79BBX/XvHEmcUppmg3V4AZ2whidugznqajT15MKho30oN9cPgDTarDKXrUA7s UGNfqV+aFxv5Q0+RNtb2o27wnTKiXcmW6YPmBMZgcDFR1w2qlaB2YFSnc3m5Bg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N On Thu, 14 Jul 2022 14:47:37 GMT Mark Johnston wrote: > The branch main has been updated by markj: > > URL: https://cgit.FreeBSD.org/src/commit/?id=03f868b163ad46d6f7cb03dc46fb83ca01fb8f69 > > commit 03f868b163ad46d6f7cb03dc46fb83ca01fb8f69 > Author: Mark Johnston > AuthorDate: 2022-07-14 14:24:25 +0000 > Commit: Mark Johnston > CommitDate: 2022-07-14 14:28:01 +0000 > > x86: Add a required store-load barrier in cpu_idle() > > ULE's tdq_notify() tries to avoid delivering IPIs to the idle thread. > In particular, it tries to detect whether the idle thread is running. > There are two mechanisms for this: > - tdq_cpu_idle, an MI flag which is set prior to calling cpu_idle(). If > tdq_cpu_idle == 0, then no IPI is needed; > - idle_state, an x86-specific state flag which is updated after > cpu_idleclock() is called. > > The implementation of the second mechanism is racy; the race can cause a > CPU to go to sleep with pending work. Specifically, cpu_idle_*() set > idle_state = STATE_SLEEPING, then check for pending work by loading the > tdq_load field of the CPU's runqueue. These operations can be reordered > so that the idle thread observes tdq_load == 0, and tdq_notify() > observes idle_state == STATE_RUNNING. > > Some counters indicate that the idle_state check in tdq_notify() > frequently elides an IPI. So, fix the problem by inserting a fence > after the store to idle_state, immediately before idling the CPU. > > PR: 264867 > Reviewed by: mav, kib, jhb > MFC after: 1 month > Sponsored by: The FreeBSD Foundation > Differential Revision: https://reviews.freebsd.org/D35777 > --- > sys/x86/x86/cpu_machdep.c | 103 ++++++++++++++++++++++++++++------------------ > 1 file changed, 62 insertions(+), 41 deletions(-) > > diff --git a/sys/x86/x86/cpu_machdep.c b/sys/x86/x86/cpu_machdep.c > index fa11f64e2779..040438043c73 100644 > --- a/sys/x86/x86/cpu_machdep.c > +++ b/sys/x86/x86/cpu_machdep.c > @@ -52,6 +52,7 @@ __FBSDID("$FreeBSD$"); > #include "opt_maxmem.h" > #include "opt_mp_watchdog.h" > #include "opt_platform.h" > +#include "opt_sched.h" > #ifdef __i386__ > #include "opt_apic.h" > #endif > @@ -532,32 +533,25 @@ static int idle_mwait = 1; /* Use MONITOR/MWAIT for short idle. */ > SYSCTL_INT(_machdep, OID_AUTO, idle_mwait, CTLFLAG_RWTUN, &idle_mwait, > 0, "Use MONITOR/MWAIT for short idle"); > > -static void > -cpu_idle_acpi(sbintime_t sbt) > +static bool > +cpu_idle_enter(int *statep, int newstate) > { > - int *state; > + KASSERT(atomic_load_int(statep) == STATE_RUNNING, > + ("%s: state %d", __func__, atomic_load_int(statep))); > > - state = &PCPU_PTR(monitorbuf)->idle_state; > - atomic_store_int(state, STATE_SLEEPING); > - > - /* See comments in cpu_idle_hlt(). */ > - disable_intr(); > - if (sched_runnable()) > - enable_intr(); > - else if (cpu_idle_hook) > - cpu_idle_hook(sbt); > - else > - acpi_cpu_c1(); > - atomic_store_int(state, STATE_RUNNING); > -} > - > -static void > -cpu_idle_hlt(sbintime_t sbt) > -{ > - int *state; > - > - state = &PCPU_PTR(monitorbuf)->idle_state; > - atomic_store_int(state, STATE_SLEEPING); > + /* > + * A fence is needed to prevent reordering of the load in > + * sched_runnable() with this store to the idle state word. Without it, > + * cpu_idle_wakeup() can observe the state as STATE_RUNNING after having > + * added load to the queue, and elide an IPI. Then, sched_runnable() > + * can observe tdq_load == 0, so the CPU ends up idling with pending > + * work. tdq_notify() similarly ensures that a prior update to tdq_load > + * is visible before calling cpu_idle_wakeup(). > + */ > + atomic_store_int(statep, newstate); > +#if defined(SCHED_ULE) && defined(SMP) > + atomic_thread_fence_seq_cst(); > +#endif Can't you combine the store and the fence with atomic_store_explicit(memory_order_seq_cst) or, since this is x86 specific code, atomic_swap_int? #if defined(SCHED_ULE) && defined(SMP) atomic_swap_int(statep, newstate); #else atomic_store_int(statep, newstate); #endif > /* > * Since we may be in a critical section from cpu_idle(), if > @@ -576,35 +570,62 @@ cpu_idle_hlt(sbintime_t sbt) > * interrupt. > */ > disable_intr(); > - if (sched_runnable()) > + if (sched_runnable()) { > enable_intr(); > - else > - acpi_cpu_c1(); > - atomic_store_int(state, STATE_RUNNING); > + atomic_store_int(statep, STATE_RUNNING); > + return (false); > + } else { > + return (true); > + } > } > > static void > -cpu_idle_mwait(sbintime_t sbt) > +cpu_idle_exit(int *statep) > +{ > + atomic_store_int(statep, STATE_RUNNING); > +} > +static void > +cpu_idle_hlt(sbintime_t sbt) > +{ > + int *state; > + > + state = &PCPU_PTR(monitorbuf)->idle_state; > + if (cpu_idle_enter(state, STATE_SLEEPING)) { > + acpi_cpu_c1(); > atomic_store_int(state, STATE_RUNNING); Should this call be replaced with cpu_idle_exit(state), just for consistency? > - enable_intr(); > - return; > } > +}