Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1303455 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-07 11:20 +0100
    Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Andy Lutomirski <luto@amacapital.net> - 2016-01-07 19:00 +0100
      Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-07 20:50 +0100
        Re: [PATCH] x86: Add an explicit barrier() to clflushopt() "H. Peter Anvin" <hpa@zytor.com> - 2016-01-07 22:10 +0100
          Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-07 23:00 +0100
            Re: [PATCH] x86: Add an explicit barrier() to clflushopt() "H. Peter Anvin" <hpa@zytor.com> - 2016-01-07 23:40 +0100
              Re: [PATCH] x86: Add an explicit barrier() to clflushopt() "H. Peter Anvin" <hpa@zytor.com> - 2016-01-07 23:40 +0100
                Re: [PATCH] x86: Add an explicit barrier() to clflushopt() "H. Peter Anvin" <hpa@zytor.com> - 2016-01-09 07:00 +0100
                Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-09 09:10 +0100
                  Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Andy Lutomirski <luto@amacapital.net> - 2016-01-09 23:40 +0100
                    Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-11 12:30 +0100
                      Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-11 21:20 +0100
                        Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-11 22:10 +0100
                          Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-12 17:40 +0100
                            Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-12 18:10 +0100
                              Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-12 22:20 +0100
                                Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-12 23:10 +0100
                                  Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-13 02:00 +0100
                                    Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-13 03:10 +0100
                                      Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Andy Lutomirski <luto@amacapital.net> - 2016-01-13 03:50 +0100
                                        Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-13 05:40 +0100
                                      Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Chris Wilson <chris@chris-wilson.co.uk> - 2016-01-13 13:40 +0100
                                        Re: [PATCH] x86: Add an explicit barrier() to clflushopt() Linus Torvalds <torvalds@linux-foundation.org> - 2016-01-13 19:50 +0100
                      Re: [PATCH] x86: Add an explicit barrier() to clflushopt() "H. Peter Anvin" <hpa@zytor.com> - 2016-01-12 18:20 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1308038

On Tue, Jan 12, 2016 at 6:42 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Since barriers are on my mind: how strong a barrier is needed to
> prevent cache fills from being speculated across the barrier?

I don't think there are *any* architectural guarantees.

I suspect that a real serializing instruction should do it. But I
don't think even that is guaranteed.

Non-coherent IO is crazy. I really thought Intel had learnt their
lesson, and finally made all the GPU's coherent. I'm afraid to even
ask why Chris is actually working on some sh*t that requires clflush.

In general, you should probably do something nasty like

 - flush before starting IO that generates data (to make sure you have
no dirty cachelines that will write back and mess up)

 - start the IO, wait for it to complete

 - flush after finishing IO that generates the data (to make sure you
have no speculative clean cachelines with stale data)

 - read the data now.

Of course, what people actually end up doing to avoid all this is to
mark the memory noncacheable.

And finally, the *correct* thing is to not have crap hardware, and
have IO be cache coherent. Things that don't do that are shit. Really.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1308365

On Tue, Jan 12, 2016 at 06:06:34PM -0800, Linus Torvalds wrote:
> On Tue, Jan 12, 2016 at 4:55 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > The double clflush() remains a mystery.
> 
> Actually, I think it's explainable.
> 
> It's wrong to do the clflush *after* the GPU has done the write, which
> seems to be what you are doing.
> 
> Why?
> 
> If the GPU really isn't cache coherent, what can happen is:
> 
>  - the CPU has the line cached
> 
>  - the GPU writes the data
> 
>  - you do the clflushopt to invalidate the cacheline
> 
>  - you expect to see the GPU data.
> 
> Right?
> 
> Wrong. The above is complete crap.
> 
> Why?
> 
> Very simple reason: the CPU may have had the cacheline dirty at some
> level in its caches, so when you did the clflushopt, it didn't just
> invalidate the CPU cacheline, it wrote it back to memory. And in the
> process over-wrote the data that the GPU had written.

Forgive me for being dense, but if we overwrite the GPU data in the
backing struct page with the cacheline from the CPU, how do we see the
results from the GPU afterwards?

> Now you can say "but the CPU never wrote to the cacheline, so it's not
> dirty in the CPU caches". That may or may not be trie. The CPU may
> have written to it quite a long time ago.

What we do is we clflush the entire object after it is written to by the
CPU (including newly allocated objects from shmemfs, or pages being
returned to us by shmemfs) before any operation on that object by the
GPU. We have to so that the GPU sees the correct page contents.
(If I change the clflush of the written objects to a wbinvd, that's not
sufficient for the tests to pass.)

We do not clflush the object after we read the backing pages on the CPU
before the next GPU operation, even if it is a GPU write. This leaves us
with clean but stale cachelines. Should. That's why we then
clflush_cache_range() prior to the next read on the object, it is
intended to be a pure cache line invalidation.

If we clflush the entire object between every CPU read back and the *next*
GPU operation, it fails. If we clflush the object before every GPU write
to it, it passes. And to refresh, we always clflush after a CPU write.

I am reasonably confident that any cachelines we dirty (or inherited)
are flushed. What you are suggesting is that there are dirty cachelines
regardless. I am also reasonably confident that even if we clflush the
entire object after touching it before the GPU write, and clflush the
individual cachelines again after the GPU write, we see the errors.

I haven't found the hole yet, or been convincingly able to explain the
differences between gen.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[toc] | [prev] | [next] | [standalone]

#1308726

On Wed, Jan 13, 2016 at 4:34 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Forgive me for being dense, but if we overwrite the GPU data in the
> backing struct page with the cacheline from the CPU, how do we see the
> results from the GPU afterwards?

Hmm. Good point.

Ok, all the symptoms just say "writes from GPU are delayed and out of order".

Do you have access to the GPU hardware people?

I thought that all the modern Intel GPU's are cache-coherent. If this
is some castrated chip where coherence is removed (perhaps because it
is not working? perhaps config setting?) maybe it needs some extra
ghardware setting to make the GPU "flush" operation actually do
something. In a cache-coherent model, a flush could/should be a noop,
so maybe the hardware is set for that kind of "flush does nothing"
behavior.

Or maybe the GPU is just a buggy pile of crap.

            Linus

[toc] | [prev] | [next] | [standalone]

#1307644

On January 11, 2016 3:28:01 AM PST, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>On Sat, Jan 09, 2016 at 02:36:03PM -0800, Andy Lutomirski wrote:
>> On Sat, Jan 9, 2016 at 12:01 AM, Chris Wilson
><chris@chris-wilson.co.uk> wrote:
>> > On Thu, Jan 07, 2016 at 02:32:23PM -0800, H. Peter Anvin wrote:
>> >> On 01/07/16 14:29, H. Peter Anvin wrote:
>> >> >
>> >> > I would be very interested in knowing if replacing the final
>clflushopt
>> >> > with a clflush would resolve your problems (in which case the
>last mb()
>> >> > shouldn't be necessary either.)
>> >> >
>> >>
>> >> Nevermind.  CLFLUSH is not ordered with regards to CLFLUSHOPT to
>the
>> >> same cache line.
>> >>
>> >> Could you add a sync_cpu(); call to the end (can replace the final
>mb())
>> >> and see if that helps your case?
>> >
>> > s/sync_cpu()/sync_core()/
>> >
>> > No. I still see failures on Baytrail and Braswell (Pineview is not
>> > affected) with the final mb() replaced with sync_core(). I can
>reproduce
>> > failures on Pineview by tweaking the clflush_cache_range()
>parameters,
>> > so I am fairly confident that it is validating the current code.
>> >
>> > iirc sync_core() is cpuid, a heavy serialising instruction, an
>> > alternative to mfence.  Is there anything that else I can infer
>about
>> > the nature of my bug from this result?
>> 
>> No clue, but I don't know much about the underlying architecture.
>> 
>> Can you try clflush_cache_ranging one cacheline less and then
>manually
>> doing clflushopt; mb on the last cache line, just to make sure that
>> the helper is really doing the right thing?  You could also try
>> clflush instead of clflushopt to see if that makes a difference.
>
>I had looked at increasing the range over which clflush_cache_range()
>runs (using roundup/rounddown by cache lines), but it took something
>like +/- 256 bytes to pass all the tests. And also did
>s/clflushopt/clflush/ to confirm that made no differnce.
>
>Bizarrely,
>
>diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
>index 6000ad7..cf074400 100644
>--- a/arch/x86/mm/pageattr.c
>+++ b/arch/x86/mm/pageattr.c
>@@ -141,6 +141,7 @@ void clflush_cache_range(void *vaddr, unsigned int
>size)
>        for (; p < vend; p += clflush_size)
>                clflushopt(p);
> 
>+       clflushopt(vend-1);
>        mb();
> }
> EXPORT_SYMBOL_GPL(clflush_cache_range);
>
>works like a charm.
>-Chris

That clflushopt touches a cache line already touched and therefore serializes with it.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web