On Mon, Jul 29, 2024 at 12:26:00PM -0400, Peter Xu wrote: > On Fri, Jul 26, 2024 at 11:48:01PM +0200, David Hildenbrand wrote: > > On 26.07.24 23:28, Peter Xu wrote: > > > On Fri, Jul 26, 2024 at 06:02:17PM +0200, David Hildenbrand wrote: > > > > On 26.07.24 17:36, Peter Xu wrote: > > > > > On Thu, Jul 25, 2024 at 08:39:54PM +0200, David Hildenbrand wrote: > > > > > > pte_lockptr() is the only *_lockptr() function that doesn't consume > > > > > > what would be expected: it consumes a pmd_t pointer instead of a pte_t > > > > > > pointer. > > > > > > > > > > > > Let's change that. The two callers in pgtable-generic.c are easily > > > > > > adjusted. Adjust khugepaged.c:retract_page_tables() to simply do a > > > > > > pte_offset_map_nolock() to obtain the lock, even though we won't actually > > > > > > be traversing the page table. > > > > > > > > > > > > This makes the code more similar to the other variants and avoids other > > > > > > hacks to make the new pte_lockptr() version happy. pte_lockptr() users > > > > > > reside now only in pgtable-generic.c. > > > > > > > > > > > > Maybe, using pte_offset_map_nolock() is the right thing to do because > > > > > > the PTE table could have been removed in the meantime? At least it sounds > > > > > > more future proof if we ever have other means of page table reclaim. > > > > > > > > > > I think it can't change, because anyone who wants to race against this > > > > > should try to take the pmd lock first (which was held already)? > > > > > > > > That doesn't explain why it is safe for us to assume that after we took the > > > > PMD lock that the PMD actually still points at a completely empty page > > > > table. Likely it currently works by accident, because we only have a single > > > > such user that makes this assumption. It might certainly be a different once > > > > we asynchronously reclaim page tables. > > > > > > I think it's safe because find_pmd_or_thp_or_none() returned SUCCEED, and > > > we're holding i_mmap lock for read. I don't see any way that this pmd can > > > become a non-pgtable-page. > > > > > > I meant, AFAIU tearing down pgtable in whatever sane way will need to at > > > least take both mmap write lock and i_mmap write lock (in this case, a file > > > mapping), no? > > > > Skimming over [1] where I still owe a review I think we can now do it now > > purely under the read locks, with the PMD lock held. > > Err, how I missed that.. yeah you're definitely right, and that's the > context here where we're collapsing. I think I somehow forgot all Hugh's > work when I replied there, sorry. > > > > > I think this is also what collapse_pte_mapped_thp() ends up doing: replace a > > PTE table that maps a folio by a PMD (present or none, depends) that maps a > > folio only while holding the mmap lock in read mode. Of course, here the > > table is not empty but we need similar ways of making PT walkers aware of > > concurrent page table retraction. > > > > IIRC, that was the magic added to __pte_offset_map(), such that > > pte_offset_map_nolock/pte_offset_map_lock can fail on races. > > Said that, I still think current code (before this patch) is safe, same to > a hard-coded line to lock the pte pgtable lock. Again, I'm fine if you > prefer pte_offset_map_nolock() but I just think the rcu read lock and stuff > can be avoided. > > I think it's because such collapse so far can only happen in such path > where we need to hold the large folio (PMD-level) lock first. It means > anyone who could change this pmd entry to be not a pte pgtable is blocked > already, hence it must keeping being a pte pgtable page even if we don't > take any rcu. > > > > > > > But if we hold the PMD lock, nothing should actually change (so far my > > understanding) -- we cannot suddenly rip out a page table. > > > > [1] > > https://lkml.kernel.org/r/cover.1719570849.git.zhengqi.arch@xxxxxxxxxxxxx > > > > > > > > > > > > > But yes, the PMD cannot get modified while we hold the PMD lock, otherwise > > > > we'd be in trouble > > > > > > > > > > > > > > I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be > > > > > nicer here, but only if my understanding is correct. > > > > > > > > I really don't like open-coding that. Fortunately we were able to limit the > > > > use of ptlock_ptr to a single user outside of arch/x86/xen/mmu_pv.c so far. > > > > > > I'm fine if you prefer like that; I don't see it a huge deal to me. > > > > Let's keep it like that, unless we can come up with something neater. At > > least it makes the code also more consistent with similar code in that file > > and the overhead should be minimal. > > > > I was briefly thinking about actually testing if the PT is full of > > pte_none(), either as a debugging check or to also handle what is currently > > handled via: > > > > if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) { > > > > Seems wasteful just because some part of a VMA might have a private page > > mapped / uffd-wp active to let all other parts suffer. > > > > Will think about if that is really worth it. > > > > ... also because I still want to understand why the PTL of the PMD table is > > required at all. What if we lock it first and somebody else wants to lock it > > after us while we already ripped it out? Sure there must be some reason for > > the lock, I just don't understand it yet :/. > > IIUC the pte pgtable lock will be needed for checking anon_vma safely. > > e.g., consider if we don't take the pte pgtable lock, I think it's possible > some thread tries to inject a private pte (and prepare anon_vma before > that) concurrently with this thread trying to collapse the pgtable into a > huge pmd. I mean, when without the pte pgtable lock held, I think it's > racy to check this line: > > if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) { > ... > } > > On the 1st condition. Hmm, right after I replied, I think it also guarantees safety on the 2nd condition.. Note that one thing I still prefer a hard-coded line over pte_offset_map_nolock() is that, the new code seems to say we can treat the pte pgtable page differently from the pte pgtable lock. But I think they're really in the same realm. In short, AFAIU the rcu lock not only protects the pte pgtable's existance, but also protects the pte lock.
