Re: [PATCH v3 4/9] mm/memory: Batch set uffd-wp markers during zapping

[Dev Jain]

On 11/05/26 1:07 pm, David Hildenbrand (Arm) wrote:
> On 5/6/26 11:44, Dev Jain wrote:
>> In preparation for the next patch, enable batch setting of uffd-wp ptes.
>>
>> The code paths passing nr > 1 to zap_install_uffd_wp_if_needed() produce
>> that nr through either folio_pte_batch or swap_pte_batch, guaranteeing that
>> all ptes are the same w.r.t belonging to the same type of VMA (anonymous
>> or non-anonymous, wp-armed or non-wp-armed), and all being marked with
>> uffd-wp or all being not marked.
>>
>> Note that we will have to use set_pte_at() in a loop instead of set_ptes()
>> since the latter cannot handle present->non-present conversion for
>> nr_pages > 1.
>>
>> Convert documentation of install_uffd_wp_ptes_if_needed to kerneldoc
>> format.
> 
> You should mention why the uffd_supports_wp_marker()+vma_is_anonymous() calls in
> zap_install_uffd_wp_if_needed can be dropped.

Okay.

> 
>>
>> No functional change is intended.
>>
>> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
>> ---
>>  include/linux/mm_inline.h | 34 +++++++++++++++++++++-------------
>>  mm/memory.c               | 20 +-------------------
>>  mm/rmap.c                 |  2 +-
>>  3 files changed, 23 insertions(+), 33 deletions(-)
>>
>> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
>> index a171070e15f05..6f7ecede2fb45 100644
>> --- a/include/linux/mm_inline.h
>> +++ b/include/linux/mm_inline.h
>> @@ -566,9 +566,17 @@ static inline pte_marker copy_pte_marker(
>>  	return dstm;
>>  }
>>  
>> -/*
>> - * If this pte is wr-protected by uffd-wp in any form, arm the special pte to
>> - * replace a none pte.  NOTE!  This should only be called when *pte is already
>> +/**
>> + * install_uffd_wp_ptes_if_needed - install uffd-wp marker on PTEs that map
>> + *				    consecutive pages of the same large folio.
>> + * @vma: The VMA the pages are mapped into.
>> + * @addr: Address the first page of this batch is mapped at.
>> + * @ptep: Page table pointer for the first entry of this batch.
>> + * @pteval: old value of the entry pointed to by ptep.
>> + * @nr_ptes: Number of entries to clear (batch size).
>> + *
>> + * If the ptes were wr-protected by uffd-wp in any form, arm special ptes to
>> + * replace none ptes.  NOTE!  This should only be called when *pte is already
>>   * cleared so we will never accidentally replace something valuable.  Meanwhile
>>   * none pte also means we are not demoting the pte so tlb flushed is not needed.
>>   * E.g., when pte cleared the caller should have taken care of the tlb flush.
>> @@ -576,11 +584,11 @@ static inline pte_marker copy_pte_marker(
>>   * Must be called with pgtable lock held so that no thread will see the none
>>   * pte, and if they see it, they'll fault and serialize at the pgtable lock.
>>   *
>> - * Returns true if an uffd-wp pte was installed, false otherwise.
>> + * Returns true if uffd-wp ptes were installed, false otherwise.
>>   */
>>  static inline bool
>> -pte_install_uffd_wp_if_needed(struct vm_area_struct *vma, unsigned long addr,
>> -			      pte_t *pte, pte_t pteval)
>> +install_uffd_wp_ptes_if_needed(struct vm_area_struct *vma, unsigned long addr,
>> +			      pte_t *ptep, pte_t pteval, unsigned long nr_ptes)
> 
> If we conditionally do something, what about while at it shorten it to:
> 
> 	cont_install_uffd_wp_ptes()

Why "cont_"? Did you mean to say "cond" ("conditionally").

> 
> Also, less churn in this patch if you don't change pte->ptep
> 
> (because if you do, you should then also do pteval->pte :) )
> 
> So I'd just leave that as is in this patch.

So I'll change pte->ptep and pteval->pte because I hate pte pointers
called pte : )
> 
>>  {
>>  	bool arm_uffd_pte = false;
>>  
>> @@ -588,7 +596,7 @@ pte_install_uffd_wp_if_needed(struct vm_area_struct *vma, unsigned long addr,
>>  		return false;
>>  
>>  	/* The current status of the pte should be "cleared" before calling */
>> -	WARN_ON_ONCE(!pte_none(ptep_get(pte)));
>> +	WARN_ON_ONCE(!pte_none(ptep_get(ptep)));
>>  
>>  	/*
>>  	 * NOTE: userfaultfd_wp_unpopulated() doesn't need this whole
>> @@ -610,13 +618,13 @@ pte_install_uffd_wp_if_needed(struct vm_area_struct *vma, unsigned long addr,
>>  	if (unlikely(pte_swp_uffd_wp_any(pteval)))
>>  		arm_uffd_pte = true;
>>  
>> -	if (unlikely(arm_uffd_pte)) {
>> -		set_pte_at(vma->vm_mm, addr, pte,
>> -			   make_pte_marker(PTE_MARKER_UFFD_WP));
>> -		return true;
>> -	}
>> +	if (likely(!arm_uffd_pte))
>> +		return false;
>>  
>> -	return false;
>> +	for (int i = 0; i < nr_ptes; ++i, ++ptep, addr += PAGE_SIZE)
>> +		set_pte_at(vma->vm_mm, addr, ptep, make_pte_marker(PTE_MARKER_UFFD_WP));
>> +
>> +	return true;
>>  }
> 
> I wonder whether this growing function is really appropriate to be in the header
> file? Can we just move that whole thing into mm/memory.c?
> 
> It's called on every try_to_unmap_one() invocation, but I doubt we care about a
> function call here.

Makes sense.

> 
>>  
>>  static inline bool vma_has_recency(const struct vm_area_struct *vma)
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 0c9d9c2cbf0e0..f14311c4d2001 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -1610,29 +1610,11 @@ zap_install_uffd_wp_if_needed(struct vm_area_struct *vma,
>>  			      unsigned long addr, pte_t *pte, int nr,
>>  			      struct zap_details *details, pte_t pteval)
>>  {
>> -	bool was_installed = false;
>> -
>> -	if (!uffd_supports_wp_marker())
>> -		return false;
>> -
>> -	/* Zap on anonymous always means dropping everything */
>> -	if (vma_is_anonymous(vma))
>> -		return false;
>> -
>>  	if (zap_drop_markers(details))
>>  		return false;
>>  
>> -	for (;;) {
>> -		/* the PFN in the PTE is irrelevant. */
>> -		if (pte_install_uffd_wp_if_needed(vma, addr, pte, pteval))
>> -			was_installed = true;
>> -		if (--nr == 0)
>> -			break;
>> -		pte++;
>> -		addr += PAGE_SIZE;
>> -	}
>> +	return install_uffd_wp_ptes_if_needed(vma, addr, pte, pteval, nr);
>>  
>> -	return was_installed;
>>  }
>>  
>>  static __always_inline void zap_present_folio_ptes(struct mmu_gather *tlb,
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index bd4e3639e26ed..b17dce752a1ea 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -2266,7 +2266,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>  		 * we may want to replace a none pte with a marker pte if
>>  		 * it's file-backed, so we don't lose the tracking info.
>>  		 */
>> -		pte_install_uffd_wp_if_needed(vma, address, pvmw.pte, pteval);
>> +		install_uffd_wp_ptes_if_needed(vma, address, pvmw.pte, pteval, 1);
>>  
>>  		/* Update high watermark before we lower rss */
>>  		update_hiwater_rss(mm);
> 
>