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

[Barry Song]

On Fri, Sep 25, 2026 at 7:09 PM Dev Jain <dev.jain@xxxxxxx> wrote:
>
>
>
> On 25/09/26 11:00 am, Barry Song wrote:
> > On Thu, Sep 24, 2026 at 9:11 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>
> >
> > The patch looks correct to me, but it seems a bit hard to follow
> > because of the `pinnable` handling.
> >
> >> ---
> >>  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 62ef511a6175a..91b5763ef466f 100644
> >> --- a/include/linux/rmap.h
> >> +++ b/include/linux/rmap.h
> >> @@ -723,17 +723,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);
> >> +
> >
> > If we make a helper that does:
> >
> > for (;;) {
> >      ClearPageAnonExclusive(page);
> >      if (--nr_pages == 0)
> >                break;
> >      page++;
> > }
> >
> > wouldn't that allow us to avoid this ugly `pinnable` variant?
> >
> > Then we can make a much smaller modification, which should
> > also be much easier to follow? No?
>
> David had suggested here:
>
> https://lore.kernel.org/all/95b8224d-3ed4-4fe3-9954-d5ba0aa373f8@xxxxxxxxxx/
>
> The argument was that the helper would have no context of the pages we are
> operating on. Although you are correct that this makes the function harder
> to follow.
>
> Maybe, revert to the original function, and have this helper
> folio_clear_pages_anon_exclusive, which will take the folio and do
> the page_anon_exclusive_batch = nr_pages sanity check, and add
> kerneldoc to it which says pages should be within folio?
>
> The kerneldoc can be saved by calling __folio_rmap_sanity_checks in
> folio_clear_pages_anon_exclusive - that will cause duplication, but
> then it's all a VM_WARN_ON function so maybe we don't care?
>

Never mind. If you and David have already discussed this, then it is
all good to go with your current code. Everyone has a different
coding style and taste :-)

For the patch, feel free to add:

Reviewed-by: Barry Song <baohua@xxxxxxxxxx>