Re: [PATCH v2 4/8] mm/rmap: Add batched version of folio_try_share_anon_rmap_pte

[Barry Song]

On Tue, Sep 1, 2026 at 1:44 PM Dev Jain <dev.jain@xxxxxxx> 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. Now, clear the PageAnonExclusive bit on a
> batch of nr_pages. Refactor the function such that the clearing of the bit
> can be done at one place without duplication.
>
> 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.
>
> Add two WARN's: check that the batch is entirely exclusive (for PMD
> callers, need to check only head page), and that there are only
> PTE/PMD paths converging into __folio_try_share_anon_rmap.
>
> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
> ---
>  include/linux/rmap.h | 56 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index 0b332770abeed..320f9f14f6020 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -706,17 +706,23 @@ 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)
> +               struct page *page, unsigned long 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;
> -       }

Somehow, I feel the early return for
`folio_is_device_private(folio)` is more readable. Can we keep it?
Then we can avoid many `if (pinnable)` checks later.

> +       VM_WARN_ON_ONCE(level > PGTABLE_LEVEL_PMD);

Maybe the below would be better, as it avoids depending on the
exact value of `PGTABLE_LEVEL_PMD` and above.

VM_WARN_ON_ONCE(level != PGTABLE_LEVEL_PTE && level != PGTABLE_LEVEL_PMD);

> +
> +       /* We only clear anon-exclusive from head page of PMD folio. */
> +       if (level == PGTABLE_LEVEL_PMD)
> +               nr_pages = 1;
> +
> +       VM_WARN_ON_FOLIO(page_anon_exclusive_batch(0, nr_pages, page, true) != nr_pages, folio);
>
>         /*
>          * We have to make sure that when we clear PageAnonExclusive, that
> @@ -760,29 +766,38 @@ 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 (likely(pinnable)) {
> +               /* Paired with the memory barrier in try_grab_folio(). */
> +               if (IS_ENABLED(CONFIG_HAVE_GUP_FAST))
> +                       smp_mb();

If we return early for `!pinnable`, shouldn't we be able to avoid
this? Is the reason you don't do the early return that you want to
batch the `folio_is_device_private(folio)` case as well? If so,
that seems sensible.

Is this a real use case that you're supporting with your patchset?

>
> -       if (unlikely(folio_maybe_dma_pinned(folio)))
> -               return -EBUSY;
> -       ClearPageAnonExclusive(page);
> +               if (unlikely(folio_maybe_dma_pinned(folio)))
> +                       return -EBUSY;
> +       }
> +
> +       for (;;) {
> +               ClearPageAnonExclusive(page);
> +               if (--nr_pages == 0)
> +                       break;
> +               page++;
> +       }

Maybe ?

     while (nr_pages--)
         ClearPageAnonExclusive(page++);

Best Regards
Barry