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

[Dev Jain]

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?


> 
> Best Regards
> Barry