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

[Dev Jain]

On 11/05/26 1:43 pm, David Hildenbrand (Arm) wrote:
> 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;
> 
> 

Okay this is better! I'll do this.