Re: [PATCH] mm/x86/pat: Only untrack the pfn range if unmap region

[David Hildenbrand <david@redhat.com>]

[catching up on mails]

>> indicates that file truncation seems to end up messing with a PFNMAP mapping
>> that has PAT set. That is ... weird. I would have thought that PFNMAP would
>> never really happen with file truncation.
>>
>> Does this only happen with an OOT driver, that seems to do weird truncate
>> stuff on files that have a PFNMAP mapping?
>>
>> [1]
>> https://lore.kernel.org/all/3879ee72-84de-4d2a-93a8-c0b3dc3f0a4c@redhat.com/
> 
> Ohhh.. I guess this will also stop working in VFIO, but I think it's fine
> for now because as Yan pointed out VFIO PCI doesn't register those regions
> now so VM_PAT is not yet set..

Interesting, I was assuming that VFIO might be relying on that.

> 
> And one thing I said wrong in the previous reply to Yan is, obviously
> memtype_check_insert() can work with >1 owners as long as the memtype
> matches.. and that's how fork() works where VM_PAT needs to be duplicated.
> But this whole thing is a bit confusing to me..  As I think it also means
> when fork the track_pfn_copy() will call memtype_kernel_map_sync one more
> time even if we're 100% sure the pgprot will be the same for the kernel
> mappings..

I consider the VM_PAT code quite ugly and I wish we could just get rid 
of it (especially, the automatic "entire VMA covered" handling thingy).

> 
> I wonder whether there's some way that untrack pfn framework doesn't need
> to rely on the pgtable to fetch the pfn, because VFIO MMIO region
> protection will also do that in the near future, AFAICT.  The pgprot part
> should be easy there to fetch: get_pat_info() should fallback to vma's
> pgprot if no mapping found; the only outlier should be CoW pages in
> reality.  The pfn is the real issue so far, so that either track_pfn_copy()
> or untrack_pfn() may need to know the pfn to untrack, even if it only has
> the vma information.

I had a prototype to store that information per VMA to avoid the page 
table lookup. VMA splitting was a bit "added complication", but I got it 
to work. (maybe I can still find it if there is demand)

The downside was having to consume more memory for all VMAs in the 
system simply (even if only 8 byte) because a handful of VMAs in the 
system could be VM_PAT. I decided that's not what we want. I managed to 
not consume memory in some configurations, but not in all, so I 
discarded that approach.

I did not explore storing that information in some auxiliary datastructure.

IMHO the whole VM_PAT model is weird:

1) mmap()
2) remap_pfn_range(): if it covers the whole VMA apply some magic
    reservation.
3) munmap(): we unmap *all* PFNs and, therefore, clean up VM_PAT

(VMA splitting make the whole model weirder, but it works, because we 
never merge these VMAs)

This model cannot properly work if we get partial page table zapping via 
truncation/MADV_DONTNEED or similar things after 2). And likely we also 
shouldn't be doing it that way. We should forbid any partial unmappings 
in that model, just like we already disallow MADV_DONTNEED as you note.

As you mention in your other comment, maybe relevant/all? caller should 
just manage the PAT side independently. So maybe we can move to a 
different model.


-- 
Cheers,

David / dhildenb