From nobody Sun Jul 17 14:36:37 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 4Lm72d1kz2z4WSs8; Sun, 17 Jul 2022 14:36:41 +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 4Lm72d1FSnz3h8v; Sun, 17 Jul 2022 14:36:41 +0000 (UTC) (envelope-from tijl@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1658068601; 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=BubgdVuHpjR+LU4bZWwP6z/XoZBpe+kprfi1J/KQrzA=; b=KfACk5cQkTo9F5l3rZN0xNslPj8AenOmg1CdN3CD+tJlhrsbohFLmFble/Ty6/KuZkJnCL cwtXIzYpsR1Bm1xklT214RJ4BRjTRumqfzhF+5rFITXULmNd4NXImVtaQ/RyOswvBmP8dh +XMKjCqlK5zVXmZavq9oJo3+A030iL8Xq1hVcdmh3Cfb4/or6QfSaB5XUib+zYMCv4Z1Mh gDyAEfVsolPbRdroHYxCL2IeF/0GZnl9TwQzgz0WBy74DALJZZViGTKIMZVy8iVWES7WXb yYMCohk2FMyp5HJLxEvh9iebEIXFZp0nGf/9zLLmEZI74kdQAOTBW+xOUtfUjg== 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 4Lm72c3DKGzrNs; Sun, 17 Jul 2022 14:36:40 +0000 (UTC) (envelope-from tijl@FreeBSD.org) Date: Sun, 17 Jul 2022 16:36:37 +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: <20220717163637.7ed1dc73@FreeBSD.org> In-Reply-To: References: <202207141447.26EElbAZ041128@gitrepo.freebsd.org> <20220717113143.358f28b2@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=UTF-8 Content-Transfer-Encoding: quoted-printable ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1658068601; 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=BubgdVuHpjR+LU4bZWwP6z/XoZBpe+kprfi1J/KQrzA=; b=AeEuT7dEK3v6t6glR3lsh4VelMjxM43FnpyTeD1Fp6LIUtctMOdFsd/IBONtBROegFZwFb BWk4WrW5KrnkWoUrPwDdB3KXiaKmI2EwH9bFYzUBKE1ii5JHMYgW+aoK49T7UBP/NQG8Pt Y0YlJbgu0iwHEf/pwYv+j205qKXPTiTI9LgSanOGc1Qo6n39yqjPnj2thFGAHJFuw3RCOx GduWx2o9YjsPRKVymJvoGDEjo0ggl6Y9lO0z/ExJfH+hPBDQEcy8OS0w7Mf5Rs+VlxPU4K 84jeQdfbaRnAoOcm4Ei7YEPpxs+c4f8SO6p6xCjRCcp0QmPy0jbh0XUR0eq+Ew== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1658068601; a=rsa-sha256; cv=none; b=NPvcpz78ckF1j9TrRB8t+8YE9e2bVyoyUPPeNqRaTcGtdU3MWcMDmadGI/365MA9PgD3g/ hgNnGSF9AL1WoTp28GBRV1wfLqAHDTwxguEhouAVlGl4xgMSv6NPEFUomaY9PjvPxIrNUY w8Br6tSy3Qio242nCtQClJoBQN/cYPyIXBUgMY6RzQFfxrsvelt+U3QoJNf2i1R0Ep5QjJ nJacddQmubX5Tl20yqy7Oxs5scs8ds8VdahQ12G/xpyJPSQIQQlnQDwgXwebLaxSCTdtA8 vXEv9pS7W+MXbLAzMUtQE7wWXDd0EV54BYVfEoTScvYLuoIDHF/ATXG8JJYi7A== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N On Sun, 17 Jul 2022 06:16:24 -0400 Mark Johnston wrote: > On Sun, Jul 17, 2022 at 11:31:43AM +0200, T=C4=B3l Coosemans wrote: > > On Thu, 14 Jul 2022 14:47:37 GMT Mark Johnston wrot= e: =20 > > > The branch main has been updated by markj: > > >=20 > > > URL: https://cgit.FreeBSD.org/src/commit/?id=3D03f868b163ad46d6f7cb03= dc46fb83ca01fb8f69 > > >=20 > > > commit 03f868b163ad46d6f7cb03dc46fb83ca01fb8f69 > > > Author: Mark Johnston > > > AuthorDate: 2022-07-14 14:24:25 +0000 > > > Commit: Mark Johnston > > > CommitDate: 2022-07-14 14:28:01 +0000 > > >=20 > > > x86: Add a required store-load barrier in cpu_idle() > > > =20 > > > ULE's tdq_notify() tries to avoid delivering IPIs to the idle thr= ead. > > > In particular, it tries to detect whether the idle thread is runn= ing. > > > There are two mechanisms for this: > > > - tdq_cpu_idle, an MI flag which is set prior to calling cpu_idle= (). If > > > tdq_cpu_idle =3D=3D 0, then no IPI is needed; > > > - idle_state, an x86-specific state flag which is updated after > > > cpu_idleclock() is called. > > > =20 > > > 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 =3D STATE_SLEEPING, then check for pending work by loa= ding the > > > tdq_load field of the CPU's runqueue. These operations can be re= ordered > > > so that the idle thread observes tdq_load =3D=3D 0, and tdq_notif= y() > > > observes idle_state =3D=3D STATE_RUNNING. > > > =20 > > > Some counters indicate that the idle_state check in tdq_notify() > > > frequently elides an IPI. So, fix the problem by inserting a fen= ce > > > after the store to idle_state, immediately before idling the CPU. > > > =20 > > > 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(-) > > >=20 > > > 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 =3D 1; /* Use MONITOR/MW= AIT for short idle. */ > > > SYSCTL_INT(_machdep, OID_AUTO, idle_mwait, CTLFLAG_RWTUN, &idle_mwai= t, > > > 0, "Use MONITOR/MWAIT for short idle"); > > > =20 > > > -static void > > > -cpu_idle_acpi(sbintime_t sbt) > > > +static bool > > > +cpu_idle_enter(int *statep, int newstate) > > > { > > > - int *state; > > > + KASSERT(atomic_load_int(statep) =3D=3D STATE_RUNNING, > > > + ("%s: state %d", __func__, atomic_load_int(statep))); > > > =20 > > > - state =3D &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 =3D &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. Withou= t it, > > > + * cpu_idle_wakeup() can observe the state as STATE_RUNNING after h= aving > > > + * added load to the queue, and elide an IPI. Then, sched_runnable= () > > > + * can observe tdq_load =3D=3D 0, so the CPU ends up idling with pe= nding > > > + * 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 =20 > >=20 > > 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? > >=20 > > #if defined(SCHED_ULE) && defined(SMP) > > atomic_swap_int(statep, newstate); > > #else > > atomic_store_int(statep, newstate); > > #endif =20 >=20 > Yes, I believe so. I wrote it that way initially, but decided it was > nicer to use the same fence as the MI code with which this synchronizes. > What's the advantage of using xchg instead, given that the CPU is about > to idle? ok, I was just wondering.