Re: [PATCH v2 1/4] mm: gup: introduce gup_unshared_pages()

[Yeoreum Yun]

> On 11/2/26 12:30, Yeoreum Yun wrote:
> > This is preparatory patch to make gup_fast_pte_range() batchable.
> > 
> > Introduce gup_unshared_pages() to count consecutive pages within a folio
> > that do not require unsharing, and use it to implement gup_must_unshare().
> > 
> > Also add a folio argument to gup_must_unshare() so callers can reuse an
> > existing folio and avoid the compound_head() call implicit in PageAnon().
> > 
> > There is no functional change.
> > 
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> > ---
> >  mm/gup.c      | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
> >  mm/internal.h | 49 ++++++------------------------------------
> >  2 files changed, 70 insertions(+), 48 deletions(-)
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 7d43ce717b246..49681f63950ec 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -633,6 +633,59 @@ static struct page *no_page_table(struct vm_area_struct *vma,
> >  	return NULL;
> >  }
> >  
> > +unsigned long gup_unshared_pages(struct vm_area_struct *vma, unsigned int flags,
> > +				 struct folio *folio, struct page *page,
> > +				 unsigned long nr_pages)
> 
> Two tab indent.

Okay.

> 
> 
> This function should be static.

Acked.

> 
> As a prerequisite patch you should move gup_unshared_pages() into this file.
> 
> $ git grep gup_must_unshare
> include/linux/rmap.h:    * gup_must_unshare().
> include/linux/rmap.h:    * gup_must_unshare().
> mm/gup.c:       if (!pud_write(pud) && gup_must_unshare(vma, flags, page))
> mm/gup.c:       if (!pmd_write(pmdval) && gup_must_unshare(vma, flags, page))
> mm/gup.c:       if (!pte_write(pte) && gup_must_unshare(vma, flags, page)) {
> mm/gup.c:               if (!pte_write(pte) && gup_must_unshare(NULL, flags, page)) {
> mm/gup.c:       if (!pmd_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) {
> mm/gup.c:       if (!pud_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) {
> mm/internal.h:static inline bool gup_must_unshare(struct vm_area_struct *vma,

Might you want move gup_must_unshared() into gup.c from internal.h
I'll do.

> 
> 
> I'm not 100% happy about the function name due to the !folio_test_anon()+!FOLL_LONGTERM
> path, but cannot think of something better.

True.

> 
> > +{
> > +	unsigned int i;
> > +
> > +	/*
> > +	 * FOLL_WRITE is implicitly handled correctly as the page table entry
> > +	 * has to be writable -- and if it references (part of) an anonymous
> > +	 * folio, that part is required to be marked exclusive.
> > +	 */
> > +	if ((flags & (FOLL_WRITE | FOLL_PIN)) != FOLL_PIN)
> > +		return nr_pages;
> > +	/*
> > +	 * Note: PageAnon(page) is stable until the page is actually getting
> 
> You are not calling PageAnon() anymore.

Yes. I'll remove it.

> /* folio_test_anon(folio) is stable until folio gets freed. */
> 
> > +	 * freed.
> > +	 */
> > +	if (!folio_test_anon(folio)) {
> > +		/*
> > +		 * We only care about R/O long-term pining: R/O short-term
> > +		 * pinning does not have the semantics to observe successive
> > +		 * changes through the process page tables.
> > +		 */
> > +		if (!(flags & FOLL_LONGTERM))
> > +			return nr_pages;
> > +
> > +		/* We really need the vma ... */
> > +		if (!vma || vma_is_cow_mapping(vma))
> > +			return 0;
> > +
> > +		/*
> > +		 * ... because we only care about writable private ("COW")
> > +		 * mappings where we have to break COW early.
> > +		 */
> > +		return nr_pages;
> > +	}
> > +
> > +	/* Paired with a memory barrier in folio_try_share_anon_rmap_*(). */
> > +	if (IS_ENABLED(CONFIG_HAVE_GUP_FAST))
> > +		smp_rmb();
> > +
> > +	/*
> > +	 * Note that KSM pages cannot be exclusive, and consequently,
> > +	 * cannot get pinned.
> > +	 */
> > +	for (i = 0; i < nr_pages; i++) {
> > +		if (!PageAnonExclusive(page + i))
> > +			return i;
> > +	}
> > +
> > +	return nr_pages;
> > +}
> > +
> >  #ifdef CONFIG_PGTABLE_HAS_HUGE_LEAVES
> >  /* FOLL_FORCE can write to even unwritable PUDs in COW mappings. */
> >  static inline bool can_follow_write_pud(pud_t pud, struct page *page,
> > @@ -668,7 +721,8 @@ static struct page *follow_huge_pud(struct vm_area_struct *vma,
> >  	pfn += (addr & ~PUD_MASK) >> PAGE_SHIFT;
> >  	page = pfn_to_page(pfn);
> >  
> > -	if (!pud_write(pud) && gup_must_unshare(vma, flags, page))
> > +	if (!pud_write(pud) &&
> > +	    gup_must_unshare(vma, flags, page_folio(page), page))
> >  		return ERR_PTR(-EMLINK);
> 
> There is already another instance of page_folio(page) int his file. Use a local
> variable and have one folio lookup. (that's the whole reason for passing a folio)

Okay. I'll merge.

> 
> >  
> >  	ret = try_grab_folio(page_folio(page), 1, flags);
> > @@ -722,7 +776,8 @@ static struct page *follow_huge_pmd(struct vm_area_struct *vma,
> >  	if (pmd_protnone(*pmd) && !gup_can_follow_protnone(vma, flags))
> >  		return NULL;
> >  
> > -	if (!pmd_write(pmdval) && gup_must_unshare(vma, flags, page))
> > +	if (!pmd_write(pmdval) &&
> > +	    gup_must_unshare(vma, flags, page_folio(page), page))
> >  		return ERR_PTR(-EMLINK);
> 
> Dito.

Acked.

> 
> >  
> >  	VM_WARN_ON_ONCE_PAGE((flags & FOLL_PIN) && PageAnon(page) &&
> > @@ -846,7 +901,7 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
> >  	}
> >  	folio = page_folio(page);
> >  
> > -	if (!pte_write(pte) && gup_must_unshare(vma, flags, page)) {
> > +	if (!pte_write(pte) && gup_must_unshare(vma, flags, folio, page)) {
> >  		page = ERR_PTR(-EMLINK);
> >  		goto out;
> >  	}
> > @@ -2892,7 +2947,7 @@ static unsigned long gup_fast_pte_range(pmd_t pmd, pmd_t *pmdp,
> >  			goto pte_unmap;
> >  		}
> >  
> > -		if (!pte_write(pte) && gup_must_unshare(NULL, flags, page)) {
> > +		if (!pte_write(pte) && gup_must_unshare(NULL, flags, folio, page)) {
> >  			gup_put_folio(folio, 1, flags);
> >  			goto pte_unmap;
> >  		}
> > @@ -2969,7 +3024,8 @@ static unsigned long gup_fast_pmd_leaf(pmd_t orig, pmd_t *pmdp,
> >  		gup_put_folio(folio, nr_pages, flags);
> >  		return 0;
> >  	}
> > -	if (!pmd_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) {
> > +	if (!pmd_write(orig) &&
> > +	    gup_must_unshare(NULL, flags, folio, &folio->page)) {
> 
> 
> This only works because we don't have PMD mappings of things that are larger than PMDs.
> 
> While at it, pass the proper page.

Okay. I'll change with pmd_page(orig).

> 
> >  		gup_put_folio(folio, nr_pages, flags);
> >  		return 0;
> >  	}
> > @@ -3011,7 +3067,8 @@ static unsigned long gup_fast_pud_leaf(pud_t orig, pud_t *pudp,
> >  		return 0;
> >  	}
> >  
> > -	if (!pud_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) {
> > +	if (!pud_write(orig) &&
> > +	    gup_must_unshare(NULL, flags, folio, &folio->page)) {
> 
> Dito.

Acked.

> 
> >  		gup_put_folio(folio, nr_pages, flags);
> >  		return 0;
> >  	}
> > diff --git a/mm/internal.h b/mm/internal.h
> > index c53c369bf8b5d..35016b1afc8bf 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -1282,6 +1282,10 @@ int migrate_device_coherent_folio(struct folio *folio);
> >  int __must_check try_grab_folio(struct folio *folio, int refs,
> >  				unsigned int flags);
> >  
> > +unsigned long gup_unshared_pages(struct vm_area_struct *vma, unsigned int flags,
> > +				 struct folio *folio, struct page *page,
> > +				 unsigned long nr_pages);
> > +
> >  /*
> >   * mm/huge_memory.c
> >   */
> > @@ -1354,51 +1358,12 @@ enum {
> >   * to fallback to the slow path just to lookup the vma.
> >   */
> >  static inline bool gup_must_unshare(struct vm_area_struct *vma,
> > -				    unsigned int flags, struct page *page)
> > +				    unsigned int flags,
> > +				    struct folio *folio, struct page *page)
> 
> Whatever you touch, please also convert to two-tab indent.

Okay.

> 
> -- 
> Cheers,
> 
> David

-- 
Sincerely,
Yeoreum Yun