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

[David Hildenbrand <david@redhat.com>]

On 29.07.24 18:39, Peter Xu wrote:
> 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.

I see what you mean but this is a very similar pattern as used in 
collapse_pte_mapped_thp(), no? There we have

start_pte = pte_offset_map_nolock(mm, pmd, haddr, &ptl);
...
if (!pml)
	spin_lock(ptl);
...
pte_unmap(start_pte);
if (!pml)
	spin_unlock(ptl);


Again, I don't have a strong opinion on this, but doing it more similar 
to collapse_pte_mapped_thp() to obtain locks makes it clearer to me. But 
if I am missing something obvious please shout and I'll change it.

-- 
Cheers,

David / dhildenb