Re: [PATCH v3 8/9] mm/rmap: Add batched version of folio_try_share_anon_rmap_pte

[David Hildenbrand (Arm)]

On 5/6/26 11:45, Dev Jain wrote:
> To enable batched unmapping of anonymous folios, we need to handle the
> sharing of exclusive pages. Hence, a batched version of
> folio_try_share_anon_rmap_pte is required.
> 
> Currently, the sole purpose of nr_pages in __folio_try_share_anon_rmap is
> to do some rmap sanity checks. Add helpers to clear the PageAnonExclusive
> bit on a batch of nr_pages. Note that __folio_try_share_anon_rmap can
> receive nr_pages == HPAGE_PMD_NR from the PMD path, but currently we only
> clear the bit on the head page. Retain this behaviour by setting
> nr_pages = 1 in case the caller is folio_try_share_anon_rmap_pmd.
> 
> While at it, convert nr_pages to unsigned long to future-proof from
> overflow in case P4D-huge mappings etc get supported down the road.
> I haven't made such a change in each function receiving nr_pages in
> try_to_unmap_one - perhaps this can be done incrementally.
> 
> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
> ---
>  include/linux/mm.h   | 11 +++++++++++
>  include/linux/rmap.h | 27 ++++++++++++++++++++-------
>  2 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 31e27ff6a35fa..0b77329cf57a4 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -243,6 +243,17 @@ static inline unsigned long folio_page_idx(const struct folio *folio,
>  	return page - &folio->page;
>  }
>  
> +static __always_inline void folio_clear_pages_anon_exclusive(struct page *page,
> +		unsigned long nr_pages)
> +{
> +	for (;;) {
> +		ClearPageAnonExclusive(page);
> +		if (--nr_pages == 0)
> +			break;
> +		++page;
> +	}
> +}

Something called folio that doesn't consume a folio is odd. I'd prefer we don't add this.

Is there a chance to simply change __folio_try_share_anon_rmap, so we get a single loop
inline?

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 8dc0871e5f00..5a1c874b2112 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -708,16 +708,13 @@ static inline int folio_try_dup_anon_rmap_pmd(struct folio *folio,
 static __always_inline int __folio_try_share_anon_rmap(struct folio *folio,
                struct page *page, int nr_pages, enum pgtable_level level)
 {
+       /* device private folios cannot get pinned via GUP. */
+       const bool pinnable = !folio_is_device_private(folio);
+
        VM_WARN_ON_FOLIO(!folio_test_anon(folio), folio);
        VM_WARN_ON_FOLIO(!PageAnonExclusive(page), folio);
        __folio_rmap_sanity_checks(folio, page, nr_pages, level);
 
-       /* device private folios cannot get pinned via GUP. */
-       if (unlikely(folio_is_device_private(folio))) {
-               ClearPageAnonExclusive(page);
-               return 0;
-       }
-
        /*
         * We have to make sure that when we clear PageAnonExclusive, that
         * the page is not pinned and that concurrent GUP-fast won't succeed in
@@ -760,19 +757,21 @@ static __always_inline int __folio_try_share_anon_rmap(struct folio *folio,
         * so we use explicit ones here.
         */
 
-       /* Paired with the memory barrier in try_grab_folio(). */
-       if (IS_ENABLED(CONFIG_HAVE_GUP_FAST))
-               smp_mb();
+       if (pinnable) {
+               /* Paired with the memory barrier in try_grab_folio(). */
+               if (IS_ENABLED(CONFIG_HAVE_GUP_FAST))
+                       smp_mb();
 
-       if (unlikely(folio_maybe_dma_pinned(folio)))
-               return -EBUSY;
+               if (unlikely(folio_maybe_dma_pinned(folio)))
+                       return -EBUSY;
+       }
        ClearPageAnonExclusive(page);
 
        /*
         * This is conceptually a smp_wmb() paired with the smp_rmb() in
         * gup_must_unshare().
         */
-       if (IS_ENABLED(CONFIG_HAVE_GUP_FAST))
+       if (pinnable && IS_ENABLED(CONFIG_HAVE_GUP_FAST))
                smp_mb__after_atomic();
        return 0;


-- 
Cheers,

David