Re: [PATCH v1 1/2] mm: let pte_lockptr() consume a pte_t pointer

[Peter Xu <peterx@redhat.com>]

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@bytedance.com
> > 
> > > 
> > > > 
> > > > 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.

From that POV, below new code in this patch:

-               ptl = pte_lockptr(mm, pmd);
+
+               /*
+                * No need to check the PTE table content, but we'll grab the
+                * PTE table lock while we zap it.
+                */
+               pte = pte_offset_map_nolock(mm, pmd, addr, &ptl);
+               if (!pte)
+                       goto unlock_pmd;
                if (ptl != pml)
                        spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
+               pte_unmap(pte);

Could be very misleading, where it seems to say that it's fine to use the
pte pgtable lock even after rcu unlock.  It might make the code harder to
understand.

Thanks,

-- 
Peter Xu