LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andi Kleen <ak@suse.de>
To: Arjan van de Ven <arjan@infradead.org>
Cc: "Siddha, Suresh B" <suresh.b.siddha@intel.com>,
	tglx@linutronix.de, mingo@elte.hu, linux-kernel@vger.kernel.org,
	venkatesh.pallipadi@intel.com
Subject: Re: [PATCH] [7/8] CPA: Don't flush caches on CPUs that support self-snoop
Date: Tue, 12 Feb 2008 09:45:17 +0100	[thread overview]
Message-ID: <47B15C9D.6090400@suse.de> (raw)
In-Reply-To: <20080211123659.421bfaee@laptopd505.fenrus.org>

Arjan van de Ven wrote:
> On Mon, 11 Feb 2008 11:47:12 -0800
> "Siddha, Suresh B" <suresh.b.siddha@intel.com> wrote:
>
>   
>> On Mon, Feb 11, 2008 at 06:58:46PM +0100, Andi Kleen wrote:
>>     
>>> On Monday 11 February 2008 18:36:06 Siddha, Suresh B wrote:
>>>       
>>>> On Mon, Feb 11, 2008 at 04:27:23PM +0100, Andi Kleen wrote:
>>>>         
>>>>>>> That is exactly the situation in pageattr.c. You're saying
>>>>>>> the manual is wrong here?
>>>>>>>               
>>>>>> I'm saying that we are not following step 2 (marking the
>>>>>> pages not present) 
>>>>>>             
>>>>> Yes that's true. It's one of the design problems of the intent
>>>>> API that makes fixing this hard unfortunately.
>>>>>           
>>>> BTW, making pages not present is required only while changing the
>>>> attribute from WB to WC or WC to WB. I think this step is for
>>>> avoiding attribute aliasing for speculative accesses. For UC, we
>>>> don't have speculative accesses and such we probably don't need
>>>> it.
>>>>         
>>> Ok that would imply that my patch is ok for all current in tree
>>> users at least (none of them ever set WC currently, only UC). It
>>> might not be safe for the out of tree WC users though.
>>>
>>> So it should be ok to apply according to standard policy :)
>>>       
>> There is atleast one issue though. For an I/O device which is not
>> capable of snooping the caches, if the driver model assumes that
>> ioremap_nocache() will flush the cpu caches(before initiating DMA),
>> then the lack of cache flush in cpa() might cause some breakages.
>>     
>
> this is a totally separate issue and I agree, it needs a separate API.
> (well there is one more or less, but lets make it explicit).
>   

I doubt we have any drivers with this problem because ioremap()
for the longest time (until I added change_page_attr() to it the first time
one a few releases ago) didn't flush its caches at all and it still
doesn't flush the caches for the common case of the ioremapped area
not being part of the direct mapping (standard case on 32bit and
not uncommon on 64bit even)
>   
>>>> So WC support through PAT should take care of it.
>>>>         
>>> You mean the PAT patch should do something about it? Yes probably,
>>> but what?
>>>       
>> marking the pages not present etc..
>>     
>
> another option is to do a three-step tango; go from WB to UC, flush, then go from UC to WB.
>
> We need to deal with this for correctness anyway (since this could happen naturally) so
> might as well do things that way.
>   
Please clarify what you mean. For what correctness aspect and in
what circumstances would such a three step tango be needed?

I don't see any mention of such a requirement in the manual.

-Andi


  reply	other threads:[~2008-02-12  8:44 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-11  9:50 [PATCH] [0/8] Misc CPA patchkit Andi Kleen
2008-02-11  9:50 ` [PATCH] [1/8] CPA: Remove my copyright notice Andi Kleen
2008-02-12  0:42   ` Thomas Gleixner
2008-02-11  9:50 ` [PATCH] [2/8] CPA: Remove inline from static_protections Andi Kleen
2008-02-11 14:04   ` Ingo Molnar
2008-02-11  9:50 ` [PATCH] [3/8] CPA: Fix some incorrect comments in the debug pagealloc code Andi Kleen
2008-02-11  9:50 ` [PATCH] [4/8] CPA: Ifdef the cpa pool for DEBUG_PAGEALLOC Andi Kleen
2008-02-11  9:50 ` [PATCH] [5/8] CPA: Move pool allocation/free into separate functions Andi Kleen
2008-02-11  9:50 ` [PATCH] [6/8] CPA: Remove BUG_ON for LRU/Compound pages Andi Kleen
2008-02-17 14:07   ` Thomas Gleixner
2008-02-11  9:50 ` [PATCH] [7/8] CPA: Don't flush caches on CPUs that support self-snoop Andi Kleen
2008-02-11 15:04   ` Arjan van de Ven
2008-02-11 15:12     ` Andi Kleen
2008-02-11 15:21       ` Arjan van de Ven
2008-02-11 15:27         ` Andi Kleen
2008-02-11 17:36           ` Siddha, Suresh B
2008-02-11 17:58             ` Andi Kleen
2008-02-11 19:47               ` Siddha, Suresh B
2008-02-11 20:36                 ` Arjan van de Ven
2008-02-12  8:45                   ` Andi Kleen [this message]
2008-02-11  9:50 ` [PATCH] [8/8] CPA: Add statistics about state of direct mapping Andi Kleen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=47B15C9D.6090400@suse.de \
    --to=ak@suse.de \
    --cc=arjan@infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=suresh.b.siddha@intel.com \
    --cc=tglx@linutronix.de \
    --cc=venkatesh.pallipadi@intel.com \
    --subject='Re: [PATCH] [7/8] CPA: Don'\''t flush caches on CPUs that support self-snoop' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).