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

["David Hildenbrand (Arm)" <david@kernel.org>]

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@arm.com>
> ---
>  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.


This function should be static.

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,


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

> +{
> +	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.

/* 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)

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

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

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

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

-- 
Cheers,

David