Date: Sun, 17 Jul 2022 16:36:37 +0200 From: =?UTF-8?B?VMSzbA==?= Coosemans <tijl@FreeBSD.org> To: Mark Johnston <markj@freebsd.org> 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: <YtPheIQaFGkg%2B0sd@nuc> References: <202207141447.26EElbAZ041128@gitrepo.freebsd.org> <20220717113143.358f28b2@FreeBSD.org> <YtPheIQaFGkg%2B0sd@nuc>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 17 Jul 2022 06:16:24 -0400 Mark Johnston <markj@freebsd.org> 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 <markj@FreeBSD.org> 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 <markj@FreeBSD.org> > > > AuthorDate: 2022-07-14 14:24:25 +0000 > > > Commit: Mark Johnston <markj@FreeBSD.org> > > > 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20220717163637.7ed1dc73>