[Intel-gfx] [Linaro-mm-sig] [PATCH 1/2] dma-buf: Require VM_PFNMAP vma for mmap

[Thomas Hellström (Intel)]

Hi,

On 3/1/21 9:28 AM, Daniel Vetter wrote:
> On Sat, Feb 27, 2021 at 9:06 AM Thomas Hellström (Intel)
> <thomas_os at shipmail.org> wrote:
>> On 2/26/21 2:28 PM, Daniel Vetter wrote:
>>> So I think it stops gup. But I haven't verified at all. Would be good
>>> if Christian can check this with some direct io to a buffer in system
>>> memory.
>> Hmm,
>>
>> Docs (again vm_normal_page() say)
>>
>>    * VM_MIXEDMAP mappings can likewise contain memory with or without "struct
>>    * page" backing, however the difference is that _all_ pages with a struct
>>    * page (that is, those where pfn_valid is true) are refcounted and
>> considered
>>    * normal pages by the VM. The disadvantage is that pages are refcounted
>>    * (which can be slower and simply not an option for some PFNMAP
>> users). The
>>    * advantage is that we don't have to follow the strict linearity rule of
>>    * PFNMAP mappings in order to support COWable mappings.
>>
>> but it's true __vm_insert_mixed() ends up in the insert_pfn() path, so
>> the above isn't really true, which makes me wonder if and in that case
>> why there could any longer ever be a significant performance difference
>> between MIXEDMAP and PFNMAP.
> Yeah it's definitely confusing. I guess I'll hack up a patch and see
> what sticks.
>
>> BTW regarding the TTM hugeptes, I don't think we ever landed that devmap
>> hack, so they are (for the non-gup case) relying on
>> vma_is_special_huge(). For the gup case, I think the bug is still there.
> Maybe there's another devmap hack, but the ttm_vm_insert functions do
> use PFN_DEV and all that. And I think that stops gup_fast from trying
> to find the underlying page.
> -Daniel

Hmm perhaps it might, but I don't think so. The fix I tried out was to set

PFN_DEV | PFN_MAP for huge PTEs which causes pfn_devmap() to be true, 
and then

follow_devmap_pmd()->get_dev_pagemap() which returns NULL and gup_fast() 
backs off,

in the end that would mean setting in stone that "if there is a huge 
devmap page table entry for which we haven't registered any devmap 
struct pages (get_dev_pagemap returns NULL), we should treat that as a 
"special" huge page table entry".

 From what I can tell, all code calling get_dev_pagemap() already does 
that, it's just a question of getting it accepted and formalizing it.

/Thomas