Re: CPA patchset

[Andi Kleen <ak@suse.de>]

On Thu, Jan 10, 2008 at 01:22:04PM +0100, Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > > What is very real though are the hard limitations of MTRRs. So i'd 
> > > rather first like to see a clean PAT approach (which all other 
> > > modern OSs have already migrated to in the past 10 years)
> > 
> > That's mostly orthogonal. Don't know why you bring it up now?
> 
> because the PAT (Page Attribute Table support) patchset and the CPA 
> (change_page_attr()) patchset are are not orthogonal at all - as their 
> name already signals: because they change the implementation/effects of 
> the same interface(s). [just at different levels].
> 
> Both patchsets change how the kernel pagetable caching is handled. PAT 
> changes the kernel pte details and unshackles us from MTRR reliance and 
> thus solves real problems on real boxes:
> 
>  55 files changed, 1288 insertions(+), 237 deletions(-)
> 
> CPA moves change_page_attr() from invwb flushing to cflush flushing, for 
> a speedup/latency-win, plus a whole bunch of intermingled fixes and 
> improvements to page attribute modification:
> 
>  26 files changed, 882 insertions(+), 423 deletions(-)
> 
> so in terms of risk management, the "perfect patch order" is:
> 
>  - minimal_set of correctness fixes to the highlevel cpa code.

The two clear bug fix patches are refcount and flush order.

refcnt could be moved earlier; flush order would be quite painful
because there are quite a lot of patches dependent on it.

I could move ref count earlier, but I would prefer not to because
of the significant work it would be for me. 

Since it is all already bisectable I'm also not sure what debugging
advantages the reordering would be. 

It's already bisectable (I have not booted all immediate steps,
but several of them and I believe all compile) and in small pieces for that.

If it's really broken it would need to be reverted and then the ref count 
stuff would go too.  But I hope that won't be needed. 

And even losing the reference count fixes wouldn't be catastrophic -- in the
worst case you lose some minor performance because kernel mappings are 
unnecessarily split to 4K pages, but it's not a correctness fix. 

So while the reordering would be possible, it would imho not
bring very much advantages and I must admit I'm not too motivated
to do another time-consuming reordering for a relatively weak reason.  

If it was a 5 patch series I would probably not complain too much
about this, but it's 25+ patches.

>  - ( then any provably NOP cleanups to pave the way. )
> 
>  - then change the lowlevel pte code (PAT) to reduce/eliminate the need 
>    to have runtime MTRR use

That's a completely different area really. Most of the PAT code
has nothing to do with clear_page_attr(). Please think that through
again after reading both patchkits.

As far as I can see making it wait for PAT would mean delaying
it for a longer time which would be a pity.


>  - then structural improvements/cleanups of the highlevel cpa code
> 
>  - then the cflush (optional) performance feature ontop of it.

There are actual a lot more performance features in there (self snoop,
minimal TLB flushing some other stuff). Most of it is related to 
that in fact.
 
> 
>  - then gigabyte-largepages/TLBs support [new CPU feature that further
>    complicates page-attribute management]

That's already at the end.

> 
> All in an easy-to-revert fashion. We _will_ regress here, and this stuff 

That's already the case.

-Andi