Re: [RFC PATCH 3/3] mm/mremap: Use can_pte_batch_count() instead of folio_pte_batch() for pte batch

[zhangqilong]

On 27.10.25 15:03, Zhang Qilong wrote:
> > In current mremap_folio_pte_batch(), 1) pte_batch_hint() always return
> > one pte in non-ARM64 machine, it is not efficient. 2) Next, it need to
> > acquire a folio to call the folio_pte_batch().
> >
> > Due to new added can_pte_batch_count(), we just call it instead of
> > folio_pte_batch(). And then rename mremap_folio_pte_batch() to
> > mremap_pte_batch().
> >
> > Signed-off-by: Zhang Qilong <zhangqilong3@xxxxxxxxxx>
> > ---
> >   mm/mremap.c | 16 +++-------------
> >   1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c index
> > bd7314898ec5..d11f93f1622f 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -169,27 +169,17 @@ static pte_t move_soft_dirty_pte(pte_t pte)
> >   		pte = pte_swp_mksoft_dirty(pte);
> >   #endif
> >   	return pte;
> >   }
> >
> > -static int mremap_folio_pte_batch(struct vm_area_struct *vma,
> > unsigned long addr,
> > +static int mremap_pte_batch(struct vm_area_struct *vma, unsigned long
> > +addr,
> >   		pte_t *ptep, pte_t pte, int max_nr)
> >   {
> > -	struct folio *folio;
> > -
> >   	if (max_nr == 1)
> >   		return 1;
> >
> > -	/* Avoid expensive folio lookup if we stand no chance of benefit. */
> > -	if (pte_batch_hint(ptep, pte) == 1)
> > -		return 1;
> > -
> > -	folio = vm_normal_folio(vma, addr, pte);
> > -	if (!folio || !folio_test_large(folio))
> > -		return 1;
> > -
> > -	return folio_pte_batch(folio, ptep, pte, max_nr);
> > +	return can_pte_batch_count(vma, ptep, &pte, max_nr, 0);
> >   }
> >
> >   static int move_ptes(struct pagetable_move_control *pmc,
> >   		unsigned long extent, pmd_t *old_pmd, pmd_t *new_pmd)
> >   {
> > @@ -278,11 +268,11 @@ static int move_ptes(struct
> pagetable_move_control *pmc,
> >   		 * make sure the physical page stays valid until
> >   		 * the TLB entry for the old mapping has been
> >   		 * flushed.
> >   		 */
> >   		if (pte_present(old_pte)) {
> > -			nr_ptes = mremap_folio_pte_batch(vma, old_addr,
> old_ptep,
> > +			nr_ptes = mremap_pte_batch(vma, old_addr, old_ptep,
> >   							 old_pte,
> max_nr_ptes);
> >   			force_flush = true;
> >   		}
> >   		pte = get_and_clear_ptes(mm, old_addr, old_ptep, nr_ptes);
> 
> get_and_clear_ptes() documents: "Clear present PTEs that map consecutive
> pages of the same folio, collecting dirty/accessed bits."

Oh, good catch. My focus was solely on the implementations of get_and_clear_ptes()
and set_ptes() and regarding their multi-folio PTEs handling, and I missed this comment.
get_and_clear_ptes() will collect dirty/accessed bits in batch ranges, and set in later.

> 
> And as can_pte_batch_count() will merge access/dirty bits, you would silently
> set ptes dirty/accessed that belong to other folios, which sounds very wrong.

Year, your analysis is very thorough. The access/dirty bit will be merged between
neighboring batched folios due to get_and_clear_ptes().

If caller don't want to merge access/dirty bits between folios(means not ignore any bits), 
they should call folio_pte_batch() or the new interface with 'flag | FPB_RESPECT_DIRTY '
(access bit is default be respected).

> 
> Staring at the code, I wonder if there is also a problem with the write bit, have
> to dig into that.

The write bit is handled similarly to dirty bit. If called don't want merge write between folios, we
could call the new interface with 'flag | FPB_RESPECT_WRITE', and pte_same() will compare the write bit
with the next neighboring folio pte.  If it's different, it will break.

Thanks for the in-depth review.
 
> --
> Cheers
> 
> David / dhildenb
>