Re: CPA boot crash (was: [PATCH] [0/36] Great change_page_attr patch series v3)

[Andi Kleen <andi@firstfloor.org>]

Ingo Molnar <mingo@elte.hu> writes:
> 
> that is (yet another) major misconception on your part. "Drivers" are an 
> easy to blame target (i guess because there's no one out there to defend 
> a vague "drivers" accusation), and they are not the problem here _at 
> all_.

In this case the problem is that drivers ask for different caching mode
on overlapping IO mappings. That only gets resolved by the underlying
MTRRs, but PAT aware code has to fail.

> 
> Drivers tell the architecture code which physical pages they'd like to 
> have access to (or which page range they'd like to see different cache 
> attributes on) and that's it. They are plain users of the ioremap() and 
> change_page_attr() APIs. Nothing more, nothing less.

That's true, but the problem is that they give different conflicting 
requests (cached and uncached).

 
> It is the utmost duty of architecture code to make those APIs 
> fool-proof.

Well if they ask for conflicting requirements what can you do? 

> Hardware _will_ mess up the physical parameters that get 
> passed in every possible way

Does it?

 - and drivers just try to use what the 
> hardware tells them to use. So robustness is key and there's just no 
> "driver reason" why these APIs cannot be robust.

ioremap as an ABI is robust I believe, but still it has some basic 
requirements like nobody passing in conflicting requests.

Are you saying the underlying ioremap() interface should silently
change the caching mode if the driver passes in a conflicting one?

I have my doubts that would be a good strategy. Better probably 
to fail and complain and fix the drivers after some code review.

> oh, and due to that i'll probably revert these two patches of yours:
> 
>   Subject: x86: c_p_a(), change kernel_map_pages to not use c_p_a()
>   Subject: x86: c_p_a(), change 32-bit back to init_mm semaphore locking
> 
> as with these changes you've removed _the_ most important stress-tester 
> for the c_p_a() code: DEBUG_PAGEALLOC.

Point of that being? I added a separate stress tester instead
that actually tests far more cases and actually verifies that it 
works.

My take is rather that with my changes DEBUG_PAGEALLOC is significantly
cheaper than it was before (e.g. it runs lockless now) so actually
more people can use it for debugging.

And after all we have millions of lines of code who can benefit
from DEBUG_PAGEALLOC and will benefit from it being cheaper, while cpa 
is just a few hundred lines of code that we will hopefully eventually
get right anyways.

-Andi