Re: [PATCH] x86: Add an explicit barrier() to clflushopt()

[Chris Wilson]

On Thu, Jan 07, 2016 at 09:55:51AM -0800, Andy Lutomirski wrote:
> On Thu, Jan 7, 2016 at 2:16 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> >> +     /* GCC (4.9.1 and 5.2.1 at least) appears to be very confused when
> >> +      * meeting this alternative() and demonstrably miscompiles loops
> >> +      * iterating over clflushopts.
> >> +      */
> >> +     barrier();
> >>  }
> >
> > Or an alternative:
> >
> > +#define alternative_output(oldinstr, newinstr, feature, output)        \
> > +       asm volatile (ALTERNATIVE(oldinstr, newinstr, feature)          \
> > +               : output : "i" (0) : "memory")
> >
> > I would really appreciate some knowledgeable folks taking a look at the
> > asm for clflushopt() as it still affects today's kernel and gcc.
> >
> > Fwiw, I have confirmed that arch/x86/mm/pageattr.c clflush_cache_range()
> > is similarly affected.
> 
> Unless I'm mis-reading the asm, clflush_cache_range() is compiled
> correctly for me.  (I don't know what the %P is for in the asm, but
> that shouldn't matter.)  The ALTERNATIVE shouldn't even be visible to
> the optimizer.
> 
> Can you attach a bad .s file and let us know what gcc version this is?
>  (You can usually do 'make foo/bar/baz.s' to get a .s file.)  I'd also
> be curious whether changing clflushopt to clwb works around the issue.

Now I feel silly. Looking at the .s, there is no difference with the
addition of the barrier to clflush_cache_range(). And sure enough
letting the test run for longer, we see a failure. I fell for a placebo.

The failing assertion is always on the last cacheline and is always one
value behind. Oh well, back to wondering where we miss the flush.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre