On Wed, Nov 10, 2021 at 02:25:50PM +0100, David Hildenbrand wrote: > On 10.11.21 13:56, Jason Gunthorpe wrote: > > On Wed, Nov 10, 2021 at 06:54:13PM +0800, Qi Zheng wrote: > > > >> In this patch series, we add a pte_refcount field to the struct page of page > >> table to track how many users of PTE page table. Similar to the mechanism of > >> page refcount, the user of PTE page table should hold a refcount to it before > >> accessing. The PTE page table page will be freed when the last refcount is > >> dropped. > > > > So, this approach basically adds two atomics on every PTE map > > > > If I have it right the reason that zap cannot clean the PTEs today is > > because zap cannot obtain the mmap lock due to a lock ordering issue > > with the inode lock vs mmap lock. > > There are different ways to zap: madvise(DONTNEED) vs > fallocate(PUNCH_HOLE). It depends on "from where" we're actually > comming: a process page table walker or the rmap. AFAIK rmap is the same issue, it can't lock the mmap_sem > The way locking currently works doesn't allow to remove a page table > just by holding the mmap lock, not even in write mode. I'm not sure I understand this. If the goal is to free the PTE tables then the main concern is use-after free on page table walkers (which this series is addressing). Ignoring bugs, we have only three ways to read the page table: - Fully locked. Under the PTLs (gup slow is an example) - Semi-locked. Under the read mmap lock and no PTLs (hmm is an example) - hw-locked. Barriered with TLB flush (gup fast is an example) #1 should be completely safe as the PTLs will protect eveything #2 is safe so long as the write side is held during any layout changes #3 interacts with the TLB flush, and is also safe with zap rmap itself is a #1 page table walker, ie it gets the PTLs under page_vma_mapped_walk(). The sin we have comitted here is that both the mmap lock and the PTLs are being used to protect the page table itself with a very complicated dual semantic. Splitting the sleeping mmap lock into 'covers vma' and 'covers page tables' lets us solve the lock ordering and semi-locked can become more fully locked by the new lock, instead of by abusing mmap sem. I'd suggest to make this new lock a special rwsem which allows either concurrent read access OR concurrent PTL access, but not both. This way we don't degrade performance of the split PTLs, *and* when something needs to change the page table structure it has a way to properly exclude all the #2 lockless readers. So evey touch to the page table starts by obtaining this new lock, depending on the access mode to be used. (PTL vs lockless read) We can keep the existing THP logic where a leaf PMD can be transformed to a non-leaf PMD in the semi-locked case, but the case where a non-leaf PMD is transformed to a leaf PMD has to take the lock. Jason
