Re: [PATCH v4 09/12] mm/rmap: Add batched version of folio_try_share_anon_rmap_pte

[David Hildenbrand (Arm)]

On 5/26/26 08:36, 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. 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.
> 
> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
> ---
>  include/linux/rmap.h | 52 +++++++++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index 8dc0871e5f001..64929490a7cfc 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -706,17 +706,18 @@ 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 = likely(!folio_is_device_private(folio));

We don't seem to have a lot of users of likely/unlikely when assigning
variables, so I wonder if the compiler really uses that information then.

git grep " = " | grep "likely(" | grep ";$" | wc -l
32


Maybe clearer to just push the "likely" into the actual two condition below.

[...]

>  /**
> - * folio_try_share_anon_rmap_pte - try marking an exclusive anonymous page
> - *				   mapped by a PTE possibly shared to prepare
> + * folio_try_share_anon_rmap_ptes - try marking exclusive anonymous pages
> + *				   mapped by PTEs possibly shared to prepare
>   *				   for KSM or temporary unmapping
>   * @folio:	The folio to share a mapping of
> - * @page:	The mapped exclusive page
> + * @page:	The first mapped exclusive page of the batch in the folio
> + * @nr_pages:	The number of pages to share in the folio (batch size)
>   *
>   * The caller needs to hold the page table lock and has to have the page table
>   * entries cleared/invalidated.
> @@ -797,11 +807,19 @@ static __always_inline int __folio_try_share_anon_rmap(struct folio *folio,
>   *
>   * Returns 0 if marking the mapped page possibly shared succeeded. Returns
>   * -EBUSY otherwise.
> + *
> + * The caller needs to hold the page table lock.

Isn't that stated further above?

>   */
> +static inline int folio_try_share_anon_rmap_ptes(struct folio *folio,
> +		struct page *page, unsigned long nr_pages)
> +{
> +	return __folio_try_share_anon_rmap(folio, page, nr_pages, PGTABLE_LEVEL_PTE);
> +}
> +
>  static inline int folio_try_share_anon_rmap_pte(struct folio *folio,
>  		struct page *page)
>  {
> -	return __folio_try_share_anon_rmap(folio, page, 1, PGTABLE_LEVEL_PTE);
> +	return folio_try_share_anon_rmap_ptes(folio, page, 1);
>  }
>  
>  /**

In general, LGTM.

-- 
Cheers,

David