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

[Dev Jain]

On 16/06/26 7:34 pm, David Hildenbrand (Arm) wrote:
> 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.

Ok.

> 
> [...]
> 
>>  /**
>> - * 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?

I'll drop it.

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