Re: 2.6.29 pat issue

["Thomas Hellström" <thellstrom@vmware.com>]

Pallipadi, Venkatesh wrote:
> On Thu, 2009-02-05 at 13:32 -0800, Thomas Hellstrom wrote:
>   
>> Pallipadi, Venkatesh wrote:
>>     
>>> Only place where vm_pgoff is getting set for a PFNMAP vma is in
>>> remap_pfn_range() which maps the entire range. vm_insert_pfn() which may
>>> have sparsely populated ranges does not set vm_pgoff. What interface are
>>> you using to map discontig pages, where you are seeing these errors?
>>>
>>>       
>> Since vm_pgoff can be nonzero upon every call to a device driver's mmap 
>> method (It corresponds to the @offset parameter, page shifted, given by 
>> the user's mmap call), _Any_ VM_PFNMAP vma can practically be assumed to 
>> be linear by is_linear_pfn_mapping(), and that's an invalid assumption.
>>
>> In this particular case, We set VM_PFNMAP explicitly in the mmap method 
>> and use fault() and vm_insert_pfn() to populate the vmas with PTEs 
>> pointing to private memory pages or io-space depending on where the data 
>> is currently located. The member vma->vm_pgoff is, as mentioned, set by 
>> the user-space mmap call, indicating what part of the device address 
>> space needs to be mapped.
>>
>> So in the end, we're hitting the WARN_ON_ONCE(1) near line 637 in 
>> arch/x86/mm/pat.c. We should never have ended up in reserve_pfn_range() 
>> in the first place.
>>
>>     
>
> OK. Now I understand how you are seeing that warning. I am not what is
> the simple way around this. There are no bits available in vm_flags that
> we can use to identify linear_pfn_mapping. I don't think you have any
> way around in the driver other than using pgoff, in order to do
> vm_insert_pfn.
> One possible way is to overload some existing flag + PFNMAP to mean
> linear pfn map. Will send a patch for this as an RFC soon.
>   
Thanks, Venki. There are a couple of other issues as well. This wasn't 
the root cause of the problem, Pls look at the mail I just sent out.

>   
>>> The result of not having the caching attribute right can be really bad
>>> as to hang/crash the system. So, having this only in debug is not the
>>> enough, IM0. Kernel has to enforce UC and WC caching types are
>>> consistent at all times. And we also have to keep the indentity map and
>>> other mappings that may be present for that address consistent.
>>>       
>> Indeed, it's crucial to keep the mappings consistent, but failure to do 
>> so is a kernel driver bug, it should never be the result of invalid user 
>> data.
>>
>> There are other more common kernel bugs that can be even worse and hang 
>> / crash the system. For example using uninitialized spinlocks, writing 
>> to kfreed memory etc. There is code in the kernel to detect these as 
>> well, but this code is behind debug defines.
>>
>> IMHO checking each vm_insert_pfn() for caching attribute correctness is 
>> not something that should be enabled by default, due to the CPU 
>> overhead. Production drivers should never violate this.
>>
>>     
>
> It is not a question of single production driver. There are many
> variables here. Different drivers can be mapping the same region. There
> can be mapping from /dev/mem. There are also kernel identity and text
> mappings. So, any change of cacheability by one driver has to make sure
> it is not stepping over some other users of that pte. Kernel has to make
> sure different things co-exist in a sane way.
>   
Yes, I understand the need for this check now.
> There is an alternative to checking this in each vm_insert_pfn, as long
> as mappings are going to be contiguous (even though they may be inserted
> individually). As in include/linux/io-mapping.h, we can have a
> create_mapping which reserves the entire space, and individual map and
> unmap, which doesn't have to check. May be we need a new API for your
> use case though...
>   
I think when the issues in the previous mail are fixed, this will in the 
end reduce to a possible performance problem when doing vm_insert_pfn() 
into a contigous range. A create_mapping API could be a way around this.

Thanks,
Thomas



> Thanks,
> Venki
>
>