Re: [tip:x86/idle] x86, idle: Use static_cpu_has() for CLFLUSH workaround, add barriers

[Ingo Molnar <mingo@kernel.org>]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Dec 20, 2013 at 5:40 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > The first mb() looks superfluous, because see
> > current_set_polling_and_test():
> >
> > static inline bool __must_check current_set_polling_and_test(void)
> > {
> >         __current_set_polling();
> >
> >         /*
> >          * Polling state must be visible before we test NEED_RESCHED,
> >          * paired by resched_task()
> >          */
> >         smp_mb();
> >
> >         return unlikely(tif_need_resched());
> > }
> >
> > So it already has a (MFENCE) barrier after ->flags is modified.
> 
> So what?
> 
> The mb() isn't necessarily against the *write*, it is also against 
> the *read*.
> 
> If the cflush is needed before the monitor, it's likely because the 
> monitor instruction has some bug with already-existing cachelines in 
> shared state or whatever.

So, my thinking was that maybe it's the other way around: the problem 
is with the write not being drained to cache yet.

One possibility would be that the bug is that MONITOR will not see the 
previous write in the store buffer and when shortly afterwards the 
store hits the cache, it falsely 'wakes up' the MWAIT.

(If so then the race window roughly depends on the the number of 
cycles the current_set_polling_and_test() modification retires and 
explains how small reorganizations of the code triggered the hw bug.)

The CLFLUSH ensures that the modification is visible in the cache 
before MONITOR is run.

If this guess is true then the need_resched() read is immaterial to 
the bug. On the flip side, if my guess is wrong then I'm leaving 
another subtle race window in the code...

So yeah I agree that without more information from Intel we are better 
off with a more conservative approach, the bug took relatively long to 
find, Thomas did the original 7d1a941731fab back in March, 3 kernel 
releases ago ...

If only there were Intel employees on Cc: who could tell us more about 
the background of the bug ;-)

> Which means that we want to make sure that the cflush is ordered wrt 
> *reads* from that cacheline too.
> 
> cflush has nothing specifically to do with writes.

Yes.

Thanks,

	Ingo