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.028 / U:29.480 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site