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

[Peter Xu]

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)?

I wonder an open coded "ptlock_ptr(page_ptdesc(pmd_page(*pmd)))" would be
nicer here, but only if my understanding is correct.

Thanks,

> 
> It's not quite clear if holding the PTE table lock is really required:
> what if someone else obtains the lock just after we unlock it? But we'll
> leave that as is for now, maybe there are good reasons.
> 
> This is a preparation for adapting hugetlb page table locking logic to
> take the same locks as core-mm page table walkers would.
> 
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> ---
>  include/linux/mm.h   |  7 ++++---
>  mm/khugepaged.c      | 21 +++++++++++++++------
>  mm/pgtable-generic.c |  4 ++--
>  3 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 2c6ccf088c7be..0472a5090b180 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2873,9 +2873,10 @@ static inline spinlock_t *ptlock_ptr(struct ptdesc *ptdesc)
>  }
>  #endif /* ALLOC_SPLIT_PTLOCKS */
>  
> -static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
> +static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
>  {
> -	return ptlock_ptr(page_ptdesc(pmd_page(*pmd)));
> +	/* PTE page tables don't currently exceed a single page. */
> +	return ptlock_ptr(virt_to_ptdesc(pte));
>  }
>  
>  static inline bool ptlock_init(struct ptdesc *ptdesc)
> @@ -2898,7 +2899,7 @@ static inline bool ptlock_init(struct ptdesc *ptdesc)
>  /*
>   * We use mm->page_table_lock to guard all pagetable pages of the mm.
>   */
> -static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
> +static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pte_t *pte)
>  {
>  	return &mm->page_table_lock;
>  }
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index cdd1d8655a76b..f3b3db1046155 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1697,12 +1697,13 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  	i_mmap_lock_read(mapping);
>  	vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
>  		struct mmu_notifier_range range;
> +		bool retracted = false;
>  		struct mm_struct *mm;
>  		unsigned long addr;
>  		pmd_t *pmd, pgt_pmd;
>  		spinlock_t *pml;
>  		spinlock_t *ptl;
> -		bool skipped_uffd = false;
> +		pte_t *pte;
>  
>  		/*
>  		 * Check vma->anon_vma to exclude MAP_PRIVATE mappings that
> @@ -1739,9 +1740,17 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  		mmu_notifier_invalidate_range_start(&range);
>  
>  		pml = pmd_lock(mm, pmd);
> -		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);
>  
>  		/*
>  		 * Huge page lock is still held, so normally the page table
> @@ -1752,20 +1761,20 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  		 * repeating the anon_vma check protects from one category,
>  		 * and repeating the userfaultfd_wp() check from another.
>  		 */
> -		if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) {
> -			skipped_uffd = true;
> -		} else {
> +		if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
>  			pgt_pmd = pmdp_collapse_flush(vma, addr, pmd);
>  			pmdp_get_lockless_sync();
> +			retracted = true;
>  		}
>  
>  		if (ptl != pml)
>  			spin_unlock(ptl);
> +unlock_pmd:
>  		spin_unlock(pml);
>  
>  		mmu_notifier_invalidate_range_end(&range);
>  
> -		if (!skipped_uffd) {
> +		if (retracted) {
>  			mm_dec_nr_ptes(mm);
>  			page_table_check_pte_clear_range(mm, addr, pgt_pmd);
>  			pte_free_defer(mm, pmd_pgtable(pgt_pmd));
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index a78a4adf711ac..13a7705df3f87 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -313,7 +313,7 @@ pte_t *pte_offset_map_nolock(struct mm_struct *mm, pmd_t *pmd,
>  
>  	pte = __pte_offset_map(pmd, addr, &pmdval);
>  	if (likely(pte))
> -		*ptlp = pte_lockptr(mm, &pmdval);
> +		*ptlp = pte_lockptr(mm, pte);
>  	return pte;
>  }
>  
> @@ -371,7 +371,7 @@ pte_t *__pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd,
>  	pte = __pte_offset_map(pmd, addr, &pmdval);
>  	if (unlikely(!pte))
>  		return pte;
> -	ptl = pte_lockptr(mm, &pmdval);
> +	ptl = pte_lockptr(mm, pte);
>  	spin_lock(ptl);
>  	if (likely(pmd_same(pmdval, pmdp_get_lockless(pmd)))) {
>  		*ptlp = ptl;
> -- 
> 2.45.2
> 

-- 
Peter Xu