Re: [PATCH] x86: add workaround monitor bug

From: Jacob Pan
Date: Mon Jul 18 2016 - 14:47:11 EST


On Fri, 8 Jul 2016 12:14:33 -0700
Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx> wrote:

> On Fri, 8 Jul 2016 14:07:12 +0200
> Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> >
> > * Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >
> > > On Fri, Jul 08, 2016 at 10:55:15AM +0200, Ingo Molnar wrote:
> > >
> > > > > static inline void mwait_idle_with_hints(unsigned long eax,
> > > > > unsigned long ecx) {
> > > > > - if (!current_set_polling_and_test()) {
> > > > > + if (static_cpu_has_bug(X86_BUG_MONITOR)
> > > > > || !current_set_polling_and_test()) {
> > > >
> > > > Hm, this might be suboptimal: if MONITOR/MWAIT is implemented by
> > > > setting the exclusive flag for the monitored memory address and
> > > > then snooping for cache invalidation requests for that cache
> > > > line, then not modifying the ->flags value with
> > > > TIF_POLLING_NRFLAG makes MWAIT not wake up - only the IPI would
> > > > wake it up.
> > >
> > > Confused.. POLLING_NRFLAGS is not used to wake up ever. It is only
> > > used to determine if we want to send IPIs or not.
> >
> > I called the IPI the 'wakeup' - it's the 'CPU wakeup' :-)
> >
> > > And since we _must_ send an IPI in this case, because the monitor
> > > is busted, we cannot set this.
> > >
> > > > I think a better approach would be to still optimistically
> > > > modify the ->flags value _AND_ to also send an IPI, to make
> > > > sure the wakeup is not lost. This means that the woken CPU will
> > > > wake up much faster (no IPI latency).
> > >
> > > This is exactly what is done. See resched_curr()'s use of
> > > set_nr_and_not_polling(). That does:
> > >
> > > if (!(fetch_or(&flags, NEED_RESCHED) & POLLING_NRFLAG))
> > > smp_send_reschedule(cpu);
> > >
> > > So we unconditionally set NEED_RESCHED, if, when we set that,
> > > POLLING was set, we skip the IPI.
> >
> > Ah, indeed, we set NEED_RESCHED in the same memory address that
> > __monitor() is watching so all is good.
> >
> > > So again, since monitor is busted, simply setting NEED_RESCHED
> > > will not wake us, we must send the IPI, this is achieved by not
> > > setting POLLING_NRFLAG.
> >
> > Yeah, so I got the impression that it might be broken in only
> > certain circumstances, or is it completely busted?
> >
> That is right, monitor is only partially broken not completely
> busted. I don't have the statistics but it is not rare to miss
> wakeup. The typical symptom is random slowness and fails to boot,
> without this patch.
>
> So doing both can speed up wake up in some cases.
>
> Jacob
> > Thanks,
> >
> > Ingo
>
BTW, I just rebased and sent out v2 to avoid conflict with the commit
below. Could you take this patch for v4.8? it is a critical fix for
affected CPUs.

commit 8709ed4d4b0eab04561c1ec9e6ea50fd1e3897ff
Author: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
Date: Fri Jun 17 17:15:03 2016 -0700

x86/cpu: Fix duplicated X86_BUG(9) macro



Thanks,

Jacob