Re: Uncool feature for TTM introduced by x86, pat: Use page flags to track memtypes of RAM pages

[Suresh Siddha <suresh.b.siddha@intel.com>]

On Thu, 2010-02-18 at 08:35 -0800, Jerome Glisse wrote:
> On Thu, Feb 18, 2010 at 04:30:32PM +0100, Jerome Glisse wrote:
> > Hi,
> > 
> > commit id: f58417409603d62f2eb23db4d2cf6853d84a1698
> > 
> > TTM is doing uncommon use of set_memory_wc|uc|wb for instance it's not
> > uncommon for TTM to change memory type from wc to uc or from uc to wc.
> > Since x86, pat: Use page flags to track memtypes of RAM pages (commit
> > id above) this isn't allowed anymore, before going from uc to wc or
> > wc to uc we first have to free the memtype by going through wb this
> > means an extra step which likely lead to some overhead (i guess that
> > uc -> wc or wc -> uc won't trigger massive tlb/cpu flush while
> > uc -> wb -> wc or wc -> wb -> uc will). reserve_ram_pages_type is the
> > function which will check that memory is wb thus enforcing us to go
> > through wb step.
> > 
> > Can we modify the interface to support again changing from uc to wc
> > or wc to uc ? (i can try to do a patch for that).
> 
> Ok, we are likely not hit by wc -> uc or uc -> wc change (which is dumb
> but i haven't yet understand what is exactly happening for the user)
> My guess is that on non PAT get_page_memtype always return -1
> Thus i think we will need to export a function bool pat_is_enabled() so
> ttm can pickup the proper path in the unlikely/broken case of
> wc -> uc.

Sure. We can definitely look into extending PAT API if there is a need.

But it looks like you have a bug in the commit
db78e27de7e29a6db6be7caf607cf803d84094aa causing this performance
regression.

> > If no, we have a sever regression on non PAT arch :
> > http://bugzilla.kernel.org/show_bug.cgi?id=15328

This is the commit db78e27de7e29a6db6be7caf607cf803d84094aa:

-       switch (c_state) {
-       case tt_cached:
-               return set_pages_wb(p, 1);
-       case tt_wc:
-           return set_memory_wc((unsigned long) page_address(p), 1);
-       default:
-               return set_pages_uc(p, 1);
+       if (get_page_memtype(p) != -1) {
+               /* p isn't in the default caching state, set it to
+                * writeback first to free its current memtype. */
+
+               ret = set_pages_wb(p, 1);
+               if (ret)
+                       return ret;
        }
+
+       if (c_state == tt_wc)
+               ret = set_memory_wc((unsigned long) page_address(p), 1);
+       else if (c_state == tt_uncached)
+               ret = set_pages_uc(p, 1);
+
+       return ret;

You are not setting the pages back to wb() before you free it. And this
is the reason for your performance issue.

We can fix it by changing the if check to something like:

if (get_page_memtype(p) != -1 || c_state == tt_cached)

thanks,
suresh