lkml.org 
[lkml]   [2009]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 2.6.29 pat issue
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
>
>



\
 
 \ /
  Last update: 2009-02-06 10:53    [W:0.107 / U:0.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site