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

[Qi Zheng <zhengqi.arch@bytedance.com>]

Hi David,

On 2024/7/27 05:48, 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.

Yes.

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

For pmd lock, I think this is needed to clear the pmd entry
(pmdp_collapse_flush()). For pte lock, there should be the following two
reasons:

1. release it after clearing pmd entry, then we can capture the changed
    pmd in pte_offset_map_lock() etc after holding this pte lock.
    (This is also what I did in my patchset)

2. As mentioned in the comments, we may be concurrent with
    userfaultfd_ioctl(), but we do not hold the read lock of mmap (or
    read lock of vma), so the VM_UFFD_WP may be set. Therefore, we need
    to hold the pte lock to check whether a new pte entry has been
    inserted.
    (See commit[1] for more details)

[1]. 
https://github.com/torvalds/linux/commit/a98460494b16db9c377e55bc13e5407a0eb79fe8

Thanks,
Qi

>