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

[Dev Jain <dev.jain@arm.com>]

On 09/09/26 2:49 am, Barry Song wrote:
> On Tue, Sep 1, 2026 at 1:44 PM Dev Jain <dev.jain@arm.com> 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@arm.com>
>> ---
>>  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);

Can do this.


> 
>> +
>> +       /* 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.

Yes.


> 
> 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++);

Was following the pattern elsewhere ... I vaguely remember the
for (;;) being faster for some reason?

> 
> Best Regards
> Barry