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

[David Hildenbrand <david@redhat.com>]

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.

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.


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 :/.

-- 
Cheers,

David / dhildenb